Fix HttpClientResponse body()/end() race by snazy · Pull Request #6064 · eclipse-vertx/vert.x

snazy · 2026-04-15T11:02:11Z

This change fixes the race in HttpClientResponseImpl described in #6038: handleTrailers()/handleException() can run before body()/end() creates the promise. If that happens, the completion signal is dropped and the future can hang. This is observed from the call site as a HTTP request timeo ut.

This fix is only applied to HttpClientResponseImpl:

keep ended/failure state on HttpClientResponseImpl
in body()/end(), create the HttpEventHandler promise while holding synchronized(conn)
after unlock, replay completion/failure via handleEnd()/handleException() when needed

Although HttpEventHandler looks very similar (like it could be affected by the same/similar race condition), it is not affected. I intentionally did not change HttpEventHandler, because it is also used in server request processing. The only case when server request processing could be affected , is when body() is called from a different thread while end() is being processed - that is very unlikely (feels like a misuse).

Also added two new tests to Http1xTest that reliably fail without the change to HttpClientResponseImpl and reliably pass with that change. The added regression tests are testResponseBodyAfterResponseEnd and testResponseEndAfterResponseEnd.

Fixes #6038

This change fixes the race in `HttpClientResponseImpl` described in eclipse-vertx#6038: `handleTrailers()`/`handleException()` can run before `body()`/`end()` creates the promise. If that happens, the completion signal is dropped and the future can hang. This is observed from the call site as a HTTP request timeo ut. This fix is only applied to `HttpClientResponseImpl`: - keep ended/failure state on `HttpClientResponseImpl` - in `body()`/`end()`, create the `HttpEventHandler` promise while holding `synchronized(conn)` - after unlock, replay completion/failure via `handleEnd()`/`handleException()` when needed Although `HttpEventHandler` looks very similar (like it _could_ be affected by the same/similar race condition), it is not affected. I intentionally did not change `HttpEventHandler`, because it is also used in server request processing. The only case when server request processing could be affected , is when `body()` is called from a _different_ thread while `end()` is being processed - that is very unlikely (feels like a misuse). Also added two new tests to `Http1xTest` that reliably fail without the change to `HttpClientResponseImpl` and reliably pass with that change. The added regression tests are `testResponseBodyAfterResponseEnd` and `testResponseEndAfterResponseEnd`. Fixes eclipse-vertx#6038

snazy · 2026-04-15T11:04:53Z

Backport for 5.0 : https://github.com/snazy/vert.x/pull/new/fix-race-6038-5.0
Backport for 4.x: https://github.com/snazy/vert.x/pull/new/fix-race-6038-4.x

vietj · 2026-04-15T11:12:09Z

to simplify, ended should be a Throwable and we should use a specific throwable constant to signal proper ending, e.g. END_SENTINEL

snazy · 2026-04-15T11:30:49Z

@vietj For the END_SENTINEL, you mean to throw that constant/stackless exception from checkEnded()?

…t behavior change)

vietj · 2026-04-15T12:12:28Z

  private Handler<HttpFrame> customFrameHandler;
  private Handler<StreamPriority> priorityHandler;

+  static final class RequestEndedException extends IllegalStateException {


no need for a specific class here, we can simply use == comparison instead

okay, updated

vietj · 2026-04-15T12:13:29Z

-    if (trailers != null) {
-      throw new IllegalStateException();
+    if (ended == ENDED_SENTINEL) {
+      throw ENDED_SENTINEL;


we should throw a new IllegalStateException each time to give proper stack information and not try to reuse the sentinel

vietj · 2026-04-15T12:14:39Z

    HttpEventHandler handler;
    synchronized (conn) {
-      if (trailers != null) {
+      if (ended == ENDED_SENTINEL) {


I think here we should ignore any exception if ended is not null and consider that receiving any excxeption will not lead to further event delivery

Yea, I had that before (and I agree that would be the correct behavior), but some tests like Http1xTest.testInvalidTrailersInHttpClientResponse() started to fail (timeout), so I reverted the previous behavior. Mean, I can try to adopt the test. WDYT?

Pushed 9a3a0ed with that change and updates to the affected tests.

vietj · 2026-04-15T12:33:53Z

-    checkEnded();
-    return eventHandler(true).end();
+  public Future<Void> end() {
+    HttpEventHandler eventHandler;


I think this part should be reimplemented more cleanly with an actual Promise set of the client response that becomes failed/succeeded by events

Hm, yea, a cleaner state handling would be nice. Let me give that a try.

Implemented via 0bbc30b

vietj · 2026-04-15T14:16:00Z

    this.stream = stream;
    this.conn = stream.connection();
+    this.completion = request.context.promise();
+    if (completion.future() != completion) {


in which case would we need that ?

Just a defensive check, if the implementation changes.
The alternative would have been to always have a field Future completionFuture with the same object instance as the Promise completion.

I don't think this is necessary as this always is the case

vietj · 2026-04-15T14:18:23Z

+    // the completion-future.
    synchronized (conn) {
-      this.trailers = trailers;
+      if (completionFuture().isComplete()) {


I think instead we should return when we call tryComplete which returns false, in that case it means we didn't complete the future, some other event did and we should not try to make more progress

like it has been done in handleException

If we return too early here, accesses to an escaped MultiMap from trailers() could miss the the trailers (after observing the completion).

I think we can still make the update and then check return after state update

Hm, that could mutate the trailers field/content after the response has been ended (failure). You think that's acceptable? (I guess you're worried about the extra synchronized cost of FutureImpl.isComplete()?)

yes that is acceptable

vietj · 2026-04-16T09:32:18Z

+      } else if (this.trailers != trailers) {
+        this.trailers.setAll(trailers);
+      }
+      if (!completion.tryComplete()) {


we should call completion.tryComplete after the synchronized block, because this can call handlers in cascade (side effects), synchronized block should only be used to ensure state visibility.

Hm, I’m a bit worried, because moving it out of the lock could introduce races with body()/handler setup (missed or duplicate end delivery).

vietj · 2026-04-16T09:32:56Z

-      if (trailers != null) {
+      // Only report the first exception, and generally only report when the
+      // response has not yet ended.
+      if (!completion.tryFail(e)) {


move tryFail before the synchronized block to avoid side effects within a synchronized block

Similar here, moving tryFail() before the lock feels risky for the same ordering reasons.

snazy mentioned this pull request Apr 15, 2026

Race condition in HttpClientResponse.body() / end() #6038

Open

snazy added 4 commits April 15, 2026 13:42

end sentinel

2e0b619

continue to pass reported exceptions to the registered handler (rever…

c57c658

…t behavior change)

rename failed to ended

aeefb69

rename elsewhere + dedup code

1ea9902

vietj reviewed Apr 15, 2026

View reviewed changes

review

7fcdb6f

vietj reviewed Apr 15, 2026

View reviewed changes

snazy added 2 commits April 15, 2026 14:39

update handleException() behavior

9a3a0ed

Promise/Future, trailers safety

0bbc30b

vietj reviewed Apr 15, 2026

View reviewed changes

Comment thread vertx-core/src/main/java/io/vertx/core/http/impl/HttpClientResponseImpl.java

vietj reviewed Apr 15, 2026

View reviewed changes

snazy added 3 commits April 16, 2026 10:27

Remove unneccessary defensive check

206ba33

Comment

dd8b903

Remove completionFuture().isComplete() in handleTrailers()

2e982ea

vietj reviewed Apr 16, 2026

View reviewed changes

Let checkEnded check for completed (not succeeded)

321ab69

Conversation

snazy commented Apr 15, 2026

Uh oh!

snazy commented Apr 15, 2026

Uh oh!

vietj commented Apr 15, 2026

Uh oh!

snazy commented Apr 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants