Fixup logcontexts after replication PR.#19146
Conversation
| with PreserveLoggingContext(): | ||
| self._deferred.callback(None) |
There was a problem hiding this comment.
I spotted some of this while reviewing the original PR (see #19142) but went back and forth on whether this was actually a problem before calling it off.
For my own reference, can you explain how this is messing up the logcontexts please?
There was a problem hiding this comment.
Yeah, so I think what is happening here is some combination of a) not being careful of logcontexts when using raw deferreds (note that .callback(None) will run code waiting on the deferred), and b) I think the new(?) call_later logcontexts get confused when passing in a synchronous deferred.callback? I didn't quite hunt down exactly what went wrong, and it may be more of a problem with how it got used by BackgroundQueue that didn't affect current usages
There was a problem hiding this comment.
What I don't understand is "a) not being careful of logcontexts when using raw deferreds (note that .callback(None) will run code waiting on the deferred)"
I can see the whole deferred and interactions in the old version of DeferredEvent. Nothing is being added to the deferred callback chain and nothing is even changing logging contexts at all there.
There was a problem hiding this comment.
When those deferreds get called they were waking up the DeferredList , which then triggered the behaviour in make_deferred_yieldable to set the logcontexts to something. Then there is nothing to set those logcontexts back after the .callback(..) has finished?
There was a problem hiding this comment.
Ahh, perfect! I see. Thanks for pointing out the connection and explaining! 🙇
3 participants
Fixes logcontext leaks introduced in #19138.