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

Simplified nurseries #375

Merged
merged 4 commits into from
Dec 23, 2017
Merged

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented Dec 19, 2017

  • Parenting is no longer a full time job
  • This commit also removes a bunch of deprecated functions

Fixes: gh-136

- Parenting is no longer a full time job
- This commit also removes a bunch of deprecated functions

Fixes: python-triogh-136
@codecov
Copy link

codecov bot commented Dec 19, 2017

Codecov Report

Merging #375 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
+ Coverage   99.24%   99.31%   +0.06%     
==========================================
  Files          87       87              
  Lines       10443    10334     -109     
  Branches      728      714      -14     
==========================================
- Hits        10364    10263     -101     
+ Misses         61       56       -5     
+ Partials       18       15       -3
Impacted Files Coverage Δ
trio/hazmat.py 100% <ø> (ø) ⬆️
trio/testing/_memory_streams.py 100% <ø> (+0.43%) ⬆️
trio/tests/test_ssl.py 100% <ø> (ø) ⬆️
trio/_toplevel_core_reexports.py 100% <ø> (ø) ⬆️
trio/_core/tests/test_run.py 100% <100%> (ø) ⬆️
trio/_core/_run.py 100% <100%> (ø) ⬆️
trio/tests/test_testing.py 100% <100%> (ø) ⬆️
trio/_socket.py 100% <0%> (ø) ⬆️
trio/tests/test_socket.py 100% <0%> (ø) ⬆️
trio/tests/test_util.py 100% <0%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7a14be...22b51de. Read the comment docs.

I don't think there's any way this could have led to a spurious
reschedule, but now I'm even more certain.
Copy link
Member

@touilleMan touilleMan left a comment

Choose a reason for hiding this comment

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

Played a bit with it, works pretty well !

to the ``racecar`` function to catch exceptions and handle them
however you like.
nursery.start_soon(jockey, async_fn)
winner = await q.get()
Copy link
Member

Choose a reason for hiding this comment

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

IIUC this should run after the loop, not inside it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! Fixed.

Thanks to Ran Benita.
@njsmith
Copy link
Member Author

njsmith commented Dec 21, 2017

Closing/reopening to restart Travis build affected by #379

@njsmith njsmith closed this Dec 21, 2017
@njsmith njsmith reopened this Dec 21, 2017
@bluetech
Copy link
Member

bluetech commented Dec 22, 2017

The rationale for the previous behavior (parenting is full-time task) was:

The difference between these is that in the first example, if walk crashes, the parent is off distracted chewing gum, and won't notice. In the second example, the parent is watching both children, and will notice and respond appropriately if anything happens.

This made sense to me.

Now if we look at this example with the new approach:

async def parent(handler):
    try:
        async with trio.open_nursery() as nursery:
            nursery.start_soon(some_task)
            await trio.sleep(inf)
    except Exception:
        pass
    print('This will be reached')

If some_task raises, what happens? IIUC, the content of the async with is now a "pseudo-task", and it will get cancelled, but then the flow continues like normal after the async with.

If I want to perform cancellation cleanup for the "pseudo-task", will the following work?

async def parent(handler):
    async def some_task(cancel_scope):
        await trio.sleep(1)
        cancel_scope.cancel()

    async with trio.open_nursery() as nursery:
        nursery.start_soon(some_task, nursery.cancel_scope)
        try:
            await trio.sleep(inf)
        except trio.Cancelled:
            # some cleanup...
            raise
    print('This will be reached')

@bluetech
Copy link
Member

(I've made some edits to the question above, if you're reading from mail).

@njsmith
Copy link
Member Author

njsmith commented Dec 22, 2017

If I want to perform cancellation cleanup for the "pseudo-task", will the following work?

Yeah, that looks correct. Of course in many cases a finally block might be simpler :-). In particular, one thing to watch out for: if the code inside being cancelled was more complicated than sleep(inf) and happened to use nurseries internally (something you normally wouldn't need to care about), then when it's cancelled you might get a MultiError containing multiple Cancelled, and except Cancelled: won't catch a MultiError, even though semantically they'd be the same in this case.

I'm not sure if there's something better we can be doing here. MultiError is the place where we're really fighting the language, and everything has some trade-offs. For example, we could collapse together equivalent Cancelled exceptions, but then you'd lose proper tracebacks for them.

@njsmith
Copy link
Member Author

njsmith commented Dec 23, 2017

Guess this has marinated long enough :-)

@njsmith njsmith merged commit 0c667d4 into python-trio:master Dec 23, 2017
@njsmith njsmith deleted the simplified-nurseries branch December 23, 2017 00:00
@mentalisttraceur
Copy link

Is there some essay on what "parenting is a full-time task" and "parenting is not a full-time task" mean, including rationale for each approach?

I haven't recursed through the linked issues or looked at the code changes themselves, but from just this issue discussion and my beginner-level familiarity with using trio, I have no idea what exactly is being expressed here (besides the partial but incomplete narrowing down of possible meanings that I can infer from the above example).

@mentalisttraceur
Copy link

Oh okay the diff contains documentation changes that seem to clarify it a bit. You can just ignore my last comment.

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.

Potential API changes to help users who don't realize parenting is a full-time job
4 participants