andreaslind01 commented on PR #6742: URL: https://github.com/apache/jmeter/pull/6742#issuecomment-6404709144
> Re-review at `d1692265` (delta since `fc1f3824`). Thanks — every point from my 2026-10-02 and 2026-10-10 reviews that got a reply is addressed, and I've checked each one against the code: the per-context thread state (with a test), the restored `@Deprecated HTTP_VERSION` and the `@since 6.0` tags, the Header Manager headers on method-changing redirects, `HC5SentRequest` reporting the last hop, and the documentation of the HTTP/2 timeout scope, the async pool limit and the spill threshold. CI is green. > > Two threads from the 2026-10-10 review are still open without a reply, plus one small point in the new redirect code. > > ### Still open > * **I/O thread blocking (thread on `HTTPHC5Impl.java`, `setThreadFactory`)** — with one I/O thread per client, SPNEGO/NTLM computation or Cookie Manager calls in `AutoRedirectStrategy` stall every in-flight request of that JMeter thread. A sentence in `component_reference.xml` would be enough. > * **Strong reference from the HTTP/2 I/O thread to the `JMeterContext`** — the new Javadoc on `THREAD_STATE_CLIENTS` says the clients "go away with the context", but for HTTP/2 clients the running I/O thread (a GC root) holds the context through `threadsSharingContext`, and the context holds the client in its thread state. They are only released by `closeClientsOfCurrentThread()`. Regular threads and the Recorder do call `threadFinished()`, so this is about the comment being accurate rather than an actual leak. > > ### Smaller observations > * `AutoRedirectStrategy.java:186` — see inline: two Header Manager headers with the same name lose the second one on a method-changing redirect. > > > _This review was drafted by an AI-assisted tool and > > confirmed by an Apache JMeter maintainer. The findings > > below are observations, not blockers; an Apache JMeter > > maintainer — a real person — will take the next look at the > > PR. If you think a finding is mis-applied, please reply on > > the PR and a maintainer will weigh in._ > > _More on how Apache JMeter handles maintainer review:_ > > [Contributing guide](https://github.com/apache/jmeter/blob/master/CONTRIBUTING.md). Thanks for the thorough re-review. All of these points are addressed: - Documented the I/O-thread blocking behavior in `component_reference.xml`. - Corrected the `THREAD_STATE_CLIENTS` Javadoc to explain that HTTP/2 I/O threads can keep the context and clients reachable until `closeClientsOfCurrentThread()` closes them. - Fixed redirect header restoration so multiple Header Manager headers with the same name survive method-changing redirects, with regression coverage for HTTP/1.1 and HTTP/2. The changes are in place, and CI is green. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
