-
Notifications
You must be signed in to change notification settings - Fork 517
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
(fix) Change context manager type to avoid race in threads #1368
(fix) Change context manager type to avoid race in threads #1368
Conversation
…oid thread race at the cost of working with the generator
b881e22
to
a2375b0
Compare
@Fofanko if this really fixes the problem, happy to merge it in, but I'm trying to fully understand where the threading problem actually lies. In the original stacktrace on the linked issue, the error occurs in From the
So I believe the actual fix for this would be to wrap the |
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.
thx @Fofanko for the PR!
I'm still not convinced this is the actual problem, but it still does get rid of generator magic so I'm merging it in.
fyi @Fofanko I'm reverting this since it caused a regression and trying to fix the problem properly in #1388. |
…gration (#1388) * Revert "fix(sqlalchemy): Change context manager type to avoid race in threads (#1368)" This reverts commit de0bc50. This caused a regression (#1385) since the span finishes immediately in __enter__ and so all db spans have wrong time durations. * Use context instead of conn in sqlalchemy hooks
Can you add something like record_sql_queries = RecordSqlQueries to the module? This breaks users who were using the previous name. |
@Forty-Bot this is reverted anyway in one of the newer releases. |
Ah, looks like I was looking at an older branch. |
Fix issue from issue #1132 when we get race condition in multithreading with generator based context manager. The generator is not thread safe!