Fix coroutine wrapper to await after exception

[stalkerg]

This PR fixes #7117 and similar issues. Short explanation - our coroutine wrapper does not properly handle the exception, which breaks coroutine handling. As you can see, any task expects results from throw - https://github.com/python/cpython/blob/main/Lib/asyncio/tasks.py#L303 but it seems like in aiohttp it was acidently removed by this commit stalkerg@f04ecc2#diff-f334e752b4894ef951105572ab8b195aeb8db90eb6e48b1dfbd9a01da4c854f5L842

This is repro a case without aiohttp:

import ssl
import collections


class TestCoroWrapper(collections.abc.Coroutine):
    __slots__ = ("_coro", "_resp")
    def __init__(self, coro):
        self._coro = coro

    def __getattr__(self, attr):
        return getattr(self._coro, attr)

    def send(self, arg):
        return self._coro.send(arg)

    def throw(self, arg):
        self._coro.throw(arg)

    def close(self):
        return self._coro.close()

    def __await__(self):
        ret = self._coro.__await__()
        return ret

async def ssl_call(context):
    loop = asyncio.get_event_loop()
    return await loop.create_connection(
        lambda: asyncio.Protocol(),
        '2404:6800:4004:824::2004',
        443,
        ssl=context,
        family=socket.AddressFamily.AF_INET6,
        proto=6,
        flags=socket.AddressInfo.AI_NUMERICHOST | socket.AddressInfo.AI_NUMERICSERV,
        server_hostname='www.google.com',
        local_addr=None
    )

async def prepare_call():
    context = ssl.create_default_context()
    try:
        connection = await ssl_call(context)
    except ssl.SSLError as e:
        print(f"Got exception1: {e}")
        raise e

    return connection

async def my_task():
    try:
        await prepare_call()
    except Exception as e:
        print(f"Got exception2: {e}")

    await asyncio.sleep(1)
    raise Exception("test")

async def main():
    my_coro = TestCoroWrapper(my_task())
    print(f"is coro? {asyncio.iscoroutine(my_coro)}")
    task = asyncio.create_task(my_coro)
    await task

asyncio.run(main())

The TestCoroWrapper here is equivalent of _BaseRequestContextManager. If you run such code like: SSL_CERT_FILE=/dev/null SSL_CERT_DIR=/dev/null python test.py you will get an error: await wasn't used with future.
The main idea here is that you are trying to await the sleep function after getting and catching an exception from the native (SSL) module.

Now I should explain why repro with aiohttp for some users return the same error:

import asyncio
import aiohttp

async def main():
    async with aiohttp.ClientSession() as session:
        try:
            response = await asyncio.ensure_future(session.get('https://www.google.com/'))
            print(await response.text())
        finally:
            response.release()

asyncio.run(main())

here it's happened because in TCPConnector._create_direct_connection we are getting all IPs for the given host and trying to connect one by one. If the first connection gets an error we will catch this error and try again for the next IP. If you have IPv6 you will have at least 2 IPs here (ipv6 and ipv4), and after the first error, you will try to connect to a second IP and get the same error.

Why it's problem only for asyncio.ensure_future? Because asyncio.ensure_future creates a task such a task starts processing coroutines and directly communicates with our coroutine wrapper witch not return a result for throw.

I will write changelog and etc after people validate this PR. But honestly, I don't think I can write a unit test for a such case.

Regards,

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[Dreamsorcerer]

It seems rather awkward that we're subclassing Coroutine in the first place. Would be nice if we can refactor it out entirely. But, atleast the reported issue is fixed for now. Thanks for digging into this.

[stalkerg]

Yeah, such a wrapper can have also a small performance penalty because usually coroutine is a C object.

[Dreamsorcerer]

Yeah, such a wrapper can have also a small performance penalty because usually coroutine is a C object.

Well, if there's any chance you get time to look at that, that'd be great. I'll merge this one in for now.

[stalkerg]

Well, if there's any chance you get time to look at that, that'd be great. I'll merge this one in for now.

okey, should I also add to CONTRIBUTORS.txt?

Also, I found what this signature is not fully correct, Python support two signatures for throw https://docs.python.org/3/reference/datamodel.html#coroutine.throw

Seems like function should be:

    def throw(self, *args, **kwargs) -> Any:
        return self._coro.throw(*args, **kwargs)

[Dreamsorcerer]

okey, should I also add to CONTRIBUTORS.txt?

Oh, whatever you like, I've not even bothered to add myself. :P

[patchback]

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply a57dc31 on top of patchback/backports/3.9/a57dc3146839e2e1978b96db48e1de3af1a2bb50/pr-7785

Backporting merged PR #7785 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.9/a57dc3146839e2e1978b96db48e1de3af1a2bb50/pr-7785 upstream/3.9

4. Now, cherry-pick PR Fix coroutine wrapper to await after exception #7785 contents into that branch:

   $ git cherry-pick -x a57dc3146839e2e1978b96db48e1de3af1a2bb50

   If it'll yell at you with something like fatal: Commit a57dc3146839e2e1978b96db48e1de3af1a2bb50 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x a57dc3146839e2e1978b96db48e1de3af1a2bb50

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Fix coroutine wrapper to await after exception #7785 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.9/a57dc3146839e2e1978b96db48e1de3af1a2bb50/pr-7785

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[stalkerg]

Thanks!