-
Notifications
You must be signed in to change notification settings - Fork 401
Remove logcontext problems caused by awaiting raw deferLater(...)
#19058
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
base: develop
Are you sure you want to change the base?
Remove logcontext problems caused by awaiting raw deferLater(...)
#19058
Conversation
``` builtins.AssertionError: Expected `looping_call` callback from the reactor to start with the sentinel logcontext but saw task-_resumable_task-0-IBzAmHUoepQfLnEA. In other words, another task shouldn't have leaked their logcontext to us. ```
# Create a deferred which we will never complete | ||
incomplete_d: Deferred = Deferred() | ||
# Await forever to simulate an aborted task because of a restart | ||
await deferLater(self.reactor, 2**16, lambda: None) | ||
await make_deferred_yieldable(incomplete_d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the goal is to wait forever, let's actually wait forever instead of 2**16
seconds.
self, task: ScheduledTask | ||
) -> Tuple[TaskStatus, Optional[JsonMapping], Optional[str]]: | ||
# Sleep for a second | ||
await deferLater(self.reactor, 1, lambda: None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have also fixed this by wrapping the deferred with make_deferred_yieldable(...)
. (see our logcontext docs
ex. await make_deferred_yieldable(deferLater(self.reactor, 1, lambda: None))
Instead, I've opted to replace all of our deferLater
usage with more proper clock utilities that handle logcontext rules already.
deferLater
deferLater(...)
Remove logcontext problems caused by raw
deferLater
. This is a normal problem where weawait
a deferred without wrapping it inmake_deferred_yieldable(...)
. But I've opted to replace the usage ofdeferLater
with something more standard for the Synapse codebase.Part of #18905
It's unclear why we're only now seeing these failures happen with the changes from #19057
Example failures seen in https://github.com/element-hq/synapse/actions/runs/18477454390/job/52645183606?pr=19057
Dev notes
Our logcontext docs:
docs/log_contexts.md
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.