refactor(internal): narrow gevent module cloning to the threading family#18779
Draft
mabdinur wants to merge 4 commits into
Draft
refactor(internal): narrow gevent module cloning to the threading family#18779mabdinur wants to merge 4 commits into
mabdinur wants to merge 4 commits into
Conversation
ddtrace's agent HTTP/UDS connections needed unpatched, real-thread sockets under gevent. This was achieved indirectly: cleanup_loaded_modules() force- imported ddtrace.internal.http and ddtrace.internal.uds before the global sys.modules sweep so they captured pre-gevent module copies. Capture the only socket primitives gevent replaces (socket.socket and socket.create_connection) directly in _unpatched at import time, the same way unpatched_Popen/open already work. These attribute references survive gevent's in-place monkey-patching, so uds and http no longer depend on the sweep having run. Remove the force-import accordingly. This removes one coupling between agent I/O and the module-cloning mechanism, a step toward narrowing that mechanism. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mabdinur
added
the
changelog/no-changelog
Codeowners resolved as |
|
…ules Remove the broad "unload every non-KEEP module from sys.modules" sweep and the ever-growing KEEP_MODULES allowlist. gevent monkey-patches stdlib modules in place, so application code that imports them already gets the greenlet-friendly versions; the sweep only ever existed to give ddtrace unpatched copies. With native periodic threads and agent I/O now holding unpatched socket primitives (captured in _unpatched), the only modules ddtrace still needs its own unpatched copies of are the threading family, which UNLOAD_MODULES already covers. This eliminates the KEEP_MODULES churn (asyncio, dataclasses, pathlib, typing, protobuf, ...) that accreted to work around the broad sweep, and with it a class of gevent-interaction bugs. Tests: test_auto, contrib/gevent, ddtracerun, and profiling (incl. gevent monkey-patch simulations) all pass locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mabdinur
changed the title
BenchmarksBenchmark execution time: 2026-06-29 21:00:42 Comparing candidate commit 90296fa in PR branch Found 0 performance improvements and 6 performance regressions! Performance is the same for 612 metrics, 10 unstable metrics. scenario:iast_aspects-re_expand_aspect
scenario:iastaspects-strip_aspect
scenario:iastaspects-translate_aspect
scenario:iastaspectsospath-ospathbasename_aspect
scenario:span-start
scenario:telemetryaddmetric-1-count-metric-1-times
|
…ading CI surfaced a RecursionError in ssl.SSLContext.options under gunicorn+gevent: leaving ssl shared lets gevent patch it in place, producing a self-referential MRO. The old broad sweep unloaded ssl (and the rest of gevent's patched set) implicitly. Narrow UNLOAD_MODULES to exactly gevent's in-place patched modules (socket, ssl, select, selectors, queue, signal, subprocess, time, threading, _thread) plus concurrent.futures/reprlib, instead of only the threading family. os is left shared (boot-loaded, fork-hooks only). Validated: ssl.create_default_context() works after gevent.monkey.patch_all() under forced module cloning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e cloning Narrowed cloning never unloads asyncio, so the _asyncio C extension stays in sync with asyncio.events and event loop registration keeps working. Replace the obsolete skipped "asyncio not imported" test (the old workaround) with a bare subprocess check under forced cloning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Because we mostly use the internal native periodic threads, the threading module is no longer an issue and we probably don't need to clone that anymore. So it seems like we're quite close to removing the module cloning mechanism entirely. In #18792 I'm trying to resolve the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Narrows ddtrace's module-cloning mechanism (
cleanup_loaded_modulesinddtrace/bootstrap/cloning.py) from a broad "unload almost everything fromsys.modules" sweep down to just the threading-family modules ddtrace genuinely needs unpatched copies of.Background
gevent monkey-patches stdlib modules in place. Application code that imports them therefore gets the greenlet-friendly versions automatically — the cloning sweep was never needed for application correctness. Its only purpose was to give ddtrace unpatched copies of modules it used on real threads.
Two things made the broad sweep obsolete:
PeriodicThreads (C/pthreads), so ddtrace no longer relies on Pythonthreadingfor its own threads.What remained was a blunt sweep that unloaded nearly everything and required an ever-growing
KEEP_MODULESallowlist (asyncio,dataclasses,pathlib,typing,protobuf,copyreg,reprlib, ...) — each entry a module that broke when cloned. That allowlist churn is also the source of a class of gevent-interaction bugs.Changes
1. Self-contained agent socket I/O. gevent only ever replaces two socket attributes (
socket.socket,socket.create_connection— verified empirically). Capture those inddtrace/internal/_unpatched.pyat import time, exactly like the existingunpatched_Popen/unpatched_open. Captured attribute references survive gevent's in-place patching._unpatched.py: addunpatched_socket,unpatched_create_connection.uds.py: build the UDS socket viaunpatched_socket.http.py: pinself._create_connectiontounpatched_create_connection.cloning.py: drop the force-import ofddtrace.internal.http/uds(no longer needed for reference capture).2. Narrow the sweep. Remove the broad "unload all except
KEEP_MODULES" loop and theKEEP_MODULESallowlist entirely. Keep only the targetedUNLOAD_MODULESset (threading,_thread,time,concurrent.futures,reprlib) that ddtrace still needs unpatched copies of (chiefly the profiler's real-thread tracking). Everything else stays shared and gevent-patched in place.Testing
All run locally on Python 3.13 (Linux):
tests/internal/test_auto.py— the "must pass ALWAYS" canary; updated to assert the new invariants (ddtrace keeps unpatched threading; agent I/O works after socket is patched). 5 passed.tests/contrib/gevent— 13 passed.tests/commands/test_runner.py(ddtracerun) — 102 passed. The obsoleteMODULES_TO_CHECK=["asyncio"]leak-checks (which asserted asyncio is not retained) are replaced with event-loop registration tests, since narrowing intentionally keeps asyncio loaded.tests/profiling/collector/test_threading.py+ profiling suite — 335 passed, including the gevent monkey-patch simulation tests.Risks
Medium. This changes a load-bearing startup mechanism for gevent users. Mitigations: native threads and the keystone remove the original reasons for the broad sweep; the profiler's gevent-simulation tests and the gevent contrib suite pass; the canary test is updated to the new contract. CI's full gevent/gunicorn/profiling matrix is the real validation gate.
Relationship to #18776
The asyncio event-loop fix (#18776) added
asyncio/_asynciotoKEEP_MODULES. This PR removesKEEP_MODULESaltogether — asyncio is simply never unloaded — so it supersedes that fix. If #18776 merges first, this branch drops the now-redundant allowlist entry on rebase.