Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Implement cancellation support/protection for module callbacks#12469

Closed
squahtx wants to merge 3 commits intosquah/delay_cancellation_for_awaitablesfrom
squah/protect_module_callbacks_from_cancellation
Closed

Implement cancellation support/protection for module callbacks#12469
squahtx wants to merge 3 commits intosquah/delay_cancellation_for_awaitablesfrom
squah/protect_module_callbacks_from_cancellation

Conversation

@squahtx
Copy link
Contributor

@squahtx squahtx commented Apr 13, 2022

Update module callbacks with read semantics to protect them against
cancellation and avoid swallowing CancelledErrors.

Other module callbacks, such as the on_* callbacks, are presumed to
live on code paths that involve writes and aren't cancellation-friendly.
These module callbacks have been left alone.

Includes changes from #12468.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@squahtx squahtx requested a review from a team as a code owner April 13, 2022 18:56
@squahtx squahtx mentioned this pull request Apr 13, 2022
Closed
@squahtx squahtx force-pushed the squah/delay_cancellation_for_awaitables branch from 835105e to c2fb882 Compare April 13, 2022 19:03
@squahtx squahtx force-pushed the squah/protect_module_callbacks_from_cancellation branch from beab661 to 1a43a50 Compare April 13, 2022 19:03
Sean Quah added 3 commits April 13, 2022 20:08
Don't swallow CancelledErrors when running module callbacks
7efb569 
Update module callbacks with read semantics to not suppress
`CancelledError`s. Module callbacks with write semantics have been left
alone, since we don't expect cancellation to work correctly for those
code paths.

Signed-off-by: Sean Quah <seanq@element.io>
Protect module callbacks with read semantics against cancellation
ad69b81 
The `on_*` callbacks have been left alone, since they are presumed to
be run on code paths that aren't cancellation-friendly.

Signed-off-by: Sean Quah <seanq@element.io>
Add newsfile
84a47e3
@squahtx squahtx force-pushed the squah/delay_cancellation_for_awaitables branch from c2fb882 to 53bc43c Compare April 13, 2022 19:08
@squahtx squahtx force-pushed the squah/protect_module_callbacks_from_cancellation branch from 1a43a50 to 84a47e3 Compare April 13, 2022 19:09
@squahtx squahtx marked this pull request as draft April 14, 2022 09:14
@squahtx
Copy link
Contributor Author

squahtx commented Apr 14, 2022

Going to close this for now. A bunch of tests need updating first to not use the Mock().return_value = defer.succeed(...) pattern, since Deferreds aren't supposed to be awaitable more than once.

@squahtx squahtx closed this Apr 14, 2022
@squahtx squahtx mentioned this pull request Apr 19, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@squahtx