Fix `keep-alive` race when creating `HttpServerOperations` by koisyu · Pull Request #4189 · reactor/reactor-netty · GitHub
Skip to content

Fix keep-alive race when creating HttpServerOperations - #4189

Merged
violetagg merged 1 commit into
reactor:1.2.xfrom
koisyu:fix/toctou-http-traffic-handler
Apr 16, 2026
Merged

Fix keep-alive race when creating HttpServerOperations#4189
violetagg merged 1 commit into
reactor:1.2.xfrom
koisyu:fix/toctou-http-traffic-handler

Conversation

@koisyu

@koisyu koisyu commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #4188

HttpTrafficHandler created new HttpServerOperations from Connection.from(ctx.channel()). On HTTP/1.1 keep-alive
connections, that lookup could still return the previous request's HttpServerOperations instead of the stable channel-
bound connection.

This change captures the stable connection once in handlerAdded() and reuses it for subsequent request-scoped
HttpServerOperations, including the pipelined path. That prevents a new request from inheriting a terminating
ChannelOperations as its parent.

Changes

  • capture the initial stable connection in HttpTrafficHandler.handlerAdded()
  • use that connection when creating HttpServerOperations in channelRead()
  • use the same connection in the pipelined request path in run()
  • add a regression test covering keep-alive reuse

Testing

  • added testKeepAliveNewOpsUsesStableParentConnection
  • verified the test fails without the fix
  • verified the test passes with the fix

@violetagg

violetagg commented Apr 14, 2026

Copy link
Copy Markdown
Member

@koisyu

koisyu commented Apr 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback.

Based on your review comments, I reworked the fix to address the keep-alive handoff race directly instead of caching initialConnection.

My understanding of the root cause is that there is a TOCTOU window between selecting the parent with
Connection.from(ctx.channel()) and binding the next HttpServerOperations, during which the previous ops may terminate and transition its internal connection state.

The revised approach is:

  1. HttpServerOperations.afterInboundComplete() fires a HttpServerOperationsTerminatedEvent through the pipeline.
    This is reached from inside ChannelOperations.terminate() after rebind(connection) succeeds and before the ops transitions to DisposedConnection, so the real channel is still reachable at that point.

  2. In HttpTrafficHandler.channelRead(), if a new HTTP/1.1 request arrives while the previous HttpServerOperations is still the current channel-bound ops, the request is buffered via doPipeline() instead of immediately creating new ops.

  3. In HttpTrafficHandler.userEventTriggered(), receiving HttpServerOperationsTerminatedEvent resumes the keep-alive
    handoff by draining the buffered request or issuing ctx.read().

  4. setAutoRead(true) in HttpServerOperations.handleLastHttpContent() is left in place so the behavior related to reactor netty does not instant propagate exception upon an attempt to send data to closed connection (regression from spring-boot 2.1.4, reactor-netty 0.8.6) #940
    is preserved.

Key design choices:

  • use a pipeline user event to avoid introducing a direct reverse dependency from HttpServerOperations to
    HttpTrafficHandler
  • resume keep-alive handoff only after the previous ops has passed the rebind(connection) point in terminate()
  • keep the LAST_FLUSH_WHEN_NO_READ path aligned with its current synchronous termination behavior

Does this direction look closer to what you had in mind before I update the PR?

@koisyu koisyu closed this Apr 14, 2026
@koisyu koisyu reopened this Apr 14, 2026
@koisyu
koisyu force-pushed the fix/toctou-http-traffic-handler branch 2 times, most recently from 83aa9dd to 381e6a9 Compare April 14, 2026 17:51
@koisyu

koisyu commented Apr 14, 2026

Copy link
Copy Markdown
Contributor Author

@violetagg I pushed an updated version of the branch based on the previous feedback.
Could you please take another look when you have a chance and let me know whether this approach looks reasonable now?

@koisyu
koisyu force-pushed the fix/toctou-http-traffic-handler branch from 381e6a9 to 4a953e4 Compare April 14, 2026 18:26
violetagg
violetagg requested changes Apr 15, 2026
View reviewed changes

@violetagg violetagg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the rework!

Comment thread ...-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperationsResumeReadEvent.java Outdated
Comment thread ...-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperationsResumeReadEvent.java Outdated
Comment thread reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java
Comment thread reactor-netty-http/src/test/java/reactor/netty/http/server/HttpServerTests.java Outdated
@violetagg

violetagg commented Apr 15, 2026

Copy link
Copy Markdown
Member

Can you add "Signed-off-by:" to the commit so that DCO check can pass?

@violetagg violetagg added the type/bug A general bug label Apr 15, 2026
@violetagg violetagg added this to the 1.2.18 milestone Apr 15, 2026
@violetagg violetagg linked an issue Apr 15, 2026 that may be closed by this pull request
HTTP/1.1 keep-alive can hang when a new request inherits the previous request's ChannelOperations as parent connection #4188
Closed
@koisyu
koisyu force-pushed the fix/toctou-http-traffic-handler branch from 4a953e4 to cc8bd92 Compare April 15, 2026 16:03
@koisyu

koisyu commented Apr 15, 2026

Copy link
Copy Markdown
Contributor Author

Can you add "Signed-off-by:" to the commit so that DCO check can pass?

Sure, I've added the sign-off and updated the commit. Thanks.

@koisyu

koisyu commented Apr 15, 2026

Copy link
Copy Markdown
Contributor Author

@violetagg Thank you for the thoughtful review! I've addressed all the comments and updated the PR accordingly. Please let me know if there's anything else I should adjust.

@koisyu
Fix keep-alive race when creating HttpServerOperations
18f5a1b 
Delay keep-alive request handoff until previous ops terminates.
Buffer incoming requests via doPipeline() when existing ops are still
channel-bound, and resume read processing on HttpServerOperationsTerminatedEvent
fired from afterInboundComplete() during terminate().

Fixes reactor#4188

Signed-off-by: kyuhee <koisyu@gmail.com>
@koisyu
koisyu force-pushed the fix/toctou-http-traffic-handler branch from cc8bd92 to 18f5a1b Compare April 15, 2026 16:54
@koisyu

koisyu commented Apr 16, 2026

Copy link
Copy Markdown
Contributor Author

@violetagg violetagg changed the title Fix keep-alive race when creating HttpServerOperations Fix keep-alive race when creating HttpServerOperations Apr 16, 2026
@violetagg
violetagg merged commit 3b03922 into reactor:1.2.x Apr 16, 2026
20 of 21 checks passed
violetagg added a commit that referenced this pull request Apr 16, 2026
@violetagg
Merge #4189 into 1.3.6
17ce71a 
Signed-off-by: Violeta Georgieva <696661+violetagg@users.noreply.github.com>
@knutwannheden knutwannheden mentioned this pull request Aug 20, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@violetagg violetagg violetagg approved these changes

Assignees

No one assigned

Labels

type/bug A general bug

Projects

None yet

Milestone

1.2.18

Development

Successfully merging this pull request may close these issues.

HTTP/1.1 keep-alive can hang when a new request inherits the previous request's ChannelOperations as parent connection

2 participants

@koisyu @violetagg