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

[middleware] Don't try to access self.state.span in handle_error of Flask DB middleware if there is no current_app #81

Merged
merged 1 commit into from
Sep 10, 2019

Conversation

benhiller
Copy link
Contributor

Both before_cursor_execute and after_cursor_execute exit early
if there is no current_app, but handle_error tries to finish
the span even if there is no current_app. This caused an
AttributeError when I was using beeline-python in the context of a
multi-threaded application, since when we used HoneyDBMiddleware off
of the thread it had been initialized on, self.state.span was never
set. We could probably also fix this by using getattr on self.state
before accessing span, but this seems more consistent with how the
other methods operate.

…lask DB middleware if there is no current_app

Summary: Both `before_cursor_execute` and `after_cursor_execute` are
no-ops if there is no `current_app`, but `handle_error` tries to finish
the span even if there is no `current_app`. This caused an
`AttributeError` when I was using beeline-python in the context of a
multi-threaded application, since when we used `HoneyDBMiddleware` off
of the thread it had been initialized on, `self.state.span` was never
set. We could probably also fix this by using `getattr` on `self.state`
before accessing `span`, but this seems more consistent with how the
other methods operate.
@tredman tredman merged commit ec05745 into honeycombio:master Sep 10, 2019
@benhiller benhiller deleted the handle_error_fix branch September 10, 2019 16:57
@tredman
Copy link
Contributor

tredman commented Sep 10, 2019

Thanks for the PR! 2.9.1 should be live within the next hour with your fix.

@benhiller
Copy link
Contributor Author

Awesome, thanks for merging it!

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.

2 participants