Skip to content

Commit

Permalink
fix: resolve asyncio event loop bug when gevent is installed [backpor…
Browse files Browse the repository at this point in the history
…t 2.19] (#11921)

Backport b769e1e from #11904 to 2.19.

## Description

Reverts a change
([4e31278](4e31278))
that introduced a regression in asyncio event loops when gevent is
installed. This issue cannot be reproduced on macOS; it was detected on
Ubuntu 24.

## Background
- In ddtrace v2.11.0, the ddtrace-py [introduced
support](dc000ae)
for crash tracking, and the crash tracker is started via ddtrace-run or
by importing ddtrace.auto
([here](https://github.com/DataDog/dd-trace-py/blob/main/ddtrace/auto.py)).
- Before the crash tracker is started, it [reads the agent
URL](https://github.com/DataDog/dd-trace-py/blob/v2.11.0/ddtrace/internal/core/crashtracking.py#L31)
using the
[ensure_binary](https://github.com/DataDog/dd-trace-py/blob/a58f139e24d78a66468dbc7f67ec42c2bdfad8ee/ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyx#L50)
function.
- The ensure_binary function imports unittest.mock, which imports
[asyncio](https://github.com/DataDog/dd-trace-py/blob/v2.11.0/ddtrace/internal/compat.py#L72)
as a side effect.
- After crash tracking is started, ddtrace unloads all modules that were
imported during the setup of ddtrace features
([here](https://github.com/DataDog/dd-trace-py/blob/main/ddtrace/bootstrap/sitecustomize.py#L120C5-L120C27)).
This includes asyncio.
- At this point, asyncio has been added to and then removed from
sys.modules.
- When asyncio is imported in a user's application event loops fail to
be set. This is seen in the script below (the script was run on Ubuntu
24 with gevent>=24)

### Script
```
import asyncio
import time

loop = asyncio.get_event_loop()
loop_id = id(loop)
new_loop = asyncio.new_event_loop()
new_loop_id = id(new_loop)

print(f"old: {loop_id} new: {new_loop_id}")
asyncio.set_event_loop(new_loop)

check = asyncio.get_event_loop()
check_id = id(check)
print(f"check: {id(check)}")


while check_id != new_loop_id:
    print("MISMATCH")
    print(f"check: {id(check)}")
    time.sleep(1)
    check = asyncio.get_event_loop()
    check_id = id(check)
```

### Output

```
MISMATCH
check: 131621237174144
MISMATCH
check: 131621237174144
MISMATCH
check: 131621237174144
MISMATCH
check: 131621237174144
MISMATCH
check: 131621237174144
MISMATCH
```

## Next steps

Investigate module unloading and asyncio incompatibility

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met 
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

Co-authored-by: Munir Abdinur <munir.abdinur@datadoghq.com>
  • Loading branch information
github-actions[bot] and mabdinur authored Jan 14, 2025
1 parent 5824827 commit befc80b
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 133 deletions.
117 changes: 0 additions & 117 deletions .github/workflows/test_frameworks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -111,68 +111,6 @@ jobs:
if: needs.needs-run.outputs.outcome == 'success'
run: cat debugger-expl.txt

sanic-testsuite:
strategy:
matrix:
include:
# TODO: profiling fails with a timeout error
#- suffix: Profiling
# profiling: 1
# iast: 0
# appsec: 0
- suffix: IAST
profiling: 0
iast: 1
appsec: 0
- suffix: APPSEC
profiling: 0
iast: 0
appsec: 1
- suffix: Tracer only
profiling: 0
iast: 0
appsec: 0
name: Sanic 24.6 (with ${{ matrix.suffix }})
runs-on: ubuntu-20.04
needs: needs-run
timeout-minutes: 15
env:
DD_PROFILING_ENABLED: ${{ matrix.profiling }}
DD_IAST_ENABLED: ${{ matrix.iast }}
DD_APPSEC_ENABLED: ${{ matrix.appsec }}
DD_TESTING_RAISE: true
CMAKE_BUILD_PARALLEL_LEVEL: 12
DD_DEBUGGER_EXPL_OUTPUT_FILE: debugger-expl.txt
defaults:
run:
working-directory: sanic
steps:
- uses: actions/checkout@v4
if: needs.needs-run.outputs.outcome == 'success'
with:
persist-credentials: false
path: ddtrace
- uses: actions/checkout@v4
if: needs.needs-run.outputs.outcome == 'success'
with:
persist-credentials: false
repository: sanic-org/sanic
ref: v24.6.0
path: sanic
- uses: actions/setup-python@v5
if: needs.needs-run.outputs.outcome == 'success'
with:
python-version: "3.11"
- name: Install sanic and dependencies required to run tests
if: needs.needs-run.outputs.outcome == 'success'
run: pip3 install '.[test]' aioquic
- name: Install ddtrace
if: needs.needs-run.outputs.outcome == 'success'
run: pip3 install ../ddtrace
- name: Run tests
if: needs.needs-run.outputs.outcome == 'success'
run: ddtrace-run pytest -k "not test_reloader and not test_reload_listeners and not test_no_exceptions_when_cancel_pending_request and 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 and not test_redirect_with_params and not test_keep_alive_client_timeout and not test_logger_vhosts and not test_ssl_in_multiprocess_mode"

django-testsuite:
strategy:
matrix:
Expand Down Expand Up @@ -963,58 +901,3 @@ jobs:
- name: Debugger exploration results
if: needs.needs-run.outputs.outcome == 'success'
run: cat debugger-expl.txt

beautifulsoup-testsuite-4_12_3:
strategy:
matrix:
include:
# TODO: profiling is disabled due to a bug in the profiler paths
# - suffix: Profiling
# profiling: 1
# iast: 0
# appsec: 0
- suffix: IAST
profiling: 0
iast: 1
appsec: 0
- suffix: APPSEC
profiling: 0
iast: 0
appsec: 1
- suffix: Tracer only
profiling: 0
iast: 0
appsec: 0
name: Beautifulsoup 4.12.3 (with ${{ matrix.suffix }})
runs-on: "ubuntu-latest"
needs: needs-run
env:
DD_TESTING_RAISE: true
DD_PROFILING_ENABLED: ${{ matrix.profiling }}
DD_IAST_ENABLED: ${{ matrix.iast }}
DD_APPSEC_ENABLED: ${{ matrix.appsec }}
CMAKE_BUILD_PARALLEL_LEVEL: 12
DD_DEBUGGER_EXPL_OUTPUT_FILE: debugger-expl.txt
steps:
- uses: actions/setup-python@v5
if: needs.needs-run.outputs.outcome == 'success'
with:
python-version: '3.9'
- uses: actions/checkout@v4
if: needs.needs-run.outputs.outcome == 'success'
with:
persist-credentials: false
path: ddtrace
- name: Checkout beautifulsoup
if: needs.needs-run.outputs.outcome == 'success'
run: |
git clone -b 4.12.3 https://git.launchpad.net/beautifulsoup
- name: Install ddtrace
if: needs.needs-run.outputs.outcome == 'success'
run: pip3 install ./ddtrace
- name: Pytest fix
if: needs.needs-run.outputs.outcome == 'success'
run: pip install pytest==8.2.1
- name: Run tests
if: needs.needs-run.outputs.outcome == 'success'
run: cd beautifulsoup && ddtrace-run pytest
16 changes: 0 additions & 16 deletions ddtrace/internal/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,32 +56,16 @@
def ensure_text(s, encoding="utf-8", errors="ignore") -> str:
if isinstance(s, str):
return s

if isinstance(s, bytes):
return s.decode(encoding, errors)

# Skip the check for Mock objects as they are used in tests
from unittest.mock import Mock

if isinstance(s, Mock):
return str(s)

raise TypeError("Expected str or bytes but received %r" % (s.__class__))


def ensure_binary(s, encoding="utf-8", errors="ignore") -> bytes:
if isinstance(s, bytes):
return s

# Skip the check for Mock objects as they are used in tests
from unittest.mock import Mock

if isinstance(s, Mock):
return bytes(s)

if not isinstance(s, str):
raise TypeError("Expected str or bytes but received %r" % (s.__class__))

return s.encode(encoding, errors)


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
fixes:
- |
asyncio: Resolves an issue where asyncio event loops fail to register when ``ddtrace-run``/``import ddtrace.auto`` is used and gevent is installed.
13 changes: 13 additions & 0 deletions tests/contrib/asyncio/test_lazyimport.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,16 @@ def test_lazy_import():
assert tracer.context_provider.active() is span
span.finish()
assert tracer.context_provider.active() is None


@pytest.mark.subprocess()
def test_asyncio_not_imported_by_auto_instrumentation():
# Module unloading is not supported for asyncio, a simple workaround
# is to ensure asyncio is not imported by ddtrace.auto or ddtrace-run.
# If asyncio is imported by ddtrace.auto the asyncio event loop with fail
# to register new loops in some platforms (e.g. Ubuntuu).
import sys

import ddtrace.auto # noqa: F401

assert "asyncio" not in sys.modules

0 comments on commit befc80b

Please sign in to comment.