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

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 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
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>
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>
@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant