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

Clean up handling of Handles #76

Merged
merged 11 commits into from
May 11, 2020
Merged

Conversation

oremanj
Copy link
Member

@oremanj oremanj commented May 3, 2020

  • Use regular asyncio.Handle and asyncio.TimerHandle in most cases, reserving our custom extensions for the few places that actually need the extra features
  • Name our extended handles in a way that clarifies what features they provide: ScopedHandle and AsyncHandle (also, two types rather than one type with a flag that changes its behavior and valid methods)
  • Remove the logic for cancelling a Task when a handle for its next step is cancelled; this was never invoked in the test suite and it's not clear to me what it's intended to help with. (@smurfix can you provide any historical perspective on the rationale for this logic? It was added in 21f4096 "Fix hang-on-close tests")
  • Move the logic for propagating the result of an async handle to a Future into the handle code (allows us to remove the "is_sync=None" special case for invoking the callback with its handle as argument) and robustify it; it should correctly handle MultiErrors now
  • Run async handles' callbacks (trio_as_aio tasks) in a copy of the contextvars context that was current when creating them. The previous code appeared to do this, but didn't actually.

@oremanj oremanj requested review from smurfix and njsmith May 3, 2020 07:54
trio_asyncio/_base.py Show resolved Hide resolved
trio_asyncio/_handles.py Show resolved Hide resolved
trio_asyncio/_handles.py Outdated Show resolved Hide resolved
@oremanj oremanj closed this May 6, 2020
@oremanj oremanj reopened this May 6, 2020
@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #76 into master will increase coverage by 1.84%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   78.05%   79.90%   +1.84%     
==========================================
  Files          11       11              
  Lines        1285     1229      -56     
  Branches      180      171       -9     
==========================================
- Hits         1003      982      -21     
+ Misses        204      173      -31     
+ Partials       78       74       -4     
Impacted Files Coverage Δ
trio_asyncio/_base.py 83.96% <94.28%> (+2.14%) ⬆️
trio_asyncio/_async.py 79.41% <100.00%> (+5.88%) ⬆️
trio_asyncio/_handles.py 94.93% <100.00%> (+17.57%) ⬆️
trio_asyncio/_sync.py 70.99% <100.00%> (-0.22%) ⬇️

@oremanj oremanj requested a review from smurfix May 6, 2020 09:57
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

@pquentin pquentin merged commit 2440c92 into python-trio:master May 11, 2020
This was referenced Jan 1, 2021
@oremanj oremanj deleted the cleanup-handles branch November 6, 2023 22:35
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