Read only when the channel does not auto-read

[pavel-ptashyts]

Problem

AsyncHttpClientHandler requested a read from both lifecycle callbacks:

public void channelActive(ChannelHandlerContext ctx)       { ctx.read(); }
public void channelReadComplete(ChannelHandlerContext ctx) { ctx.read(); }

Netty's HeadContext already calls Channel#read() immediately after firing
either event whenever autoRead is on. AsyncHttpClient never clears autoRead
(no ChannelOption.AUTO_READ anywhere in the codebase) and Netty defaults it to
on, so that was the path for every connection: each read cycle traversed the
outbound pipeline and reached doBeginRead twice instead of once. The cost is
per read cycle, so it scales with how many reads a response takes.

Change

Drive the read from the handler only when autoRead is off.

That is also a fix in its own right: a caller who disabled autoRead through
setChannelOption previously had the setting silently defeated by these two
unconditional reads, since the handler kept requesting reads regardless.

Verification of the Netty behaviour

Checked against netty-codec-http2 / netty-transport 4.2.16.Final, the
version this project builds against, rather than assumed:

• DefaultChannelPipeline$HeadContext.channelActive and .channelReadComplete
  both call readIfIsAutoRead(), which is
  if (channel.config().isAutoRead()) channel.read();
• DefaultChannelConfig's constructor initialises autoRead to 1, so on.
• HTTP/2 stream channels inherit both behaviours:
  AbstractHttp2StreamChannel$2 extends DefaultChannelPipeline, and
  Http2StreamChannelConfig extends DefaultChannelConfig without overriding
  isAutoRead.

The last point matters because this is the shared base class of HttpHandler,
WebSocketHandler and Http2Handler, and neither of the two callbacks is
overridden by any of them, so the change applies to HTTP/1.1, WebSocket and
HTTP/2 stream channels alike.

Verification of the build

mvnw clean verify - BUILD SUCCESS, 1371 tests, 0 failures, 0 errors,
19 skipped. Error Prone, NullAway and Revapi all clean.

The suites covering the paths most exposed to a change in read behaviour are
green: 168 HTTP/2 tests (BasicHttp2Test, Http2MultiplexBugRegressionTest,
Http2StreamingBodyFlowControlTest, Http2StreamOrphanRegressionTest,
Http2ConformanceRegressionTest and the rest) and 36 WebSocket tests
(TextMessageTest, ByteMessageTest, CloseCodeReasonMessageTest,
WebSocketWriteFutureTest, ws.ProxyTunnellingTest).

Caveat on the testing gate: AGENTS.md requires the build to run on JDK 11 and
no JDK 11 is installed on this machine, so it was run on JDK 17 (also in the
CI matrix). The JDK 11 leg of CI on this PR is the real gate.

No public API change. Same review pass as #2300 and #2301.

Claude Code on behalf of @pavel-ptashyts

🤖 Generated with Claude Code

[hyperxpro]

Worth noting for anyone else reading this, HeadContext calls readIfIsAutoRead after fireChannelActive returns, not as part of the propagation. So it does not matter that this handler never forwards the event, the read still happens. Same for channelReadComplete below.

[pavel-ptashyts]

Right, and that ordering is what makes this safe. The javadoc says it now, so the next reader does not have to wonder whether the handler swallowing the event matters.

[hyperxpro]

I traced the HTTP/2 side since that is where dropping one of two reads could have bitten us. Inside the read loop the second read only flipped readStatus from IN_PROGRESS to REQUESTED and resetReadStatus put it straight back, so the stream channel ends in the same state either way. Outside the loop the status is already reset to IDLE before the event is fired, so the HeadContext read does the full drain on its own. No read is lost, and we lose a layer of re-entrancy.

[pavel-ptashyts]

Thank you for tracing that. The pipeline and config side I could confirm from the bytecode, but the readStatus transitions inside the read loop were the part I could not rule out that way.

[hyperxpro]

This sentence is a claim about the rest of the codebase and it goes stale the day someone puts AUTO_READ in a config. Can we keep the comment to the mechanism and drop the survey of current usage.

[pavel-ptashyts]

Dropped. Fair point that it dates the comment for no benefit; the mechanism holds whether or not anything in the codebase sets AUTO_READ.

[hyperxpro]

Netty spells this readIfNeeded in SslHandler, and Http2ConnectionHandler.channelReadComplete0 does the same guard inline. Matching the name and pointing at one of those in the javadoc would make it obvious this is the house style and not something we invented.

[pavel-ptashyts]

Renamed to readIfNeeded, and the javadoc now points at SslHandler and at the inline version in Http2ConnectionHandler.channelReadComplete0. Better to match what Netty already does in two places than to invent a name for it.

[hyperxpro]

Right call checking this per event instead of caching it at channelActive. autoRead is a volatile field and a user handler can flip it mid connection, so a cached value would go wrong quietly.

[pavel-ptashyts]

Yes. autoRead is volatile and a user handler can flip it mid connection, so a value cached at channelActive would have been wrong in a way that produces a stalled connection rather than a failure.