Nursery: don't act as a checkpoint when not running anything

[catern]

trafficstars

Previously, a Nursery would checkpoint on exit, regardless of whether it was running anything. This is a bit annoying, see #1457, so let's change it to not checkpoint on exit. Now it won't be a checkpoint at all if there's no tasks running inside.

However, this doesn't fully solve #1457, because we'll still inject a cancel even if none of the child tasks raise Cancelled.

This seems to break trio/_core/tests/test_asyncgen.py::test_last_minute_gc_edge_case such that it no longer ever hits the edge case.

That seems like a pretty complicated test, so maybe @oremanj can say better why exactly it's breaking with this change.

[njsmith]

However, this doesn't fully solve #1457, because we'll still inject a cancel even if none of the child tasks raise Cancelled.

Here's the code that does that, at line 923:

            def aborted(raise_cancel):
                self._add_exc(capture(raise_cancel).error)

...I was going to say we could delete this line and simply ignore cancellation here, but that's not quite right. We do want to ignore Cancelled exceptions here, but if Trio is using cancellation to deliver a KeyboardInterrupt to the main task while it's blocked in __aexit__, then we still need that to be injected so that the nursery will cancel the other tasks and then raise KeyboardInterrupt. That should be fixed at some point Soon™, but in the mean time I guess we could work around this by adding a check for whether capture(raise_cancel).error is a Cancelled, and if so then ignore it.

[oremanj]

I left a comment on #1457: I think we shouldn't drop the schedule point in either branch, only the cancellation point; and we shouldn't drop the cancellation point if the nursery didn't run any tasks, so that async with open_nursery(): pass still executes a full checkpoint.

The test is failing because it relies on nursery exit being a schedule point.

[catern]

Also, what do you think about making assert_yields_or_not public? I just needed it for the test that a nursery is a schedule point but not a cancel point. Maybe that's something we should discourage?

[codecov[bot]]

Codecov Report

Merging #1696 (06c176c) into master (3288cfc) will increase coverage by 0.10%. The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1696      +/-   ##
==========================================
+ Coverage   99.03%   99.14%   +0.10%     
==========================================
  Files         115      115              
  Lines       17433    17456      +23     
  Branches     3107     3110       +3     
==========================================
+ Hits        17265    17306      +41     
+ Misses        122      104      -18     
  Partials       46       46              

Files	Coverage Δ
trio/_core/_run.py	100.00% <100.00%> (ø)
trio/_core/_tests/test_run.py	100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

[njsmith]

Also, what do you think about making assert_yields_or_not public? I just needed it for the test that a nursery is a schedule point but not a cancel point. Maybe that's something we should discourage?

I actually just ran into this issue again while working on #1805 on stream, so I did some tweaks to land this quicker. And one of those tweaks was to revert the assert_yields_or_not changes, because they seemed overly complex for a single weird internal test :-). So I guess that answers the question...

[catern]

The changes look fine to me. Excited for this to be fixed!

[richardsheridan]

The merge was not as painful as one might expect. The two new commits are nearly trivial fixes responsive to recent updates to the test suite.

[trio-bot[bot]]

Hey @catern, it looks like that was the first time we merged one of your PRs! Thanks so much! :tada: :birthday:

If you want to keep contributing, we'd love to have you. So, I just sent you an invitation to join the python-trio organization on Github! If you accept, then here's what will happen:

• Github will automatically subscribe you to notifications on all our repositories. (But you can unsubscribe again if you don't want the spam.)

• You'll be able to help us manage issues (add labels, close them, etc.)

• You'll be able to review and merge other people's pull requests

• You'll get a [member] badge next to your name when participating in the Trio repos, and you'll have the option of adding your name to our member's page and putting our icon on your Github profile (details)

If you want to read more, here's the relevant section in our contributing guide.

Alternatively, you're free to decline or ignore the invitation. You'll still be able to contribute as much or as little as you like, and I won't hassle you about joining again. But if you ever change your mind, just let us know and we'll send another invitation. We'd love to have you, but more importantly we want you to do whatever's best for you.

If you have any questions, well... I am just a humble Python script, so I probably can't help. But please do post a comment here, or in our chat, or on our forum, whatever's easiest, and someone will help you out!