Skip to content

Defer connectionEstablished observer callback and H2 auto-read until - #3461

Open
daschl wants to merge 2 commits into
apple:mainfrom
daschl:ca-accept
Open

Defer connectionEstablished observer callback and H2 auto-read until#3461
daschl wants to merge 2 commits into
apple:mainfrom
daschl:ca-accept

Conversation

@daschl

@daschl daschl commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

after connection acceptors complete

Motivation

Two related issues stem from the same root cause — actions that should happen after all connection acceptors complete instead happen when the connection Single resolves in tryCompleteSubscriber/completeSubscriber:

  1. connectionEstablished/multiplexedConnectionEstablished observer callbacks fire before Late/Legacy ConnectionAcceptors evaluate. This means connection time measurement excludes acceptor time, and the callback fires even if the acceptor later rejects the connection.

  2. HTTP/2 server enables auto-read in AbstractH2ParentConnection#handlerAdded() before acceptors evaluate. If the client's HTTP/2 preface and request arrive in the same TCP segment, the server processes the request before the acceptor can reject — meaning StreamingHttpService.handle() gets invoked for connections that should have been rejected.

Modifications

  1. Add a deferConnectionEstablished parameter to DefaultNettyConnection.initChannel() and guard the connectionEstablished call in completeSubscriber(). Add a method for callers to invoke after acceptors pass.

  2. Add a deferAutoRead parameter to AbstractH2ParentConnection. When true, handlerAdded() no longer enables auto-read. The server passes true; the client passes false (unchanged behavior).

  3. Add new tests covering observer callback timing and H2 auto-read deferral across H1, H2 prior-knowledge, and H2 ALPN code paths.

Result

connectionEstablished/multiplexedConnectionEstablished observer
callbacks now fire after all connection acceptors complete, and only
when the connection is accepted. HTTP/2 auto-read is deferred until
the connectionConsumer runs, preventing requests from reaching the
service handler before acceptors can reject. Client-side behavior is
unchanged.

@daschl
daschl force-pushed the ca-accept branch 4 times, most recently from 878ffe6 to 8fcc9cd Compare March 24, 2026 14:05
@daschl
daschl marked this pull request as ready for review March 24, 2026 14:05

@bryce-anderson bryce-anderson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like the underlying problem is that we setup our pipelines too early. If that's true and how to address it might b a lot more work than we want to tackle.

daschl added 2 commits April 28, 2026 14:51
after connection acceptors complete

Motivation:

Two related issues stem from the same root cause — actions that should
happen after all connection acceptors complete instead happen when the
connection Single resolves in tryCompleteSubscriber/completeSubscriber:

 1. connectionEstablished/multiplexedConnectionEstablished observer
    callbacks fire before Late/Legacy ConnectionAcceptors evaluate. This
    means connection time measurement excludes acceptor time, and the
    callback fires even if the acceptor later rejects the connection.

 2. HTTP/2 server enables auto-read in AbstractH2ParentConnection
    .handlerAdded() before acceptors evaluate. If the client's HTTP/2
    preface and request arrive in the same TCP segment, the server
    processes the request before the acceptor can reject — meaning
    StreamingHttpService.handle() gets invoked for connections that
    should have been rejected.

Modifications:

 1. Add a deferConnectionEstablished parameter to
    DefaultNettyConnection.initChannel() and guard the
    connectionEstablished call in completeSubscriber(). Add a public
    notifyConnectionEstablished(ConnectionObserver) method for callers
    to invoke after acceptors pass.

 2. Add a deferAutoRead parameter to AbstractH2ParentConnection. When
    true, handlerAdded() no longer enables auto-read. The server passes
    true; the client passes false (unchanged behavior).

 3. Store the ConnectionObserver on NettyHttpServerConnection and
    H2ServerParentConnectionContext so the connectionConsumer (which
    runs after all acceptors) can invoke notifyConnectionEstablished()
    followed by process(true) for H1 or setAutoRead(true) for H2.

 4. Update all four server-side TcpServerBinder.bind() call sites
    consistently: NettyHttpServer, H2ServerParentConnectionContext,
    DeferredServerChannelBinder, and OptionalSslNegotiator.

 5. Add new tests covering observer callback timing and H2 auto-read
    deferral across H1, H2 prior-knowledge, and H2 ALPN code paths.

 Result:

 connectionEstablished/multiplexedConnectionEstablished observer
 callbacks now fire after all connection acceptors complete, and only
 when the connection is accepted. HTTP/2 auto-read is deferred until
 the connectionConsumer runs, preventing requests from reaching the
 service handler before acceptors can reject. Client-side behavior is
 unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants