Skip to content

Remove explicit __del__'s in threaded classes #4590

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 1 commit into from
Jul 17, 2025

Conversation

sl0thentr0py
Copy link
Member

@sl0thentr0py sl0thentr0py commented Jul 17, 2025

The changes in #4577 introduced a bit of flakiness on pre-3.10 due to a weird interaction of capsys, stderr logging and our object lifecycles.

In this PR, I'm removing all the explicit __del__ kills since we call them all explicitly in client.close anyway and that's already cleaner. Having logic in __del__ causes non-deterministic GC behavior, especially with threaded code.

Stacktrace is linked in a comment below where you can see the transport.__del__ method is causing the weird reentrant logging bug to stderr.

@sl0thentr0py sl0thentr0py requested a review from a team as a code owner July 17, 2025 13:18
@sl0thentr0py
Copy link
Member Author

Traceback (most recent call last):
  File "/Users/neel/.asdf/installs/python/3.8.19/lib/python3.8/logging/__init__.py", line 1088, in emit
    stream.write(msg + self.terminator)
RuntimeError: reentrant call inside <_io.BufferedWriter name=\'<stderr>\'>
Call stack:
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/bin/pytest", line 8, in <module>
    sys.exit(console_main())
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/config/__init__.py", line 201, in console_main
    code = main()
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/config/__init__.py", line 175, in main
    ret: ExitCode | int = config.hook.pytest_cmdline_main(config=config)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_hooks.py", line 513, in __call__
    return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_manager.py", line 120, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_callers.py", line 103, in _multicall
    res = hook_impl.function(*args)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/main.py", line 330, in pytest_cmdline_main
    return wrap_session(config, _main)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/main.py", line 283, in wrap_session
    session.exitstatus = doit(config, session) or 0
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/main.py", line 337, in _main
    config.hook.pytest_runtestloop(session=session)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_hooks.py", line 513, in __call__
    return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_manager.py", line 120, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_callers.py", line 103, in _multicall
    res = hook_impl.function(*args)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/main.py", line 362, in pytest_runtestloop
    item.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_hooks.py", line 513, in __call__
    return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_manager.py", line 120, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_callers.py", line 103, in _multicall
    res = hook_impl.function(*args)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/runner.py", line 113, in pytest_runtest_protocol
    runtestprotocol(item, nextitem=nextitem)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/runner.py", line 132, in runtestprotocol
    reports.append(call_and_report(item, "call", log))
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/runner.py", line 241, in call_and_report
    call = CallInfo.from_call(
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/runner.py", line 341, in from_call
    result: TResult | None = func()
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/runner.py", line 242, in <lambda>
    lambda: runtest_hook(item=item, **kwds), when=when, reraise=reraise
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_hooks.py", line 513, in __call__
    return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_manager.py", line 120, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_callers.py", line 103, in _multicall
    res = hook_impl.function(*args)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/runner.py", line 174, in pytest_runtest_call
    item.runtest()
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/python.py", line 1627, in runtest
    self.ihook.pytest_pyfunc_call(pyfuncitem=self)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_hooks.py", line 513, in __call__
    return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_manager.py", line 120, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/pluggy/_callers.py", line 103, in _multicall
    res = hook_impl.function(*args)
  File "/Users/neel/sentry/sdks/sentry-python/.tox/py3.8-common/lib/python3.8/site-packages/_pytest/python.py", line 159, in pytest_pyfunc_call
    result = testfunction(**testargs)
  File "/Users/neel/sentry/sdks/sentry-python/tests/test_transport.py", line 210, in test_transport_works
    client = make_client(
  File "/Users/neel/sentry/sdks/sentry-python/tests/test_transport.py", line 149, in inner
    return Client(
  File "/Users/neel/sentry/sdks/sentry-python/sentry_sdk/client.py", line 283, in __init__
    self._init_impl()
  File "/Users/neel/sentry/sdks/sentry-python/sentry_sdk/client.py", line 412, in _init_impl
    self.integrations = setup_integrations(
  File "/Users/neel/sentry/sdks/sentry-python/sentry_sdk/integrations/__init__.py", line 198, in setup_integrations
    for integration_cls in iter_default_integrations(
  File "/Users/neel/sentry/sdks/sentry-python/sentry_sdk/integrations/__init__.py", line 53, in iter_default_integrations
    logger.debug(
  File "/Users/neel/.asdf/installs/python/3.8.19/lib/python3.8/logging/__init__.py", line 1434, in debug
    self._log(DEBUG, msg, args, **kwargs)
  File "/Users/neel/.asdf/installs/python/3.8.19/lib/python3.8/logging/__init__.py", line 1589, in _log
    self.handle(record)
  File "/Users/neel/.asdf/installs/python/3.8.19/lib/python3.8/logging/__init__.py", line 1599, in handle
    self.callHandlers(record)
  File "/Users/neel/sentry/sdks/sentry-python/sentry_sdk/integrations/logging.py", line 127, in sentry_patched_callhandlers
    return old_callhandlers(self, record)
  File "/Users/neel/.asdf/installs/python/3.8.19/lib/python3.8/logging/__init__.py", line 1661, in callHandlers
    hdlr.handle(record)
  File "/Users/neel/.asdf/installs/python/3.8.19/lib/python3.8/logging/__init__.py", line 954, in handle
    self.emit(record)
  File "/Users/neel/.asdf/installs/python/3.8.19/lib/python3.8/logging/__init__.py", line 1088, in emit
    stream.write(msg + self.terminator)
  File "/Users/neel/sentry/sdks/sentry-python/sentry_sdk/transport.py", line 164, in __del__
    self.kill()
  File "/Users/neel/sentry/sdks/sentry-python/sentry_sdk/transport.py", line 588, in kill
    self._worker.kill()
  File "/Users/neel/sentry/sdks/sentry-python/sentry_sdk/worker.py", line 83, in kill
    logger.debug("background worker got kill request")
  File "/Users/neel/.asdf/installs/python/3.8.19/lib/python3.8/logging/__init__.py", line 1434, in debug
    self._log(DEBUG, msg, args, **kwargs)
  File "/Users/neel/.asdf/installs/python/3.8.19/lib/python3.8/logging/__init__.py", line 1589, in _log
    self.handle(record)
  File "/Users/neel/.asdf/installs/python/3.8.19/lib/python3.8/logging/__init__.py", line 1599, in handle
    self.callHandlers(record)
  File "/Users/neel/sentry/sdks/sentry-python/sentry_sdk/integrations/logging.py", line 127, in sentry_patched_callhandlers
    return old_callhandlers(self, record)
Message: \'background worker got kill request\'
Arguments: ()

Copy link

codecov bot commented Jul 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.68%. Comparing base (da8332a) to head (e87fabb).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4590   +/-   ##
=======================================
  Coverage   80.67%   80.68%           
=======================================
  Files         156      156           
  Lines       16507    16498    -9     
  Branches     2806     2806           
=======================================
- Hits        13317    13311    -6     
+ Misses       2303     2296    -7     
- Partials      887      891    +4     
Files with missing lines Coverage Δ
sentry_sdk/monitor.py 82.53% <ø> (-0.54%) ⬇️
sentry_sdk/sessions.py 79.43% <ø> (-0.29%) ⬇️
sentry_sdk/transport.py 80.53% <ø> (+<0.01%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

🚀

but let's also have @sentrivana approve before the merge

@sl0thentr0py sl0thentr0py force-pushed the neel/flaky-transport branch from 09c2e32 to e87fabb Compare July 17, 2025 14:29
@sl0thentr0py sl0thentr0py requested a review from sentrivana July 17, 2025 14:33
@sl0thentr0py sl0thentr0py enabled auto-merge (squash) July 17, 2025 14:38
@sl0thentr0py sl0thentr0py merged commit 34dcba4 into master Jul 17, 2025
137 checks passed
@sl0thentr0py sl0thentr0py deleted the neel/flaky-transport branch July 17, 2025 14:41
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.

3 participants