Skip to content

Commit 6397951

Browse files
fix(sanic): avoids unloading re module when gevent is installed (backport #5867 to 1.11) (#5896)
## Background When gevent is installed (or `DD_UNLOAD_MODULES_FROM_SITECUSTOMIZE=auto`), all modules imported by [ddtrace.bootstrap.sitecustomize](https://github.com/DataDog/dd-trace-py/blob/1.x/ddtrace/bootstrap/sitecustomize.py#L91) are unloaded from sys.modules (except for a small [exclude list](https://github.com/DataDog/dd-trace-py/blob/1.x/ddtrace/bootstrap/sitecustomize.py#L103-L113)). ## Description Currently the [typing module](https://github.com/DataDog/dd-trace-py/blob/1.x/ddtrace/bootstrap/sitecustomize.py#L110) is excluded from module unloading but re is NOT. This causes [isinstance(some_re_pattern, typing.Pattern)](https://github.com/sanic-org/sanic-routing/blob/main/sanic_routing/router.py#L289) to return False (here typing holds a stale reference to [re.Pattern](https://github.com/python/cpython/blob/main/Lib/typing.py#L3422)). ## Risks This bug was called out and reproduced in the following github issue: #5722. However this issue could not be reproduced using riot due to how packages are installed. To workaround this limitation a framework test was added for sanic which reproduced this error. This is less than ideal. Module cloning is a vital part of our codebase and should have test coverage for all common use-cases. ### Explanation Riot uses `pip install -e` to install dependencies. When packages are installed with the `-e` option a `.pth` file is added to `site-packages/` which get run even before `sitecustomize.py`. This is done to extend the Python path to include additional packages. The creation of the `.pth` file has a side-effect of that masks this bug. It loads the re module before importing ddtrace and before LOADED_MODULES can get set, so it is basically the same impact as ignoring/not cleaning up re module. ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines) are followed. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] OPTIONAL: PR description includes explicit acknowledgement of the performance implications of the change as reported in the benchmarks PR comment. ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. Co-authored-by: Brett Langdon <[email protected]>
1 parent 984b12b commit 6397951

File tree

3 files changed

+33
-0
lines changed

3 files changed

+33
-0
lines changed

.github/workflows/test_frameworks.yml

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,33 @@ jobs:
5252
# log output and it contains phony error messages.
5353
run: PYTHONPATH=../ddtrace/tests/debugging/exploration/ ddtrace-run pytest test --continue-on-collection-errors -v -k 'not test_simple'
5454

55+
sanic-testsuite-22_12:
56+
runs-on: ubuntu-20.04
57+
env:
58+
# Regression test for DataDog/dd-trace-py/issues/5722
59+
DD_UNLOAD_MODULES_FROM_SITECUSTOMIZE: true
60+
defaults:
61+
run:
62+
working-directory: sanic
63+
steps:
64+
- uses: actions/checkout@v3
65+
with:
66+
path: ddtrace
67+
- uses: actions/checkout@v3
68+
with:
69+
repository: sanic-org/sanic
70+
ref: v22.12.0
71+
path: sanic
72+
- uses: actions/setup-python@v4
73+
with:
74+
python-version: "3.11"
75+
- name: Install sanic and dependencies required to run tests
76+
run: pip3 install '.[test]' aioquic
77+
- name: Install ddtrace
78+
run: pip3 install ../ddtrace
79+
- name: Run tests
80+
run: ddtrace-run pytest -k "not test_add_signal and not test_ode_removes and not test_skip_touchup and not test_dispatch_signal_triggers and not test_keep_alive_connection_context"
81+
5582
django-testsuite-3_1:
5683
strategy:
5784
matrix:

ddtrace/bootstrap/sitecustomize.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ def drop(module_name):
128128
"asyncio",
129129
"concurrent",
130130
"typing",
131+
"re", # referenced by the typing module
131132
"logging",
132133
"attr",
133134
"google.protobuf", # the upb backend in >= 4.21 does not like being unloaded
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
fixes:
3+
- |
4+
sanic: Resolves ``sanic_routing.exceptions.InvalidUsage`` error raised when
5+
gevent is installed or ``DD_UNLOAD_MODULES_FROM_SITECUSTOMIZE`` is set to True.

0 commit comments

Comments
 (0)