From f2271ce97f7bff12cfde5176b299459493b48574 Mon Sep 17 00:00:00 2001 From: Jihun Kim Date: Tue, 28 Apr 2026 13:19:08 +0900 Subject: [PATCH 1/3] Fix multipart decoder cleanup on abnormal termination --- .../core/http/impl/HttpServerRequestImpl.java | 59 +++++++++++++++---- .../http/impl/http1/Http1ServerRequest.java | 44 ++++++++++---- .../Http1xServerFileUploadTest.java | 5 ++ .../fileupload/Http2ServerFileUploadTest.java | 6 ++ .../fileupload/HttpServerFileUploadTest.java | 53 +++++++++++++++++ 5 files changed, 146 insertions(+), 21 deletions(-) diff --git a/vertx-core/src/main/java/io/vertx/core/http/impl/HttpServerRequestImpl.java b/vertx-core/src/main/java/io/vertx/core/http/impl/HttpServerRequestImpl.java index 604b52998b0..9ee4ed6df3c 100644 --- a/vertx-core/src/main/java/io/vertx/core/http/impl/HttpServerRequestImpl.java +++ b/vertx-core/src/main/java/io/vertx/core/http/impl/HttpServerRequestImpl.java @@ -141,17 +141,31 @@ public void handleHeaders(HttpRequestHead headers) { private void handleException(Throwable cause) { boolean notify; + HttpPostRequestDecoder decoderToCleanup; synchronized (connection) { notify = !ended; + decoderToCleanup = notify ? postRequestDecoder : null; } - if (notify) { - notifyException(cause); + try { + if (notify) { + notifyException(cause); + } + response.handleException(cause); + } finally { + cleanupPostRequestDecoder(decoderToCleanup); } - response.handleException(cause); } private void handleClosed(Void v) { - response.handleClose(v); + HttpPostRequestDecoder decoderToCleanup; + synchronized (connection) { + decoderToCleanup = postRequestDecoder; + } + try { + response.handleClose(v); + } finally { + cleanupPostRequestDecoder(decoderToCleanup); + } } private void notifyException(Throwable failure) { @@ -178,8 +192,7 @@ public void handleData(Buffer data) { } catch (HttpPostRequestDecoder.ErrorDataDecoderException | HttpPostRequestDecoder.TooLongFormFieldException | HttpPostRequestDecoder.TooManyFormFieldsException e) { - postRequestDecoder.destroy(); - postRequestDecoder = null; + cleanupPostRequestDecoder(); handleException(e); } } @@ -215,8 +228,7 @@ public void handleTrailers(MultiMap trailers) { } catch (Exception e) { handleException(e); } finally { - postRequestDecoder.destroy(); - postRequestDecoder = null; + cleanupPostRequestDecoder(); } } handler = eventHandler; @@ -228,13 +240,38 @@ public void handleTrailers(MultiMap trailers) { public void handleReset(long errorCode) { boolean notify; + HttpPostRequestDecoder decoderToCleanup; synchronized (connection) { notify = !ended; + decoderToCleanup = postRequestDecoder; + } + try { + if (notify) { + notifyException(new StreamResetException(errorCode)); + } + response.handleReset(errorCode); + } finally { + cleanupPostRequestDecoder(decoderToCleanup); } - if (notify) { - notifyException(new StreamResetException(errorCode)); + } + + private void cleanupPostRequestDecoder() { + HttpPostRequestDecoder decoderToCleanup; + synchronized (connection) { + decoderToCleanup = postRequestDecoder; + } + cleanupPostRequestDecoder(decoderToCleanup); + } + + private void cleanupPostRequestDecoder(HttpPostRequestDecoder decoderToCleanup) { + if (decoderToCleanup != null) { + synchronized (connection) { + if (postRequestDecoder == decoderToCleanup) { + postRequestDecoder = null; + } + } + decoderToCleanup.destroy(); } - response.handleReset(errorCode); } private void checkEnded() { diff --git a/vertx-core/src/main/java/io/vertx/core/http/impl/http1/Http1ServerRequest.java b/vertx-core/src/main/java/io/vertx/core/http/impl/http1/Http1ServerRequest.java index c815278e2c3..dc69416b150 100644 --- a/vertx-core/src/main/java/io/vertx/core/http/impl/http1/Http1ServerRequest.java +++ b/vertx-core/src/main/java/io/vertx/core/http/impl/http1/Http1ServerRequest.java @@ -567,8 +567,7 @@ private void onData(Buffer data) { } catch (HttpPostRequestDecoder.ErrorDataDecoderException | HttpPostRequestDecoder.TooLongFormFieldException | HttpPostRequestDecoder.TooManyFormFieldsException e) { - decoder.destroy(); - decoder = null; + cleanupDecoder(); handleException(e); } } @@ -628,6 +627,7 @@ private void endDecode() { } catch (Exception e) { // Will never happen, anyway handle it somehow just in case handleException(e); + return; } finally { attr.release(); } @@ -640,8 +640,7 @@ private void endDecode() { } catch (HttpPostRequestDecoder.EndOfDataDecoderException e) { // ignore this as it is expected } finally { - decoder.destroy(); - decoder = null; + cleanupDecoder(); } } @@ -649,11 +648,13 @@ void handleException(Throwable t) { HttpEventHandler handler = null; Http1ServerResponse resp = null; InterfaceHttpData upload = null; + HttpPostRequestDecoder decoderToCleanup = null; synchronized (conn) { if (!isEnded()) { handler = eventHandler; if (decoder != null) { upload = decoder.currentPartialHttpData(); + decoderToCleanup = decoder; } } if (!response.ended()) { @@ -663,14 +664,37 @@ void handleException(Throwable t) { resp = response; } } - if (resp != null) { - resp.handleException(t); + try { + if (resp != null) { + resp.handleException(t); + } + if (upload instanceof NettyFileUpload) { + ((NettyFileUpload) upload).handleException(t); + } + if (handler != null) { + handler.handleException(t); + } + } finally { + cleanupDecoder(decoderToCleanup); } - if (upload instanceof NettyFileUpload) { - ((NettyFileUpload) upload).handleException(t); + } + + private void cleanupDecoder() { + HttpPostRequestDecoder decoderToCleanup; + synchronized (conn) { + decoderToCleanup = decoder; } - if (handler != null) { - handler.handleException(t); + cleanupDecoder(decoderToCleanup); + } + + private void cleanupDecoder(HttpPostRequestDecoder decoderToCleanup) { + if (decoderToCleanup != null) { + synchronized (conn) { + if (decoder == decoderToCleanup) { + decoder = null; + } + } + decoderToCleanup.destroy(); } } diff --git a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http1xServerFileUploadTest.java b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http1xServerFileUploadTest.java index 62bd31eee7b..38573dcd4ed 100644 --- a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http1xServerFileUploadTest.java +++ b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http1xServerFileUploadTest.java @@ -22,6 +22,11 @@ public Http1xServerFileUploadTest() { super(HttpConfig.Http1x.DEFAULT); } + @Test + public void testClientResetMultipartUploadCleansDecoder() throws Exception { + super.testClientResetMultipartUploadCleansDecoder(); + } + @Ignore @Test @Override diff --git a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http2ServerFileUploadTest.java b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http2ServerFileUploadTest.java index cde04b491e3..1984d0c40ce 100644 --- a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http2ServerFileUploadTest.java +++ b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http2ServerFileUploadTest.java @@ -11,6 +11,7 @@ package io.vertx.tests.http.fileupload; import io.vertx.test.http.HttpConfig; +import org.junit.Test; /** */ @@ -23,4 +24,9 @@ public Http2ServerFileUploadTest() { protected Http2ServerFileUploadTest(boolean multiplex) { super(new HttpConfig.H2(multiplex)); } + + @Test + public void testClientResetMultipartUploadCleansDecoder() throws Exception { + super.testClientResetMultipartUploadCleansDecoder(); + } } diff --git a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java index 8e66bade557..c272cc27c39 100644 --- a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java +++ b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java @@ -33,6 +33,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.UUID; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BooleanSupplier; @@ -56,6 +57,58 @@ public void setUp() throws Exception { testDir = testFolder.newFolder(); } + protected void testClientResetMultipartUploadCleansDecoder() throws Exception { + AtomicReference clientRequest = new AtomicReference<>(); + AtomicBoolean completed = new AtomicBoolean(); + server.requestHandler(req -> { + if (req.method() == HttpMethod.POST) { + Context context = Vertx.currentContext(); + req.setExpectMultipart(true); + req.exceptionHandler(err -> context.runOnContext(v -> { + if (completed.compareAndSet(false, true)) { + req.headers().set(HttpHeaders.CONTENT_TYPE, "text/plain"); + try { + req.setExpectMultipart(true); + Assert.fail("Should throw IllegalStateException"); + } catch (IllegalStateException e) { + Assert.assertTrue(e.getMessage(), e.getMessage().contains("valid content-type")); + } + testComplete(); + } + })); + req.uploadHandler(upload -> { + upload.handler(buffer -> { + }); + HttpClientRequest request = clientRequest.get(); + if (request != null) { + request.reset(); + } + }); + } + }); + startServer(testAddress); + + String boundary = "multipart-cleanup"; + String body = "--" + boundary + "\r\n" + + "Content-Disposition: form-data; name=\"file\"; filename=\"tmp-0.txt\"\r\n" + + "Content-Type: text/plain\r\n" + + "\r\n" + + "content"; + + client.request(new RequestOptions(requestOptions) + .setMethod(HttpMethod.POST) + .setURI("/form")).onComplete(TestUtils.onSuccess(request -> { + clientRequest.set(request); + request + .putHeader(HttpHeaders.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary) + .putHeader(HttpHeaders.CONTENT_LENGTH, "999999") + .response().onComplete(ar -> { + }); + request.write(body); + })); + await(); + } + @Test public void testFormUploadEmptyFile() { testFormUploadFile("", false, false, false, false); From adea3e5c33dac7203cdb77c524be7a0e4298d5e0 Mon Sep 17 00:00:00 2001 From: Jihun Kim Date: Fri, 1 May 2026 01:29:12 +0900 Subject: [PATCH 2/3] Refine multipart decoder cleanup --- .../core/http/impl/http1/Http1ServerRequest.java | 16 ++++++++-------- .../fileupload/Http1xServerFileUploadTest.java | 5 ----- .../fileupload/Http2ServerFileUploadTest.java | 6 ------ .../fileupload/HttpServerFileUploadTest.java | 11 ++++++----- 4 files changed, 14 insertions(+), 24 deletions(-) diff --git a/vertx-core/src/main/java/io/vertx/core/http/impl/http1/Http1ServerRequest.java b/vertx-core/src/main/java/io/vertx/core/http/impl/http1/Http1ServerRequest.java index dc69416b150..7626a3e353d 100644 --- a/vertx-core/src/main/java/io/vertx/core/http/impl/http1/Http1ServerRequest.java +++ b/vertx-core/src/main/java/io/vertx/core/http/impl/http1/Http1ServerRequest.java @@ -616,9 +616,10 @@ private void reportRequestBegin() { } private void endDecode() { + Exception failure = null; try { decoder.offer(LastHttpContent.EMPTY_LAST_CONTENT); - while (decoder.hasNext()) { + while (failure == null && decoder.hasNext()) { InterfaceHttpData data = decoder.next(); if (data instanceof Attribute) { Attribute attr = (Attribute) data; @@ -626,13 +627,15 @@ private void endDecode() { attributes().add(attr.getName(), attr.getValue()); } catch (Exception e) { // Will never happen, anyway handle it somehow just in case - handleException(e); - return; + failure = e; } finally { attr.release(); } } } + if (failure != null) { + handleException(failure); + } } catch (HttpPostRequestDecoder.ErrorDataDecoderException | HttpPostRequestDecoder.TooLongFormFieldException | HttpPostRequestDecoder.TooManyFormFieldsException e) { @@ -655,6 +658,7 @@ void handleException(Throwable t) { if (decoder != null) { upload = decoder.currentPartialHttpData(); decoderToCleanup = decoder; + decoder = null; } } if (!response.ended()) { @@ -683,17 +687,13 @@ private void cleanupDecoder() { HttpPostRequestDecoder decoderToCleanup; synchronized (conn) { decoderToCleanup = decoder; + decoder = null; } cleanupDecoder(decoderToCleanup); } private void cleanupDecoder(HttpPostRequestDecoder decoderToCleanup) { if (decoderToCleanup != null) { - synchronized (conn) { - if (decoder == decoderToCleanup) { - decoder = null; - } - } decoderToCleanup.destroy(); } } diff --git a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http1xServerFileUploadTest.java b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http1xServerFileUploadTest.java index 38573dcd4ed..62bd31eee7b 100644 --- a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http1xServerFileUploadTest.java +++ b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http1xServerFileUploadTest.java @@ -22,11 +22,6 @@ public Http1xServerFileUploadTest() { super(HttpConfig.Http1x.DEFAULT); } - @Test - public void testClientResetMultipartUploadCleansDecoder() throws Exception { - super.testClientResetMultipartUploadCleansDecoder(); - } - @Ignore @Test @Override diff --git a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http2ServerFileUploadTest.java b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http2ServerFileUploadTest.java index 1984d0c40ce..cde04b491e3 100644 --- a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http2ServerFileUploadTest.java +++ b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/Http2ServerFileUploadTest.java @@ -11,7 +11,6 @@ package io.vertx.tests.http.fileupload; import io.vertx.test.http.HttpConfig; -import org.junit.Test; /** */ @@ -24,9 +23,4 @@ public Http2ServerFileUploadTest() { protected Http2ServerFileUploadTest(boolean multiplex) { super(new HttpConfig.H2(multiplex)); } - - @Test - public void testClientResetMultipartUploadCleansDecoder() throws Exception { - super.testClientResetMultipartUploadCleansDecoder(); - } } diff --git a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java index c272cc27c39..35e4ce46be8 100644 --- a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java +++ b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java @@ -21,6 +21,7 @@ import io.vertx.test.http.HttpConfig; import io.vertx.test.http.HttpTestBase; import io.vertx.test.http.SimpleHttpTest; +import org.junit.Assume; import org.junit.Rule; import org.junit.Assert; import org.junit.Test; @@ -57,7 +58,9 @@ public void setUp() throws Exception { testDir = testFolder.newFolder(); } - protected void testClientResetMultipartUploadCleansDecoder() throws Exception { + @Test + public void testClientResetMultipartUploadCleansDecoder() throws Exception { + Assume.assumeTrue(config.version() == HttpVersion.HTTP_1_1 || config.version() == HttpVersion.HTTP_2); AtomicReference clientRequest = new AtomicReference<>(); AtomicBoolean completed = new AtomicBoolean(); server.requestHandler(req -> { @@ -77,8 +80,7 @@ protected void testClientResetMultipartUploadCleansDecoder() throws Exception { } })); req.uploadHandler(upload -> { - upload.handler(buffer -> { - }); + upload.handler(buffer -> {}); HttpClientRequest request = clientRequest.get(); if (request != null) { request.reset(); @@ -102,8 +104,7 @@ protected void testClientResetMultipartUploadCleansDecoder() throws Exception { request .putHeader(HttpHeaders.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary) .putHeader(HttpHeaders.CONTENT_LENGTH, "999999") - .response().onComplete(ar -> { - }); + .response().onComplete(ar -> {}); request.write(body); })); await(); From 11dde2153ead6e0b5999d25e7e5ea344c4f880ca Mon Sep 17 00:00:00 2001 From: Jihun Kim Date: Tue, 5 May 2026 00:53:23 +0900 Subject: [PATCH 3/3] Clarify multipart cleanup test sequencing --- .../http/fileupload/HttpServerFileUploadTest.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java index 35e4ce46be8..534bd434da6 100644 --- a/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java +++ b/vertx-core/src/test/java/io/vertx/tests/http/fileupload/HttpServerFileUploadTest.java @@ -67,8 +67,12 @@ public void testClientResetMultipartUploadCleansDecoder() throws Exception { if (req.method() == HttpMethod.POST) { Context context = Vertx.currentContext(); req.setExpectMultipart(true); - req.exceptionHandler(err -> context.runOnContext(v -> { - if (completed.compareAndSet(false, true)) { + req.exceptionHandler(err -> { + if (!completed.compareAndSet(false, true)) { + return; + } + // Defer until handleException cleanup runs. + context.runOnContext(v -> { req.headers().set(HttpHeaders.CONTENT_TYPE, "text/plain"); try { req.setExpectMultipart(true); @@ -77,8 +81,8 @@ public void testClientResetMultipartUploadCleansDecoder() throws Exception { Assert.assertTrue(e.getMessage(), e.getMessage().contains("valid content-type")); } testComplete(); - } - })); + }); + }); req.uploadHandler(upload -> { upload.handler(buffer -> {}); HttpClientRequest request = clientRequest.get();