fix: an around_call teardown cannot change a call's outcome, and shares its setup's context - #20
Merged
Conversation
…es its setup's context
One `around_call` generator was stepped from four different places, and both defects
follow from that. `contextvars` compares contexts by identity, so the teardown of every
kind but unary-unary could not reset a token the setup had minted — the pair that scopes
a request id, a correlation id or an OpenTelemetry context to a single call. And an
exception raised after the `yield` had three different fates depending only on the kind
of call it wrapped: it replaced a response the server had already sent, it truncated a
response stream, or it disappeared into the event loop's exception handler.
`_AroundScope` now owns the generator. It closes it exactly once, logs at ERROR and drops
whatever the teardown raises — cancellation excepted, since a task being torn down is not
a teardown failing — and, for the three kinds whose teardown finishes in another task,
steps the setup, the RPC creation and the teardown in one `contextvars.Context` of its
own, which `asyncio.Task(context=...)` enters rather than copies. Unary-unary pins
nothing: its three phases are already one coroutine, and a task per call per layer is a
real cost on the busiest path in the kit.
The tracing interceptor's reason for avoiding the seam ("detaching the OpenTelemetry
context there fails") no longer holds; its docstring now says what does keep it on
`intercept`, which is the cost of an around scope on a layer that does nothing at all
when no SDK is configured.
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
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.
Summary
One
around_callgenerator was being stepped from four different places, and both defects in #19 fall out of that._closing_stream— whichever task drains the response_spawn_background(...), anensure_futuretask whose context is a copycontextvarscompares contexts by identity, so every row but the first refused to reset a token the setup had minted — which rules out the pair that scopes a request id, a correlation id or an OpenTelemetry context to a single call, on three of the four kinds. And because the teardown lived in three callers, an exception out of it had three different fates: it replaced a response the server had already sent, it truncated a response stream, or it disappeared into the event loop's exception handler where nobody was looking._AroundScopenow owns the generator and both problems are fixed where they come from:__aexit__happens in exactly one place, which logs whatever it raises at ERROR against the method and drops it — the same treatment for all four kinds, whether the call succeeded or failed.CancelledError,KeyboardInterruptandSystemExitstill propagate: a task being torn down around the teardown is not the teardown failing, and swallowing it would strand a cancel the caller asked for. Raising before theyieldstill refuses the call, untouched.yieldshare a context.asyncio.Tasktakes acontext=and enters that object rather than copying it, so for the three kinds whose teardown finishes elsewhere the scope creates onecontextvars.Contextand steps the setup, the RPC creation and the teardown in it. The RPC creation is inside it deliberately: pinning only__aenter__/__aexit__would make the token legal and make the value invisible to every layer below, which is the whole point of setting it.Design decisions, and what I turned down
Unary-unary pins nothing. Its setup, call and teardown are already one coroutine and therefore already one context; the guarantee holds there for free. Pinning it anyway would be tidier in the source and would cost every unary-unary call an extra task per around layer — four of them in the canonical chain — on the busiest path in the kit. The four kinds end up observationally identical either way; only this one gets there without paying.
Documenting the constraint instead (the fallback the issue offers) was the answer only if the mechanism did not exist. It does, and a
ContextVarset/reset pair is the canonical use of a seam like this one — "you cannot do the obvious thing here" is what you write when there is no alternative.Swallowing teardown errors only where they reach the caller would have left stream-unary dropping them into the loop handler: the same mistake with four outcomes, which is what the report objects to.
Context.runwithout a task cannot drive a coroutine; anything built on it is a hand-rolledTask.The tracing interceptor keeps
intercept. Its docstring claimedaround_callwas unusable because "detaching the OpenTelemetry context there fails"; after this change that is simply untrue, and a stale rule on the one layer that hit the problem is how this stayed invisible for so long. The docstring now says what does keep it there — withopentelemetry-apiinstalled and no SDK configured, the default state of thetracingextra, every span is non-recording and the layer does nothing at all, while an around scope would be opened for every call regardless, and on a streaming call that is a task per RPC. Moving a shipped observability layer onto a different seam is a change with its own risk and belongs in its own PR, not in the one that changed the seam underneath it.Verification
probe.pyfrom the issue's lab, against this branch (0.1.1 + this change, grpcio 1.83.1):No loop-exception-handler lines left anywhere, and the mid-stream failure still reaches the caller as the status it is.
Both behaviours are tested per RPC kind twice over: at the seam in
tests/unit/interceptors/test_base.py, driving the chain from a task of its own because that boundary is the whole bug, and against a live server intests/integration/test_around_call.py. Revertingbase.pyto master's copy and rerunning them fails 8 of 9 unit cases and 7 of 8 integration cases — the one that passes either way is the contextvars case for unary-unary, which is exactly the row that already worked in the report's table.make checkclean;make test525 passed, 2 xfailed, coverage 96.19% (was 508 passed, 96.02%).Type of change
Checklist
make checkpasses locally (ruff+mypy)CHANGELOG.mdupdated under[Unreleased]— n/a, release-please writes it from the conventional commitdocs/agents.mdincludeddocs/agents.mdgets the seam's row in the interceptor table, the paragraph under it, and a rule of its own (14) for the two guarantees, which had nowhere to live before; rules 14–20 shift down by one. The interceptors guide gets the corrected "after theyield" bullet and a Scoping a value to one call section with the pattern that now works and what it costs. The README paragraph on the seam and thebasemodule preamble say the same in fewer words.Related issues
Closes #19