bpo-41229: Update docs for explicit aclose()-required cases and add contextlib.aclosing() method #21545

achimnol · 2020-07-19T10:47:01Z

This is a PR to:

Add contextlib.aclosing which ia analogous to contextlib.closing but for async-generators with an explicit test case for bpo-41229
Update the docs to describe when we need explicit aclose() invocation.

which are motivated by the following issues, articles, and examples:

Particuarly regarding PEP-533, its acceptance (__aiterclose__()) would make this little addition of contextlib.aclosing() unnecessary for most use cases, but until then this could serve as a good counterpart and analogy to contextlib.closing(). The same applies for contextlib.closing with __iterclose__().
Also, still there are other use cases, e.g., when working with non-generator objects with aclose() methods.

https://bugs.python.org/issue41229

Automerge-Triggered-By: GH:aeros

achimnol · 2020-07-19T11:03:26Z

@njsmith I just read your comments and resolution of bpo-41229, and I also see that async-gens in the original leak.py were not being gc'ed because the event loop is indefinitely blocked by input().
Still, I think it's worth to add this PR, so please let me know what you think about this. :)

aeros · 2020-07-19T19:56:19Z

Adding DO-NOT-MERGE for now, so it's not merged prior to approval of the contextlib.aclosing() feature. Without an actual memory leak occurring in async generators, this is now a more subjective enhancement to be argued for, rather than a bugfix.

(I'm not currently +1 or -1 for the feature, I have to spend some more time considering the use cases.)

achimnol · 2020-07-20T02:12:48Z

I think the major issue for contextlib.aclosing would be whether people will use the name "aclose" in their objects to async-cleanup resources.

There are currently the following approaches I see:

obj.close() – e.g., zmq.asyncio.Socket
await obj.close() – e.g., aiohttp.WebSocketResponse, aiofiles.AIOFile
obj.close() followed by await obj.wait_closed() – e.g., asyncio.StreamWriter, aioredis.RedisConnection
await obj.aclose() – e.g., async generator, httpx.AsyncClient, trio.SocketStream

So, from the perspective of naming, adding contextlib.aclosing to the stdlib might be promoting the last case for non-generator use cases as well. Supporting all 2nd, 3rd, 4th cases above in contextlib.aclosing would make things complicated.

Still, I believe that we need a better approach so that people don't be confused about resource cleanup upon generator termination/interruption, and currently the only reliable and predictable way is to explicitly close the generator (for both sync/async generators). Having both contextlib.closing and contextlib.aclosing provides a consistent, idiomatic way to achieve explicit cleanup of both sync/async generators, and that's the main reason why I made this PR.

For the @njsmith's response to the original issue, I'd like to say relying on the event loop's behavior or GC is counter-intuitive. For instance, even an experienced developer like him was surprised at his first glance on this issue and after some examination he concluded that this was "actually" an expected behavior.

I don't know how things are going on with PEP-533, but if it's going to be accepted in the future, I can agree with skipping contextlib.aclosing because it defines the resource cleanup behavior clearly for sync/async generators (the most motivating issue here) and I have no strong opinion to insist that everybody should use "aclose" instead of "close".

1st1 · 2020-07-23T01:44:16Z

I think the major issue for contextlib.aclosing would be whether people will use the name "aclose" in their objects to async-cleanup resources.

We're going to start prompting aclose() in asyncio APIs soon.

1st1

+1 from me to add this.

corona10

@aeros
Anything more stuff is needed before merging this PR?

aeros

Mostly LGTM. A few nits though:

The docs mention aclosing as a function, when in Lib/contextlib.py it's implemented as a class. As a result, it should use the .. class:: reST role instead of .. function::. Also, instead of "return an async context manager", it would be more correct to directly call it an async context manager [1].
In the docstring for aclosing(), I would suggest for it to be changed to something like: "Async context manager for safely finalizing an async generator using its aclose() method". I think the current version of the docstring may cause readers to miss the main point of it, and potentially make incorrect assumptions about its usage. Also, the "end of a block" part is a bit vague [2]. Instead, it makes more sense to me to just refer to it as a context manager.
I think this would also benefit from a "What's New" entry in 3.10 for contextlib to briefly mention the addition of contextlib.aclosing(), as it is a new public member.

Other than the above points, I think it's good to go. Feel free to @ mention me once the changes are made, and I can proceed with merging it (unless @1st1 is 100% content with the current state, and decides to merge it as is).

[1] - Any class that implements __aenter__ and __aexit__ is considered an async context manager, or at the very least is usable as one.

[2] - I get that you're referring to the block/scope within the with statement, but I don't think that will be immediately clear to all readers; especially if they're not intimately familiar with context managers in general. By simply referring to it as an "async context manager", readers can much more easily look up how context managers work rather than trying to guess what "context" and "block" are referring to within the docstring.

bedevere-bot · 2020-08-18T21:55:05Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

achimnol · 2020-10-01T16:08:10Z

@aeros Thanks for your feedback. Updated the PR, but I have some questions regarding existing docs/docstrings for closing, to make it consistent or not.

Mostly LGTM. A few nits though:

The docs mention aclosing as a function, when in Lib/contextlib.py it's implemented as a class. As a result, it should use the .. class:: reST role instead of .. function::. Also, instead of "return an async context manager", it would be more correct to directly call it an async context manager [1].

I updated the docs to use .. class::. However, I see the same issue on the existing docs for closing, because I copy-and-pasted it to start with. Shall I update it as well?

In the docstring for aclosing(), I would suggest for it to be changed to something like: "Async context manager for safely finalizing an async generator using its aclose() method". I think the current version of the docstring may cause readers to miss the main point of it, and potentially make incorrect assumptions about its usage. Also, the "end of a block" part is a bit vague [2]. Instead, it makes more sense to me to just refer to it as a context manager.

Agreed and applied your suggestion, but the same question: shall I update the docstring for existing closing() to be consistent?

I think this would also benefit from a "What's New" entry in 3.10 for contextlib to briefly mention the addition of contextlib.aclosing(), as it is a new public member.

I have already added the news fragment using blurb.
Please let me know if there are something I've missed.

Other than the above points, I think it's good to go. Feel free to @ mention me once the changes are made, and I can proceed with merging it (unless @1st1 is 100% content with the current state, and decides to merge it as is).

[1] - Any class that implements __aenter__ and __aexit__ is considered an async context manager, or at the very least is usable as one.

[2] - I get that you're referring to the block/scope within the with statement, but I don't think that will be immediately clear to all readers; especially if they're not intimately familiar with context managers in general. By simply referring to it as an "async context manager", readers can much more easily look up how context managers work rather than trying to guess what "context" and "block" are referring to within the docstring.

belm0 · 2020-10-10T23:17:08Z

Neglecting to mention https://bugs.python.org/issue40213 in the PR description has cost me several hours of duplicated effort in #22640 😦

Anyway I'll add comments here based on thought I've put into it.

belm0 · 2020-10-10T22:57:06Z

Doc/library/contextlib.rst

+      from contextlib import asynccontextmanager
+
+      @asynccontextmanager
+      def aclosing(thing):


should be async def

belm0 · 2020-10-10T23:01:07Z

Doc/library/contextlib.rst

+   And lets you write code like this::
+
+      async def ticker(delay, to):
+          for i in range(to):
+              yield i
+              await asyncio.sleep(delay)
+
+      with aclosing(ticker(10)) as ticks:
+          async for tick in ticks:
+              print(tick)
+
+   without needing to explicitly call the ``aclose()`` method of ``ticks``.
+   Even if an error occurs, ``ticks.aclose()`` will be called when
+   the :keyword:`async with` block is exited.


I think rather than some vague example, this is the right place to mention how it's useful for deterministic async generator cleanup, and use that as an example.

See https://github.com/python/cpython/pull/22640/files#diff-aa53aa6efed705fd0589ce0fdac405d7R171-R185

Replaced the example with #22640!

belm0 · 2020-10-10T23:10:43Z

Doc/reference/expressions.rst

+If an asynchronous generator is not resumed before it reaches the end of function
+but the event loop where the generator is bound to continues to run (e.g., when
+the caller task is cancelled), the caller must explicitly close the async
+generator by calling :meth:`~agen.aclose` method to free the resources allocated
+in the async generator function's stack and detach the generator from the event
+loop.
+


It could be explained more clearly-- most readers won't have in mind how this situation comes up. And the rationale could be more detailed.

If an asynchronous generator happens to exit early by :keyword:break, the caller task being cancelled, or other exception, the generator's async cleanup code will run and possibly raise exceptions or access context variables in an unexpected context-- perhaps after the lifetime of tasks it depends on or the event loop itself. To prevent this, the caller must explicitly close the async generator by calling :meth:~agen.aclose method to finalize the generator and ultimately detach it from the event loop.

Subtlety: It can't happen after the event loop exits, thanks to the async gen gc hooks.

belm0 · 2020-10-10T23:11:24Z

Lib/contextlib.py

+
+    is equivalent to this:
+
+        agen = <module>.fetch(<arguments>)


await fetch()

belm0 · 2020-10-10T23:13:27Z

Misc/NEWS.d/next/Library/2020-07-19-20-10-41.bpo-41229.p8rJa2.rst

+Add ``contextlib.aclosing`` analogous to ``contextlib.closing`` but for
+async generators and arbitrary objects with an ``aclose`` coroutine method.


it would be useful to mention the primary motivation (deterministic cleanup of async generators)

1st1 · 2020-10-12T17:47:33Z

There's a bunch of good feedback from @belm0 -- please address and I think the PR is OK to be merged now. We can polish it in follow up PRs. @aeros please re-review.

aeros · 2020-10-17T08:25:27Z

please address and I think the PR is OK to be merged now. We can polish it in follow up PRs. @aeros please re-review. @aeros please re-review.

Sounds good, I'll look through it as soon as I get the chance. If I'm not able to by this weekend, I'll definitely be able to during the core sprint next week.

belm0 · 2020-10-17T10:17:24Z

Lib/contextlib.py

@@ -303,6 +303,31 @@ def __exit__(self, *exc_info):
        self.thing.close()


+class aclosing(AbstractAsyncContextManager):
+    """Async context manager for safely finalizing an async generator using its ``aclose()`` method.


Is this description too specific? aclosing can be used with any object having aclose(), not necessarily a generator.

Updated a little to embrace other cases.

belm0 · 2020-10-17T10:22:47Z

Lib/contextlib.py

+
+    is equivalent to this:
+
+        agen = await <module>.fetch(<arguments>)


Sorry, I said the await should be here, but that was incorrect.

The object passed to aclosing() is just taken as is, returned by __aenter__, and has aclose() called in __aexit__.

Oh, I also missed to check it. Reverted here.

belm0

Thank you!

corona10

@aeros Gentle ping~

aeros

Thanks for the reminder @corona10, for the suggestions @belm0, and for the changes made @achimnol! LGTM, I'll proceed with merging.

(The only remaining component that would also be nice is a mention of the new contextlib.aclosing() in the 3.10 whatsnew document. But that can be done in a separate PR -- if you open one, I'd suggest linking to it here and requesting a review from me)

…ontextlib.aclosing() method (pythonGH-21545) This is a PR to: * Add `contextlib.aclosing` which ia analogous to `contextlib.closing` but for async-generators with an explicit test case for [bpo-41229]() * Update the docs to describe when we need explicit `aclose()` invocation. which are motivated by the following issues, articles, and examples: * [bpo-41229]() * https://github.com/njsmith/async_generator * https://vorpus.org/blog/some-thoughts-on-asynchronous-api-design-in-a-post-asyncawait-world/#cleanup-in-generators-and-async-generators * https://www.python.org/dev/peps/pep-0533/ * https://github.com/achimnol/aiotools/blob/ef7bf0cea7af/src/aiotools/context.py#L152 Particuarly regarding [PEP-533](https://www.python.org/dev/peps/pep-0533/), its acceptance (`__aiterclose__()`) would make this little addition of `contextlib.aclosing()` unnecessary for most use cases, but until then this could serve as a good counterpart and analogy to `contextlib.closing()`. The same applies for `contextlib.closing` with `__iterclose__()`. Also, still there are other use cases, e.g., when working with non-generator objects with `aclose()` methods.

uburuntu · 2021-09-29T09:08:23Z

😒
#21285
https://bugs.python.org/issue41197

achimnol added 2 commits July 19, 2020 19:19

Add contextlib.aclosing() and update docs for explicit aclose()

83ac6e7

Add test case

a9b2d91

achimnol requested a review from 1st1 as a code owner July 19, 2020 10:47

the-knights-who-say-ni added the CLA signed label Jul 19, 2020

bedevere-bot added the awaiting review label Jul 19, 2020

achimnol added 2 commits July 19, 2020 20:07

Fix typo

25556e1

Add NEWS fragment

65faafe

corona10 requested a review from njsmith July 19, 2020 16:48

aeros added DO-NOT-MERGE topic-asyncio type-feature A feature request or enhancement labels Jul 19, 2020

1st1 approved these changes Jul 23, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jul 23, 2020

aeros removed the DO-NOT-MERGE label Jul 23, 2020

corona10 requested a review from aeros August 16, 2020 11:04

corona10 reviewed Aug 16, 2020

View reviewed changes

aeros suggested changes Aug 18, 2020

View reviewed changes

bedevere-bot removed the awaiting merge label Aug 18, 2020

bedevere-bot added the awaiting changes label Aug 18, 2020

Apply aeros' review comments

aeae633

corona10 mentioned this pull request Oct 10, 2020

bpo-40213: add contextlib.aclosing() #22640

Closed

belm0 suggested changes Oct 10, 2020

View reviewed changes

achimnol added 3 commits October 17, 2020 15:47

Fix missing async/await as reviewed by belm0

53c881d

Use better example from python#22640 and update async-gen expr doc

351b85c

Mention the primary motivation for this PR

69ce971

aeros self-requested a review October 17, 2020 08:25

belm0 reviewed Oct 17, 2020

View reviewed changes

achimnol added 3 commits October 18, 2020 00:12

Add contributor names in the changelog.

e2249a5

Generalize aclosing docstring and revert mistakenly added await keyword

32e7c92

De-pluralize the docstring since aclosing applies to one object

6adce3d

belm0 approved these changes Oct 18, 2020

View reviewed changes

corona10 reviewed Nov 1, 2020

View reviewed changes

aeros approved these changes Nov 2, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Nov 2, 2020

aeros added the 🤖 automerge label Nov 2, 2020

miss-islington merged commit 6e8dcda into python:master Nov 2, 2020

bedevere-bot removed the awaiting merge label Nov 2, 2020

achimnol deleted the bpo-41229 branch November 10, 2020 07:41

achimnol mentioned this pull request Nov 10, 2020

bpo-41229: Add a whatsnew entry about contextlib.aclosing #23217

Merged

njsmith mentioned this pull request Sep 29, 2021

bpo-41197: Async magic methods in contextlib.closing #21285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-41229: Update docs for explicit aclose()-required cases and add contextlib.aclosing() method #21545

bpo-41229: Update docs for explicit aclose()-required cases and add contextlib.aclosing() method #21545

achimnol commented Jul 19, 2020 •

edited by miss-islington

Loading

achimnol commented Jul 19, 2020 •

edited by bedevere-bot

Loading

aeros commented Jul 19, 2020

achimnol commented Jul 20, 2020 •

edited

Loading

1st1 commented Jul 23, 2020

1st1 left a comment

corona10 left a comment •

edited

Loading

aeros left a comment •

edited

Loading

bedevere-bot commented Aug 18, 2020

achimnol commented Oct 1, 2020 •

edited

Loading

belm0 commented Oct 10, 2020 •

edited

Loading

belm0 Oct 10, 2020

achimnol Oct 17, 2020

belm0 Oct 10, 2020

achimnol Oct 17, 2020

belm0 Oct 10, 2020

njsmith Oct 10, 2020

achimnol Oct 17, 2020

belm0 Oct 10, 2020

achimnol Oct 17, 2020

belm0 Oct 10, 2020

achimnol Oct 17, 2020

1st1 commented Oct 12, 2020

aeros commented Oct 17, 2020

belm0 Oct 17, 2020

achimnol Oct 17, 2020

belm0 Oct 17, 2020

achimnol Oct 17, 2020

belm0 left a comment

corona10 left a comment

aeros left a comment •

edited

Loading

uburuntu commented Sep 29, 2021

		Add ``contextlib.aclosing`` analogous to ``contextlib.closing`` but for
		async generators and arbitrary objects with an ``aclose`` coroutine method.


		is equivalent to this:

		agen = await <module>.fetch(<arguments>)

bpo-41229: Update docs for explicit aclose()-required cases and add contextlib.aclosing() method #21545

bpo-41229: Update docs for explicit aclose()-required cases and add contextlib.aclosing() method #21545

Conversation

achimnol commented Jul 19, 2020 • edited by miss-islington Loading

achimnol commented Jul 19, 2020 • edited by bedevere-bot Loading

aeros commented Jul 19, 2020

achimnol commented Jul 20, 2020 • edited Loading

1st1 commented Jul 23, 2020

1st1 left a comment

Choose a reason for hiding this comment

corona10 left a comment • edited Loading

Choose a reason for hiding this comment

aeros left a comment • edited Loading

Choose a reason for hiding this comment

bedevere-bot commented Aug 18, 2020

achimnol commented Oct 1, 2020 • edited Loading

belm0 commented Oct 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 commented Oct 12, 2020

aeros commented Oct 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belm0 left a comment

Choose a reason for hiding this comment

corona10 left a comment

Choose a reason for hiding this comment

aeros left a comment • edited Loading

Choose a reason for hiding this comment

uburuntu commented Sep 29, 2021

achimnol commented Jul 19, 2020 •

edited by miss-islington

Loading

achimnol commented Jul 19, 2020 •

edited by bedevere-bot

Loading

achimnol commented Jul 20, 2020 •

edited

Loading

corona10 left a comment •

edited

Loading

aeros left a comment •

edited

Loading

achimnol commented Oct 1, 2020 •

edited

Loading

belm0 commented Oct 10, 2020 •

edited

Loading

aeros left a comment •

edited

Loading