Skip to content

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

Merged
merged 36 commits into from
Apr 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
7d14008
Some test showing trx in trx is a bad idea
antonpirker Apr 11, 2025
9c35e83
test that shows we are leaking scope changes
antonpirker Apr 11, 2025
664f0e0
remove flaky tests, it does more harm then it helps
antonpirker Apr 11, 2025
0d9a221
better readability
antonpirker Apr 11, 2025
0100bdb
better readable
antonpirker Apr 11, 2025
bd44959
Propagate a copy of the scopes to threads
antonpirker Apr 11, 2025
0e92608
More tests
antonpirker Apr 11, 2025
fd69f64
better span names
antonpirker Apr 11, 2025
e4d94a5
what is happening?
antonpirker Apr 11, 2025
c7af4b9
back to forking
antonpirker Apr 11, 2025
678f515
trigger ci
antonpirker Apr 14, 2025
eef7ff6
trying without new tests
antonpirker Apr 14, 2025
518dc04
Revert all the changes
antonpirker Apr 14, 2025
24ea433
Trying somethign
antonpirker Apr 14, 2025
0d5b3fb
Reenable tests
antonpirker Apr 14, 2025
d3585a3
cleanup
antonpirker Apr 14, 2025
5bcba35
trying test forking
antonpirker Apr 14, 2025
4c2f7d9
Removed some stuff again
antonpirker Apr 14, 2025
890640f
commented wrong lines
antonpirker Apr 14, 2025
957d7bb
does it work in django 3.0?
antonpirker Apr 14, 2025
07d44c9
no channels in django 3.0
antonpirker Apr 14, 2025
2a2cde1
trying without channels
antonpirker Apr 14, 2025
8cd789b
No channels for newer Django versions
antonpirker Apr 14, 2025
6982479
Warning
antonpirker Apr 14, 2025
3b85ef4
Better error message
antonpirker Apr 15, 2025
e783af9
.
antonpirker Apr 15, 2025
160e430
checking for warning
antonpirker Apr 15, 2025
8cd1584
Merge branch 'master' into antonpirker/threading-tests
antonpirker Apr 15, 2025
9aad099
linting
antonpirker Apr 15, 2025
0da1c2e
explanation
antonpirker Apr 15, 2025
096579e
Revert tox to version in master
antonpirker Apr 15, 2025
33d9f3d
Merge branch 'master' into antonpirker/threading-tests
antonpirker Apr 15, 2025
815e930
Better checks and importing only once
antonpirker Apr 15, 2025
5d306ee
Merge branch 'antonpirker/threading-tests' of github.com:getsentry/se…
antonpirker Apr 15, 2025
8aa5edf
Better check
antonpirker Apr 15, 2025
ca8aa30
Merge branch 'master' into antonpirker/threading-tests
antonpirker Apr 15, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions sentry_sdk/integrations/threading.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import sys
import warnings
from functools import wraps
from threading import Thread, current_thread

Expand Down Expand Up @@ -49,6 +50,15 @@ def setup_once():
# type: () -> None
old_start = Thread.start

try:
from django import VERSION as django_version # noqa: N811
import channels # type: ignore[import-not-found]

channels_version = channels.__version__
except ImportError:
django_version = None
channels_version = None

@wraps(old_start)
def sentry_start(self, *a, **kw):
# type: (Thread, *Any, **Any) -> Any
Expand All @@ -57,8 +67,27 @@ def sentry_start(self, *a, **kw):
return old_start(self, *a, **kw)

if integration.propagate_scope:
isolation_scope = sentry_sdk.get_isolation_scope()
current_scope = sentry_sdk.get_current_scope()
if (
sys.version_info < (3, 9)
and channels_version is not None
and channels_version < "4.0.0"
and django_version is not None
and django_version >= (3, 0)
and django_version < (4, 0)
):
warnings.warn(
Copy link
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.

Copy link
Member 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.

Copy link
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

Copy link
Member 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.

"There is a known issue with Django channels 2.x and 3.x when using Python 3.8 or older. "
"(Async support is emulated using threads and some Sentry data may be leaked between those threads.) "
"Please either upgrade to Django channels 4.0+, use Django's async features "
"available in Django 3.1+ instead of Django channels, or upgrade to Python 3.9+.",
stacklevel=2,
)
isolation_scope = sentry_sdk.get_isolation_scope()
current_scope = sentry_sdk.get_current_scope()

else:
isolation_scope = sentry_sdk.get_isolation_scope().fork()
current_scope = sentry_sdk.get_current_scope().fork()
else:
isolation_scope = None
current_scope = None
Expand Down
22 changes: 19 additions & 3 deletions tests/integrations/django/asgi/test_asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,25 @@ async def test_basic(sentry_init, capture_events, application):

events = capture_events()

comm = HttpCommunicator(application, "GET", "/view-exc?test=query")
response = await comm.get_response()
await comm.wait()
import channels # type: ignore[import-not-found]

if (
sys.version_info < (3, 9)
and channels.__version__ < "4.0.0"
and django.VERSION >= (3, 0)
and django.VERSION < (4, 0)
):
# We emit a UserWarning for channels 2.x and 3.x on Python 3.8 and older
# because the async support was not really good back then and there is a known issue.
# See the TreadingIntegration for details.
with pytest.warns(UserWarning):
comm = HttpCommunicator(application, "GET", "/view-exc?test=query")
response = await comm.get_response()
await comm.wait()
else:
comm = HttpCommunicator(application, "GET", "/view-exc?test=query")
response = await comm.get_response()
await comm.wait()

assert response["status"] == 500

Expand Down
101 changes: 101 additions & 0 deletions tests/integrations/threading/test_threading.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import gc
from concurrent import futures
from textwrap import dedent
from threading import Thread

import pytest
Expand Down Expand Up @@ -172,3 +173,103 @@ def target():
assert Thread.run.__qualname__ == original_run.__qualname__
assert t.run.__name__ == "run"
assert t.run.__qualname__ == original_run.__qualname__


@pytest.mark.parametrize(
"propagate_scope",
(True, False),
ids=["propagate_scope=True", "propagate_scope=False"],
)
def test_scope_data_not_leaked_in_threads(sentry_init, propagate_scope):
sentry_init(
integrations=[ThreadingIntegration(propagate_scope=propagate_scope)],
)

sentry_sdk.set_tag("initial_tag", "initial_value")
initial_iso_scope = sentry_sdk.get_isolation_scope()

def do_some_work():
# check if we have the initial scope data propagated into the thread
if propagate_scope:
assert sentry_sdk.get_isolation_scope()._tags == {
"initial_tag": "initial_value"
}
else:
assert sentry_sdk.get_isolation_scope()._tags == {}

# change data in isolation scope in thread
sentry_sdk.set_tag("thread_tag", "thread_value")

t = Thread(target=do_some_work)
t.start()
t.join()

# check if the initial scope data is not modified by the started thread
assert initial_iso_scope._tags == {
"initial_tag": "initial_value"
}, "The isolation scope in the main thread should not be modified by the started thread."


@pytest.mark.parametrize(
"propagate_scope",
(True, False),
ids=["propagate_scope=True", "propagate_scope=False"],
)
def test_spans_from_multiple_threads(
sentry_init, capture_events, render_span_tree, propagate_scope
):
sentry_init(
traces_sample_rate=1.0,
integrations=[ThreadingIntegration(propagate_scope=propagate_scope)],
)
events = capture_events()

def do_some_work(number):
with sentry_sdk.start_span(
op=f"inner-run-{number}", name=f"Thread: child-{number}"
):
pass

threads = []

with sentry_sdk.start_transaction(op="outer-trx"):
for number in range(5):
with sentry_sdk.start_span(
op=f"outer-submit-{number}", name="Thread: main"
):
t = Thread(target=do_some_work, args=(number,))
t.start()
threads.append(t)

for t in threads:
t.join()

(event,) = events
if propagate_scope:
assert render_span_tree(event) == dedent(
"""\
- op="outer-trx": description=null
- op="outer-submit-0": description="Thread: main"
- op="inner-run-0": description="Thread: child-0"
- op="outer-submit-1": description="Thread: main"
- op="inner-run-1": description="Thread: child-1"
- op="outer-submit-2": description="Thread: main"
- op="inner-run-2": description="Thread: child-2"
- op="outer-submit-3": description="Thread: main"
- op="inner-run-3": description="Thread: child-3"
- op="outer-submit-4": description="Thread: main"
- op="inner-run-4": description="Thread: child-4"\
"""
)

elif not propagate_scope:
assert render_span_tree(event) == dedent(
"""\
- op="outer-trx": description=null
- op="outer-submit-0": description="Thread: main"
- op="outer-submit-1": description="Thread: main"
- op="outer-submit-2": description="Thread: main"
- op="outer-submit-3": description="Thread: main"
- op="outer-submit-4": description="Thread: main"\
"""
)
Loading