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

Let the Nursery context-manage itself #340

Closed
wants to merge 3 commits into from

Conversation

smurfix
Copy link
Contributor

@smurfix smurfix commented Oct 19, 2017

I was curious whether the commented-out NurseryManager class would actually work, so I turned it on ;-)
I had to insert one conditional because of an impedance mismatch between context managing and cancel handling. All tests pass.

On second thought (second patch), a Nursery can be its own context manager. This can be improved further (third patch) by deferring the initialization to __aenter__(), noting that a Nursery is never used outside of a context manager.

This change speeds up the full test run by a bit more than 1% on my system (tested with 3.5 and 3.6).

@njsmith
Copy link
Member

njsmith commented Nov 28, 2017

I should note here: the nursery implementation is going to get majorly revisited in 0.3.0 (#136), so I've been putting this off to look at as part of that.

@njsmith
Copy link
Member

njsmith commented Dec 19, 2017

First pass of simplified nurseries is in #375.

Do you remember why you had to add the if isinstance(exc, Cancelled): return True bit to your Nursery.__aexit__ method? I don't see how it can be correct (doesn't it make nurseries swallow all Cancelled exceptions?), but I assume you ran into trouble without it...

@smurfix
Copy link
Contributor Author

smurfix commented Dec 20, 2017

I'll test my code with the current master branch and report back.

@njsmith
Copy link
Member

njsmith commented Dec 20, 2017

Thanks!

@kyrias
Copy link
Contributor

kyrias commented Feb 1, 2018

Any updates on this? :)

@smurfix
Copy link
Contributor Author

smurfix commented Feb 1, 2018

I'll have a look soon(ish).

@njsmith
Copy link
Member

njsmith commented Apr 22, 2018

We should probably stop using @asynccontextmanager in the nursery implementation (#229, #393). But this branch is way out of date, discussion has stalled, and I think it goes a bit to far (I like that the actual Nursery object is only returned from __aenter__, not from open_nursery). I'm going to close this for now, and we can always re-open it or steal bits from it later if we want to.

@njsmith njsmith closed this Apr 22, 2018
@belm0 belm0 mentioned this pull request Aug 18, 2018
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants