-
Notifications
You must be signed in to change notification settings - Fork 535
fix: Data leak in ThreadingIntegration between threads #4281
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #4281 +/- ##
==========================================
+ Coverage 79.52% 79.55% +0.03%
==========================================
Files 142 142
Lines 15905 15917 +12
Branches 2721 2722 +1
==========================================
+ Hits 12648 12663 +15
Misses 2390 2390
+ Partials 867 864 -3
|
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, but check with @sl0thentr0py first (I was unclear about the outcome from the slack thread discussion)
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.
Nice! Please check comments
and django_version >= (3, 0) | ||
and django_version < (4, 0) | ||
): | ||
warnings.warn( |
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.
Since this is happening in Thread.start
, we might log this even if a thread is started outside of Django -- but I don't think we have a way of detecting that. And since this is warnings.warn
message, it should only be printed once and not spam on every Thread.start
, so hopefully this is fine.
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.
the problem with the unawaited future is only happening if old python, django and channels are used in combination. on a vanilla python project on old python versions the threading works fine.
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.
Yeah, my point was more that someone might have Django/channels installed in their venv but uses threads in an unrelated script that has nothing to do with their Django app and they would get this warning too. But as said, I don't see a way around this
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.
now I get it. yes that is true. but as you said, the warning is only emitted once, so I think it is ok.
…ntry-python into antonpirker/threading-tests
It is possible to leak data from started threads into the main thread via the scopes. (Because the same scope object from the main thread could be changed in the started thread.)
This change always makes a fork (copy) of the scopes of the main thread before it propagates those scopes into the started thread.