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..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 @@ -567,8 +567,7 @@ private void onData(Buffer data) { } catch (HttpPostRequestDecoder.ErrorDataDecoderException | HttpPostRequestDecoder.TooLongFormFieldException | HttpPostRequestDecoder.TooManyFormFieldsException e) { - decoder.destroy(); - decoder = null; + cleanupDecoder(); handleException(e); } } @@ -617,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; @@ -627,12 +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); + failure = e; } finally { attr.release(); } } } + if (failure != null) { + handleException(failure); + } } catch (HttpPostRequestDecoder.ErrorDataDecoderException | HttpPostRequestDecoder.TooLongFormFieldException | HttpPostRequestDecoder.TooManyFormFieldsException e) { @@ -640,8 +643,7 @@ private void endDecode() { } catch (HttpPostRequestDecoder.EndOfDataDecoderException e) { // ignore this as it is expected } finally { - decoder.destroy(); - decoder = null; + cleanupDecoder(); } } @@ -649,11 +651,14 @@ 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; + decoder = null; } } if (!response.ended()) { @@ -663,14 +668,33 @@ 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; + decoder = null; } - if (handler != null) { - handler.handleException(t); + cleanupDecoder(decoderToCleanup); + } + + private void cleanupDecoder(HttpPostRequestDecoder decoderToCleanup) { + if (decoderToCleanup != null) { + decoderToCleanup.destroy(); } } 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..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 @@ -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; @@ -33,6 +34,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 +58,62 @@ public void setUp() throws Exception { testDir = testFolder.newFolder(); } + @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 -> { + if (req.method() == HttpMethod.POST) { + Context context = Vertx.currentContext(); + req.setExpectMultipart(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); + 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);