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

(fix) Change context manager type to avoid race in threads #1368

Conversation

vlmaksimuk
Copy link
Contributor

Fix issue from issue #1132 when we get race condition in multithreading with generator based context manager. The generator is not thread safe!

…oid thread race at the cost of working with the generator
@vlmaksimuk vlmaksimuk force-pushed the fix/thread-safe-record-sql-queries-context-manager branch from b881e22 to a2375b0 Compare March 11, 2022 07:49
@sl0thentr0py
Copy link
Member

@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 _after_cursor_execute, so it seems the ctx_mgr variable on conn is being acted on by multiple threads, and so the conn itself is being used across threads.

From the Connection documentation

The Connection object is not thread-safe. While a Connection can be shared among threads using properly synchronized access, it is still possible that the underlying DBAPI connection may not support shared access between threads. Check the DBAPI documentation for details.

So I believe the actual fix for this would be to wrap the _sentry_sql_span_manager getter/setter in a lock?

@sl0thentr0py sl0thentr0py enabled auto-merge (squash) March 14, 2022 15:23
Copy link
Member

@sl0thentr0py sl0thentr0py left a 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.

@sl0thentr0py sl0thentr0py merged commit de0bc50 into getsentry:master Mar 14, 2022
sl0thentr0py added a commit that referenced this pull request Apr 8, 2022
… 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.
@sl0thentr0py
Copy link
Member

fyi @Fofanko I'm reverting this since it caused a regression and trying to fix the problem properly in #1388.

antonpirker pushed a commit that referenced this pull request Apr 11, 2022
…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
@Forty-Bot
Copy link

Can you add something like

record_sql_queries = RecordSqlQueries

to the module? This breaks users who were using the previous name.

@sl0thentr0py
Copy link
Member

@Forty-Bot this is reverted anyway in one of the newer releases.

@Forty-Bot
Copy link

Ah, looks like I was looking at an older branch.

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