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

WIP: Add graceful cancel to cancel scopes #941

Closed

Conversation

nmalaguti
Copy link
Contributor

See https://trio.discourse.group/t/graceful-shutdown/93/19?u=nmalaguti for the intended behavior.

No tests yet, just some code to illustrate intended behavior.

@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #941 into master will decrease coverage by 0.35%.
The diff coverage is 51.66%.

@@            Coverage Diff             @@
##           master     #941      +/-   ##
==========================================
- Coverage   99.39%   99.03%   -0.36%     
==========================================
  Files         102      102              
  Lines       12461    12249     -212     
  Branches      915      918       +3     
==========================================
- Hits        12385    12131     -254     
- Misses         56       85      +29     
- Partials       20       33      +13
Impacted Files Coverage Δ
trio/_core/_run.py 95.8% <51.66%> (-3.92%) ⬇️
trio/_subprocess_platform/waitid.py 60.52% <0%> (-39.48%) ⬇️
trio/socket.py 92.85% <0%> (-7.15%) ⬇️
trio/_util.py 94.73% <0%> (-4.23%) ⬇️
trio/_core/_multierror.py 95.59% <0%> (-3.79%) ⬇️
trio/tests/test_file_io.py 98.21% <0%> (-1.79%) ⬇️
trio/_socket.py 98.31% <0%> (-0.88%) ⬇️
trio/testing/_trio_test.py 87.5% <0%> (-0.74%) ⬇️
trio/_highlevel_open_unix_stream.py 83.33% <0%> (-0.67%) ⬇️
trio/_core/_ki.py 98.36% <0%> (-0.13%) ⬇️
... and 48 more

@@ -366,8 +428,7 @@ def _exc_filter(self, exc):
def _close(self, exc):
scope_task = current_task()
if (
exc is not None and self.cancel_called
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like self.cancel_called is in this if statement to ensure that raising Cancelled won't be caught here unless scope.cancel() was called. Looking at the logic in _pending_cancel_scope it looks like you can't return a scope unless that is already true, so this check is redundant.

I have removed it because there are now cases where you can be cancelled gracefully without having been explicitly cancelled.

@nmalaguti nmalaguti force-pushed the feature/graceful-cancel branch 2 times, most recently from 1889f00 to 9128593 Compare February 19, 2019 22:12
@nmalaguti nmalaguti force-pushed the feature/graceful-cancel branch from 9128593 to 7ca63d5 Compare February 20, 2019 03:44
@nmalaguti
Copy link
Contributor Author

@oremanj I was surprised at how simple this was to implement, and it turns out it's because you've been doing a ton of work in the cancellation space.

Seeing how this relates to #886, #885, #910, and #147, I feel like I didn't do enough research before embarking on this change.

Also I just noticed you have an alternate approach in #921. I'd love to discuss further the differences between them.

that are outside this scope that have been gracefully canceled.
If this is set to :data:`None` then this scope will inherit its
graceful cancel behavior from its parent scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

This wording is hard to understand and should be clarified.

@nmalaguti nmalaguti closed this Dec 7, 2021
@nmalaguti nmalaguti deleted the feature/graceful-cancel branch December 7, 2021 14:06
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.

2 participants