Fix continuation leakage on netty http2 client pipeline#11595
Fix continuation leakage on netty http2 client pipeline#11595amarziali wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bf8a318e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (Boolean.TRUE.equals( | ||
| pipeline.channel().attr(HTTP2_CONNECTION_CODEC_ATTRIBUTE_KEY).get())) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Preserve connect error spans for HTTP/2
For HTTP/2 prior-knowledge clients the Http2FrameCodec is added during channel initialization, before ChannelPipeline.connect, so this early return also skips capturing the continuation that ChannelFutureListenerInstrumentation.OperationCompleteAdvice consumes to create netty.connect error spans on failed connects. In the inspected Netty 4.1 workflow, a refused/timeout H2 connection never reaches the request handler, so there is no later consumer to report the connection failure under the active parent trace.
Useful? React with 👍 / 👎.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
9652b38 to
c2a3193
Compare
What Does This Do
Fixes an orphaned async continuation in Netty 4.1 HTTP/2 client flows.
For HTTP/1, the connect continuation captured in ConnectAdvice is later consumed by the request handler. In HTTP/2 prior-knowledge mode, the continuation is captured on the connection channel but requests are written on stream channels, so the continuation was never consumed or canceled.
This change marks channels that have an HTTP/2 connection codec as HTTP/2 connections and skips capturing the connect continuation for them. This avoids creating a continuation with no consumer. The request handler now also checks the parent channel for an existing connect continuation, covering stream-channel request paths where a parent-channel continuation is valid.
Issue spotted using an experimental continuation lifecycle tracker that gave:
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]