Skip to content

fix: Data leak in ThreadingIntegration between threads#4281

Merged
antonpirker merged 36 commits into
masterfrom
antonpirker/threading-tests
Apr 15, 2025
Merged

fix: Data leak in ThreadingIntegration between threads#4281
antonpirker merged 36 commits into
masterfrom
antonpirker/threading-tests

Conversation

@antonpirker

@antonpirker antonpirker commented Apr 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@codecov

codecov Bot commented Apr 11, 2025 •
edited
Loading

Copy link
Copy Markdown

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
22033 2 22031 5561
View the top 2 failed test(s) by shortest run time 
tests.integrations.django.asgi.test_asgi test_basic[application1]
Stack Traces | 0.538s run time 
.../django/asgi/test_asgi.py:48: in test_basic
    await comm.wait()
E   Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E    Emitted warnings: [ DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.')].
tests.integrations.django.asgi.test_asgi test_basic[application0]
Stack Traces | 0.583s run time 
.../django/asgi/test_asgi.py:48: in test_basic
    await comm.wait()
E   Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E    Emitted warnings: [ DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.'),
E     DeprecationWarning('The `same_process_as_parent` parameter is deprecated.')].

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@antonpirker antonpirker changed the title More threading tests fix: data leak in threads Apr 11, 2025
@antonpirker antonpirker changed the title fix: data leak in threads fix: Data leak in ThreadingIntegration between threads Apr 11, 2025
@antonpirker
antonpirker marked this pull request as ready for review April 11, 2025 07:54
@antonpirker
antonpirker requested a review from a team as a code owner April 11, 2025 07:54
szokeasaurusrex
szokeasaurusrex approved these changes Apr 11, 2025
View reviewed changes

@szokeasaurusrex szokeasaurusrex left a comment

Copy link
Copy Markdown
Member

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)

sentrivana
sentrivana approved these changes Apr 15, 2025
View reviewed changes

@sentrivana sentrivana left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! Please check comments

Comment thread sentry_sdk/integrations/threading.py
and django_version >= (3, 0)
and django_version < (4, 0)
):
warnings.warn(

@sentrivana sentrivana Apr 15, 2025

Copy link
Copy Markdown
Contributor

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.

@antonpirker antonpirker Apr 15, 2025

Copy link
Copy Markdown
Contributor Author

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.

@sentrivana sentrivana Apr 15, 2025

Copy link
Copy Markdown
Contributor

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

@antonpirker antonpirker Apr 15, 2025

Copy link
Copy Markdown
Contributor Author

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.

Comment thread sentry_sdk/integrations/threading.py Outdated
@antonpirker
antonpirker merged commit 2d392af into master Apr 15, 2025
@antonpirker
antonpirker deleted the antonpirker/threading-tests branch April 15, 2025 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sentrivana sentrivana sentrivana approved these changes
@szokeasaurusrex szokeasaurusrex szokeasaurusrex approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@antonpirker @szokeasaurusrex @sentrivana