Skip to content
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

Format frame locals when captured #1744

Merged
merged 1 commit into from Mar 11, 2023

Conversation

living180
Copy link
Contributor

Before this commit, if ENABLE_STACKTRACES_LOCALS was True, each stack frame's locals dict would be stored directly in the captured stack trace, and only converted to a string once the stack trace was rendered. This means that for panels, such as the SQL Panel, which do not render the stack traces until later after they have all been captured, it is quite possible that the values in the locals dicts at the point when the stack trace was rendered would no longer be the same as they were at the time when the stack trace was captured, resulting in incorrect values being displayed to the user.

Fix by converting the locals dict to a string immediately when it is captured (in the get_stack_trace() function as well as in the deprecated tidy_stacktrace() function) instead of in the render_stacktrace() function.

I realize that this represents an internal API change, as now the last item of the tuples returned by get_stack_trace() will now be a string rather than a dict. However, this is not actually a documented public API and I do not expect any third-party panels to be trying to inspect or use the locals dicts from the stack traces directly, even if they are using get_stack_trace() (or more likely, get_stack() and tidy_stacktrace()). But if there are concerns about this, I have some thoughts about possible ways to address it.

Before this commit, if ENABLE_STACKTRACES_LOCALS was True, each stack
frame's locals dict would be stored directly in the captured stack
trace, and only converted to a string once the stack trace was rendered.
This means that for panels, such as the SQL Panel, which do not render
the stack traces until later after they have all been captured, it is
quite possible that the values in the locals dicts at the point when the
stack trace was rendered would no longer be the same as they were at the
time when the stack trace was captured, resulting in incorrect values
being displayed to the user.

Fix by converting the locals dict to a string immediately when it is
captured (in the get_stack_trace() function as well as in the deprecated
tidy_stacktrace() function) instead of in the render_stacktrace()
function.
@tim-schilling
Copy link
Contributor

The concern here is would this recording evaluate anything in the stack, such as a generator or callable that would then prevent the application (not the toolbar) from operating as expected?

@living180
Copy link
Contributor Author

I don't expect so. The new code is using the exact same mechanism to convert to a string (pprint.pformat()) as the old code, it just shifts when it runs. And .pformat() uses repr() internally, so I don't think it should cause any problems with generators or callables. Of course, with Python's full dynamism, it is certainly possible to create a local variable that does Bad Things™ when its repr is generated, but I think that would have to be deliberate and not something that would come up with normal code.

@living180
Copy link
Contributor Author

Also, the Cache panel actually does exactly this right now, because it calls render_stacktrace() which internally calls pprint.pformat() when recording each call. So if there were any problems with this, they would have already been hit in the Cache panel.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

@tim-schilling tim-schilling merged commit ccdf705 into jazzband:main Mar 11, 2023
will be converted to strings when the stack trace is captured rather when it
is rendered, so that the correct values will be displayed in the rendered
stack trace, as they may have changed between the time the stack trace was
captured and when it is rendered.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the well written changelog @living180!

@living180 living180 deleted the stack_frame_locals branch April 15, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants