Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: fix asynchronous_mail_send #953

Closed
wants to merge 3 commits into from
Closed

Conversation

Taywee
Copy link

@Taywee Taywee commented Nov 20, 2020

Fixes

The previous solution was not properly asynchronous despite using an event loop and async functions (as mentioned in this comment on #363 and a few comments in #296). It had an async function that didn't actually await a future, meaning it would still block the entire event loop on every send. This uses the event loop executor to background it onto an executor thread, which will still at least let IO properly parallelize while the GIL is released.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • [N/A] I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • [N/A] I have added inline documentation to the code I modified

The previous solution was still a blocking solution.  It had an async
function that didn't actually await a future, meaning it would still
block the entire event loop.  This uses the event loop executor to
background it onto an executor thread, which will still at least let IO
properly parallelize while the GIL is released.
@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Nov 20, 2020
@stalkerg
Copy link

Crating new threads it's not what you expected from the async approach.

@Taywee
Copy link
Author

Taywee commented Nov 23, 2020

@stalkerg
This doesn't necessarily create a new thread, this uses the existing event loop's executor, which is by default a ThreadPoolExecutor. This is a part of Python's asyncio for a reason. ThreadPoolExecutor reuses idle threads rather than starting up new ones when it can. Creating new threads can easily be what you'd expect in an async approach. Tons of asynchronous event loops use threading behind the scenes already to coordinate their working and waiting, and asynchronous filesystem access is often done with threading too because most OSes don't present asynchronous APIs for filesystem work.

Some code is difficult to integrate into existing asyncio code because it is already written for blocking IO. Using threading can enable it to run in parallel because it's only waiting on IO anyway, but you can't just use the Python threading module, because you have no safe way to await it. That's exactly why asyncio provides an executor for you.

I understand it's not an ideal solution, but the ideal solution would be this library presenting an official async API, which is not a small ask. This is a proper way to integrate it into async code and allow it proper async parallelism in about the best way you can.

Either way, creating new threads may not be what you expect from async code, but neither is simply blocking the event loop for the entire IO and not doing any asynchronous operation at all, which is what the previous example actually did.

# Python 3.6+ compatibility
from asyncio import get_event_loop as get_running_loop

def async_run(future):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be within the except? Or top level?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within the except. asyncio.run is imported as async_run, but asyncio.run only exists for Python 3.7. The version within the except is a rough implementation of asyncio.run for when it's absent (which would be the case for Python 3.6).

@stalkerg
Copy link

This doesn't necessarily create a new thread, this uses the existing event loop's executor, which is by default a ThreadPoolExecutor.

I mostly thought about context switch, GIL, and ThreadPoolExecutor itself.

Tons of asynchronous event loops use threading behind the scenes already to coordinate their working and waiting, and asynchronous filesystem access is often done with threading too because most OSes don't present asynchronous APIs for filesystem work.

Yes, but:

  1. We talking currently about network communication what in most cases async.
  2. Main OS like Linux, Windows support async access to FS now. (effectiveness of this is a different question)

I understand it's not an ideal solution, but the ideal solution would be this library presenting an official async API, which is not a small ask.

But it's the main ask. ) run lib in thread executor or work directly with REST API we can easily without additional explanations.
It's can just postpone proper asyncio integration into the library.
Looks like real code with blocking API in the library not much it should be easy to refactoring, one-week maximum.

@eshanholtz
Copy link
Contributor

@stalkerg In your opinion, do you feel this PR is an improvement over the async guidance that already exists in this library?

@stalkerg
Copy link

@eshanholtz yes, it's should improve current guidance, because currently, it's looking wrong for python >=3.6.
But at the same time, we should mention what this library is not async/await yet, and the current approach just a temporal workaround (hack).

As I understand you can represent Twillio, do you have a plan to prepare the true async/await version?

@Taywee
Copy link
Author

Taywee commented Nov 27, 2020

I wouldn't call it a hack (it's not nearly clever enough to be a hack; it's just throwing a blocking method into the executor), but it is definitely a temporary workaround. I personally think a good solution is what @stalkerg suggested here in the async ticket, because that will offload the responsibility of managing a good async API out of this library and let the end user use whatever async networking infrastructure they want. It is also easy to refactor the existing library around that plumbing-level API to avoid introducing code duplication while still presenting the same convenient high-level API.

Copy link
Contributor

@thinkingserious thinkingserious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are a few more changes based on my local tests:

  1. Add this import: from python_http_client import exceptions

  2. Change:

plain_text_content = Content("text/plain", "This is asynchronous sending test.")
html_content = Content("text/html", "<strong>This is asynchronous sending test.</strong>")

to

content = Content("text/plain", "This is asynchronous sending test.")

  1. Change:
partial(
    sendgrid_client.send,
    request_body=email))

to

partial(
    sendgrid_client.send,
    email))
  1. Change:

except urllib.error.HTTPError as e: to except exceptions.BadRequestsError as e:

  1. Add:

print("Note that the response.body will be empty on a 202 status code.")

above

if response.status_code < 300:
    print("Email #{} processed".format(n), response.body, response.status_code)

@thinkingserious thinkingserious added status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap and removed status: code review request requesting a community code review or review from Twilio labels Dec 3, 2020
@JenniferMah
Copy link
Contributor

Closing until PR feedback is addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants