-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add more log context checks when patching inlineCallbacks #6127
Conversation
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.
looks generally sane, but a few thoughts
tests/patch_inline_callbacks.py
Outdated
frame.f_code.co_filename, | ||
) | ||
) | ||
changes.append(err) |
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.
why do we put these in an array, rather than printing them out immediately?
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.
I've added some comments, but broadly functions do change contexts fairly frequently, e.g. when using Measure
or manually creating log contexts, so we only really want to log this info if it later turns out something is going wrong.
In future we might be able to do something that lets us turn this into a hard error by somehow understanding when its "ok" that the log context has changed.
tests/patch_inline_callbacks.py
Outdated
else: | ||
d = gen.send(result) | ||
except (StopIteration, defer._DefGen_Return) as e: | ||
if LoggingContext.current_context() != expected_context: |
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.
isn't this case handled by check_ctx
above?
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.
(if we're going to double-check the normal-termination case, we should probably also double-check the raised-an-exception case, and tbh it looks like a bunch of cod e that we can get rid of)
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.
Hmm, yeah, I mainly found it useful for the line numbers
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.
lgtm
Co-Authored-By: Richard van der Hoff <[email protected]>
This does two things:
Adds a function that tracks changes in log contexts over yield points. This helps track down roughly where things are going wrong.
Adds an env var to enable patching, so that we can enable it for things like sytest as well as unit tests.