fix: HTTP 2.0 Throws KeyError rather than the internal exception thrown in th...

[yhay81]

Fixes #808.

Summary

HTTP 2.0 Throws KeyError rather than the internal exception thrown in the thread

Validation

• Mechanical gate: +68/-4, tests passed
• Adversarial review: approved

---

🤖 AI-authored PR, operated by @yhay81.

[codspeed-hq]

Merging this PR will not alter performance

✅ 15 untouched benchmarks
⏩ 7 skipped benchmarks¹

---

_{Comparing yhay81:agent/issue-808 (e8fb2c5) with main (fccb6b9)}

Footnotes

1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[cubic-dev-ai]

1 issue found across 4 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="tests/httpcore2/_sync/test_http2.py">

<violation number="1" location="tests/httpcore2/_sync/test_http2.py:99">
P2: The test calls `conn._response_closed` only once on a live stream. The docstring says the test is for the "twice" / already-removed scenario, but the second call that would exercise the fix's `if stream_id in self._events:` guard is missing. Add a second `conn._response_closed(stream_id=1)` call so the test actually verifies the no-op behavior on an already-removed stream.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: The test calls conn._response_closed only once on a live stream. The docstring says the test is for the "twice" / already-removed scenario, but the second call that would exercise the fix's if stream_id in self._events: guard is missing. Add a second conn._response_closed(stream_id=1) call so the test actually verifies the no-op behavior on an already-removed stream.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At tests/httpcore2/_sync/test_http2.py, line 99:

<comment>The test calls `conn._response_closed` only once on a live stream. The docstring says the test is for the "twice" / already-removed scenario, but the second call that would exercise the fix's `if stream_id in self._events:` guard is missing. Add a second `conn._response_closed(stream_id=1)` call so the test actually verifies the no-op behavior on an already-removed stream.</comment>

<file context>
@@ -69,6 +69,37 @@ def test_http2_connection_closed() -> None:
+        conn.request("GET", "https://example.com/")
+
+        # The stream was closed when the response completed.
+        conn._response_closed(stream_id=1)
+
+
</file context>

Suggested change

	conn._response_closed(stream_id=1)
	# Call twice: first close succeeds, second close must be a no-op (no KeyError).
	conn._response_closed(stream_id=1)
	conn._response_closed(stream_id=1)

[mbeijen]

This is not really a problem and should be ignored

[mbeijen]

Thanks, this indeed fixes a real problem. My PR #1012 and #1013 were also around this code but somehow I overlooked this issue! @Kludex I think this is a valid fix, please consider

[Kludex]

The cleanup is now idempotent under the state lock, and the semaphore permit is released only when the stream entry is actually removed. The regression test does exercise the duplicate-close path: conn.request() reads and closes the response, then the explicit _response_closed() call performs the second cleanup. Targeted HTTP/2 tests also pass against the current main branch.