Skip to content

Commit 82571ae

Browse files
fix: avoid prematurely exhausting wrapped generators during stack unwinding [backport 3.9] (#13694)
Backport a4c9ba0 from #13661 to 3.9. This change resolves an issue wherein `tracer.wrap` on a generator function that yields another generator when called changed the relative ordering of generator exhaustion and exception handling calls in the middle of the stack. This behavior caused applications upgrading from 3.7.0 to 3.9.0 to experience a change in behavior that relied on this relative ordering. In short, this bug existed because `for v in g: yield v` was used where `yield from g` was needed. See PEP380 for explanation of why they're not the same thing https://peps.python.org/pep-0380/#formal-semantics ## 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: Emmett Butler <723615+emmettbutler@users.noreply.github.com>
1 parent 6ed9248 commit 82571ae

File tree

3 files changed

+52
-5
lines changed

3 files changed

+52
-5
lines changed

ddtrace/_trace/tracer.py

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -801,9 +801,7 @@ def func_wrapper(*args, **kwargs):
801801
)
802802

803803
with self.trace(span_name, service=service, resource=resource, span_type=span_type):
804-
gen = f(*args, **kwargs)
805-
for value in gen:
806-
yield value
804+
yield from f(*args, **kwargs)
807805

808806
return func_wrapper
809807

@@ -820,8 +818,7 @@ def _wrap_generator_async(
820818
@functools.wraps(f)
821819
async def func_wrapper(*args, **kwargs):
822820
with self.trace(span_name, service=service, resource=resource, span_type=span_type):
823-
agen = f(*args, **kwargs)
824-
async for value in agen:
821+
async for value in f(*args, **kwargs):
825822
yield value
826823

827824
return func_wrapper
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
fixes:
3+
- |
4+
This fix resolves an issue in which traced nested generator functions had their execution order subtly changed
5+
in a way that affected the stack unwinding sequence during exception handling. The issue was caused
6+
by the tracer's use of simple iteration via ``for v in g: yield v`` during the wrapping of generator functions
7+
where full bidrectional communication with the sub-generator via ``yield from g`` was appropriate. See
8+
PEP380 for an explanation of how these two generator uses differ.

tests/tracer/test_tracer.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,48 @@ def f(tag_name, tag_value):
313313
assert span.duration is not None
314314
assert span.duration > 0.03
315315

316+
def test_tracer_wrap_nested_generators_preserves_stack_order(self):
317+
result = {"handled": False}
318+
319+
def trace_and_call_next(*call_args, **call_kwargs):
320+
def _outer_wrapper(func):
321+
@self.tracer.wrap("foobar")
322+
def _inner_wrapper(*args, **kwargs):
323+
wrapped_generator = func(*args, **kwargs)
324+
try:
325+
yield next(wrapped_generator)
326+
except BaseException as e:
327+
result["handled"] = True
328+
wrapped_generator.throw(e)
329+
330+
return _inner_wrapper
331+
332+
return _outer_wrapper(*call_args, **call_kwargs)
333+
334+
@contextlib.contextmanager
335+
@trace_and_call_next
336+
def wrapper():
337+
try:
338+
yield
339+
except NotImplementedError:
340+
raise ValueError()
341+
342+
exception_note = (
343+
"The expected exception should bubble up from a traced generator-based context manager "
344+
"that yields another generator"
345+
)
346+
try:
347+
with wrapper():
348+
raise NotImplementedError()
349+
except Exception as e:
350+
assert isinstance(e, ValueError), exception_note
351+
else:
352+
assert False, exception_note
353+
assert result["handled"], (
354+
"Exceptions raised by traced generator-based context managers that yield generators should be "
355+
"visible to the caller"
356+
)
357+
316358
def test_tracer_disabled(self):
317359
self.tracer.enabled = True
318360
with self.trace("foo") as s:

0 commit comments

Comments
 (0)