Async context manager for client can be sync.

[dfee]

It seems that since aiohttp's ClientSession's close() is no longer asynchronous, we don't need the async context manager for it... and we also don't need the async context manager for aiobotocore's client.

https://github.com/aio-libs/aiobotocore/blob/master/aiobotocore/client.py#L141 https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L43

Can we update the code?

[asvetlov]

That's wrong. ClientSession.close() are going from sync to async. We are in the middle of translation period now but .close() will definitely become a pure async coroutine up to end of year.

[thehesiod]

Agreed. We don't want to pidgin hole ourselves into sync when async may later be required. Closing unless there's a good reason to reopen. Also the outer is important to close the session and thus all the pool connections. Please don't say it's not needed if you don't understand why it was put there to begin with :)

[dfee]

@asvetlov very interesting!

@thehesiod humor me, please.

Please don't say it's not needed if you don't understand why it was put there to begin with :)

Why is an async context manager needed for aiobotocore today? Indeed, a sync context manager would do fine (today), but as mentioned above, the current design provides scaffolding for future architecture.

Maybe you read a bit too quickly, or were a bit too brutish in your comment?

[thehesiod]

@dfee sorry for being snappy, just finding too many bugs recently.

When ending a request context, one of the following can happen: 1) the connection can be immediately released to the connection pool if the body was read (no wait) 2) the connection has to be closed and a new connection created (body not read of client closed connection). In this scenario ideally aiohttp would wait for the connection to close (it currently does not and thus can cause the number of open connections to keep growing..and yes I hit this before via SSL sockets).

Currently aiohttp does not wait but ideally in the future it will to keep the # of pool connections below the limit.

I'm going to re-open this so that we add a comment as to why we use an async context, how does that sound? In general though I don't understand the request, how would making the context class sync help you? It does not solve any problem or performance issue, and in fact creates forwards compatibility issues. In fact aiopg has this problem right now and will cause people to have to re-write code going forward to behave "correctly" in terms of connection release and reuse.

[dfee]

It really only helps in the case of using it in a pytest fixture.

Even using @pytest.mark.asyncio (from pytest-asyncio), we can't use async context managers in fixtures because there is no way to yield from within an async block (the fixtures themselves can't be coroutines).

Though I guess I could manually call __aenter__ and __aexit__ via event_loop.run_until_complete() I wanted to see what was actually happening under the covers.

[thehesiod]

ah, ya sounds like pytest needs an enhancement then because this is a crucial feature of asyncio :) Have you thought of using asynctest? That's what I use for this type of test. I'll do a little more googling re pytest.

[dfee]

Apparently async fixtures are supported now... https://github.com/pytest-dev/pytest-asyncio#async-fixtures

[thehesiod]

oh yay! =)

[jettify]

Works only for py3.6 :)

[thehesiod]

hmm, I tried on 3.5 seems to work

[jettify]

Have you used yield ? That keyword only possible in python3.6 https://www.python.org/dev/peps/pep-0525/

[thehesiod]

oh ya, I was just testing being able to write a test as async def so you can use a async with. Note to dfee you can make the test as optional by version by using textwrap.detent and wrapping it with a version test, I stole that idea from the ratelimiter project :) Another thing is via tox or whatever only running certain test files with certain versions of python.

[jettify]

can we close this issue?

[thehesiod]

we just need to add a comment why it's an async context manager

[jettify]

Closing for now, feel free to open new issue