test: release the first connection before asserting the second reuses it - #23
Merged
Merged
Conversation
The connection-metrics test made two calls without reading either body. aiohttp returns a connection to the pool when the response is released, so the second call raced the first one's cleanup: in isolation the first connection was back in time and the assertion held, under the full suite it often was not. It failed the release pull request's integration job, and reproduced locally in one full run out of two. Reading each body makes the release ordering explicit; three consecutive full integration runs pass.
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
1 participant
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.
test__conn_metrics__annotate_the_call_spanfailed the integration job on the release pull request (#22), assertinghttp.connection.reused is Trueon the second call and gettingFalse. The test is two days old — it arrived with theconn_metricsfix in #20 — and it passed on both earlier runs today, so it is a flake rather than a regression.Why it flakes. The test made two calls and read neither body. aiohttp returns a connection to the pool when the response is released, which for an unread body happens on cleanup rather than at the call site. The second request therefore raced the first one's release: with the file running alone the connection was back in the pool in time and the assertion held; under the whole integration suite it often was not.
Reproduced locally: one failure in two full
-m integrationruns before the change. After it, three consecutive full runs pass, 174 tests each.The fix is in the test, not the adapter. Reading each body makes the release ordering explicit instead of leaving it to scheduling. What the test is actually about — that
Attempt.connreaches the call span, that a fresh connection carries a connect duration and a pooled one does not — is unchanged and still asserted.make checkpasses, including the three import-linter contracts.