From 2d25ff2336526aa0f42eb758c031ff495c72be0b Mon Sep 17 00:00:00 2001 From: Tomas Langer Date: Wed, 21 Dec 2022 16:50:22 +0100 Subject: [PATCH] Move reset to RoutingResponse Introduce commit to RoutingResponse Correctly terminate HTTP/2 streams when an error is thrown Update tests with try with resources --- .../microprofile/server/JaxRsService.java | 35 +++- .../http2/webserver/Http2ServerResponse.java | 41 +++-- .../nima/http2/webserver/Http2Stream.java | 8 +- ...ttp2ErrorHandlingWithOutputStreamTest.java | 151 +++++++++--------- .../test/resources/logging-test.properties | 2 +- .../ErrorHandlingWithOutputStreamTest.java | 61 ++++--- .../nima/webserver/http/ErrorHandlers.java | 24 ++- .../nima/webserver/http/HttpRouting.java | 2 + .../nima/webserver/http/RoutingResponse.java | 15 ++ .../nima/webserver/http/ServerResponse.java | 5 - .../webserver/http1/Http1ServerResponse.java | 35 ++-- .../webserver/http/ErrorHandlersTest.java | 11 +- .../apps/bookstore/mp/BookResourceTest.java | 2 - 13 files changed, 250 insertions(+), 142 deletions(-) diff --git a/microprofile/server/src/main/java/io/helidon/microprofile/server/JaxRsService.java b/microprofile/server/src/main/java/io/helidon/microprofile/server/JaxRsService.java index 527d44fcb62..e003b25985c 100644 --- a/microprofile/server/src/main/java/io/helidon/microprofile/server/JaxRsService.java +++ b/microprofile/server/src/main/java/io/helidon/microprofile/server/JaxRsService.java @@ -330,7 +330,7 @@ public OutputStream writeResponseStatusAndHeaders(long contentLengthParam, if (contentLength > 0) { res.header(Header.create(Header.CONTENT_LENGTH, String.valueOf(contentLength))); } - this.outputStream = res.outputStream(); + this.outputStream = new NoFlushOutputStream(res.outputStream()); return outputStream; } @@ -386,4 +386,37 @@ public void await() { } } } + + private static class NoFlushOutputStream extends OutputStream { + private final OutputStream delegate; + + private NoFlushOutputStream(OutputStream delegate) { + this.delegate = delegate; + } + + @Override + public void write(byte[] b) throws IOException { + delegate.write(b); + } + + @Override + public void write(byte[] b, int off, int len) throws IOException { + delegate.write(b, off, len); + } + + @Override + public void flush() { + // intentional no-op, flush did not work nicely with Jersey + } + + @Override + public void close() throws IOException { + delegate.close(); + } + + @Override + public void write(int b) throws IOException { + delegate.write(b); + } + } } diff --git a/nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2ServerResponse.java b/nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2ServerResponse.java index 79bf27b4b4d..442916a2cdb 100644 --- a/nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2ServerResponse.java +++ b/nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2ServerResponse.java @@ -118,17 +118,6 @@ public boolean isSent() { return isSent; } - @Override - public boolean reset() { - if (isSent) { - return false; - } - headers.clear(); - streamingEntity = false; - outputStream = null; - return true; - } - @Override public OutputStream outputStream() { if (isSent) { @@ -166,6 +155,24 @@ public boolean hasEntity() { return isSent || streamingEntity; } + @Override + public boolean reset() { + if (isSent || outputStream != null && outputStream.bytesWritten > 0) { + return false; + } + headers.clear(); + streamingEntity = false; + outputStream = null; + return true; + } + + @Override + public void commit() { + if (outputStream != null) { + outputStream.commit(); + } + } + private static class BlockingOutputStream extends OutputStream { private final ServerResponseHeaders headers; @@ -210,8 +217,19 @@ public void write(byte[] b, int off, int len) throws IOException { write(BufferData.create(b, off, len)); } + @Override + public void flush() throws IOException { + if (firstByte && firstBuffer != null) { + write(BufferData.empty()); + } + } + @Override public void close() { + // does nothing, we expect commit(), so we can reset response when no bytes were written to response + } + + void commit() { if (closed) { return; } @@ -292,6 +310,7 @@ private void sendHeadersAndPrepare() { headers.setIfAbsent(Header.create(Header.DATE, true, false, Http.DateTime.rfc1123String())); Http2Headers http2Headers = Http2Headers.create(headers); + http2Headers.status(status); http2Headers.validateResponse(); int written = writer.writeHeaders(http2Headers, streamId, diff --git a/nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2Stream.java b/nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2Stream.java index b8c94ba33b0..ffa90b1d21e 100644 --- a/nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2Stream.java +++ b/nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2Stream.java @@ -258,10 +258,10 @@ public void run() { + ctx.childSocketId() + " ] - " + streamId); try { handle(); - } catch (SocketWriterException e) { - throw new CloseConnectionException(e.getMessage(), e); - } catch (CloseConnectionException | UncheckedIOException e) { - throw e; + } catch (SocketWriterException | CloseConnectionException | UncheckedIOException e) { + Http2RstStream rst = new Http2RstStream(Http2ErrorCode.STREAM_CLOSED); + writer.write(rst.toFrameData(serverSettings, streamId, Http2Flag.NoFlags.create()), flowControl); + // no sense in throwing an exception, as this is invoked from an executor service directly } catch (RequestException e) { DirectHandler handler = ctx.directHandlers().handler(e.eventType()); DirectHandler.TransportResponse response = handler.handle(e.request(), diff --git a/nima/tests/integration/http2/server/src/test/java/io/helidon/nima/tests/integration/http2/webserver/Http2ErrorHandlingWithOutputStreamTest.java b/nima/tests/integration/http2/server/src/test/java/io/helidon/nima/tests/integration/http2/webserver/Http2ErrorHandlingWithOutputStreamTest.java index 7d5b7683855..3d643f59c3b 100644 --- a/nima/tests/integration/http2/server/src/test/java/io/helidon/nima/tests/integration/http2/webserver/Http2ErrorHandlingWithOutputStreamTest.java +++ b/nima/tests/integration/http2/server/src/test/java/io/helidon/nima/tests/integration/http2/webserver/Http2ErrorHandlingWithOutputStreamTest.java @@ -16,6 +16,16 @@ package io.helidon.nima.tests.integration.http2.webserver; +import java.io.IOException; +import java.io.OutputStream; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.util.Optional; + import io.helidon.common.configurable.Resource; import io.helidon.common.http.Http; import io.helidon.common.pki.KeyConfig; @@ -29,41 +39,38 @@ import io.helidon.nima.webserver.http.HttpRouting; import io.helidon.nima.webserver.http.ServerRequest; import io.helidon.nima.webserver.http.ServerResponse; + import org.hamcrest.BaseMatcher; import org.hamcrest.Description; import org.hamcrest.Matcher; import org.junit.jupiter.api.Test; -import java.io.OutputStream; -import java.net.URI; -import java.net.http.HttpClient; -import java.net.http.HttpRequest; -import java.net.http.HttpResponse; -import java.nio.charset.StandardCharsets; -import java.time.Duration; -import java.util.Optional; - import static io.helidon.common.http.Http.Method.GET; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; @ServerTest class Http2ErrorHandlingWithOutputStreamTest { private static final Http.HeaderName MAIN_HEADER_NAME = Http.Header.create("main-handler"); private static final Http.HeaderName ERROR_HEADER_NAME = Http.Header.create("error-handler"); - + private static HttpClient httpClient; private final int plainPort; private final int tlsPort; - private static HttpClient httpClient; - Http2ErrorHandlingWithOutputStreamTest(WebServer server) { this.plainPort = server.port(); this.tlsPort = server.port("https"); } + public static Matcher> emptyOptional() { + return new EmptyOptionalMatcher<>(); + } + @SetUpServer static void setUpServer(WebServer.Builder serverBuilder) { KeyConfig privateKeyConfig = KeyConfig.keystoreBuilder() @@ -77,7 +84,7 @@ static void setUpServer(WebServer.Builder serverBuilder) { .build(); serverBuilder.socket("https", - socketBuilder -> socketBuilder.tls(tls)); + socketBuilder -> socketBuilder.tls(tls)); httpClient = http2Client(); } @@ -100,7 +107,6 @@ static void router(HttpRouting.Builder router) { })) .route(Http2Route.route(GET, "get-outputStream-writeTwiceThenError", (req, res) -> { res.status(Http.Status.OK_200); - res.header(Http.Header.create("status"), "200"); res.header(MAIN_HEADER_NAME, "x"); OutputStream os = res.outputStream(); os.write("writeOnce".getBytes(StandardCharsets.UTF_8)); @@ -115,31 +121,16 @@ static void router(HttpRouting.Builder router) { os.flush(); throw new CustomException(); })) - .route(Http2Route.route(GET, "", ((req, res) -> - res.send("ok")))); - } - - private static HttpClient http2Client() { - return HttpClient.newBuilder() - .version(HttpClient.Version.HTTP_2) - .connectTimeout(Duration.ofSeconds(30)) - .build(); - } - - private HttpResponse request(String uriSuffix) { - try { - return httpClient.send(httpRequest(uriSuffix), HttpResponse.BodyHandlers.ofString()); - } catch (Exception e) { - throw new RuntimeException(e); - } - } - - private HttpRequest httpRequest(String uriSuffix) { - return HttpRequest.newBuilder() - .timeout(Duration.ofSeconds(30)) - .uri(URI.create("http://localhost:" + plainPort + uriSuffix)) - .GET() - .build(); + .get("get-outputStream-tryWithResources", (req, res) -> { + res.header(MAIN_HEADER_NAME, "x"); + try (OutputStream os = res.outputStream()) { + os.write("This should not be sent immediately".getBytes(StandardCharsets.UTF_8)); + throw new CustomException(); + } + }) + .route(Http2Route.route(GET, "", ( + (req, res) -> + res.send("ok")))); } @Test @@ -160,7 +151,6 @@ void testGetOutputStreamThenError_expect_CustomErrorHandlerMessage() { assertThat(response.headers().firstValue(MAIN_HEADER_NAME.lowerCase()), is(emptyOptional())); } - @Test void testGetOutputStreamWriteOnceThenError_expect_CustomErrorHandlerMessage() { var response = request("/get-outputStream-writeOnceThenError"); @@ -171,35 +161,54 @@ void testGetOutputStreamWriteOnceThenError_expect_CustomErrorHandlerMessage() { assertThat(response.headers().firstValue(MAIN_HEADER_NAME.lowerCase()), is(emptyOptional())); } -// ------------------ -// This test hangs -// ------------------ -// @Test -// void testGetOutputStreamWriteTwiceThenError_expect_invalidResponse() { -// try { -// var response = request("/get-outputStream-writeTwiceThenError"); -// assertEquals(500, response.statusCode()); -// //String body = response.body(); -// } catch (Exception e) { -// System.err.println("never get here ---------------"); -// e.printStackTrace(); -// } -// } - -// ------------------ -// This test hangs -// ------------------ -// @Test -// void testGetOutputStreamWriteFlushThenError_expect_invalidResponse() { -// try { -// var response = request("/get-outputStream-writeFlushThenError"); -// assertEquals(500, response.statusCode()); -// //String body = response.body(); -// } catch (Exception e) { -// System.err.println("never get here ---------------"); -// e.printStackTrace(); -// } -// } + @Test + void testGetOutputStreamWriteTwiceThenError_expect_invalidResponse() { + RuntimeException r = assertThrows(RuntimeException.class, () -> request("/get-outputStream-writeTwiceThenError")); + assertThat(r.getCause(), instanceOf(IOException.class)); + // stream should have been reset during processing + assertThat(r.getMessage(), containsString("RST_STREAM")); + } + + @Test + void testGetOutputStreamWriteFlushThenError_expect_invalidResponse() { + RuntimeException r = assertThrows(RuntimeException.class, () -> request("/get-outputStream-writeFlushThenError")); + assertThat(r.getCause(), instanceOf(IOException.class)); + // stream should have been reset during processing + assertThat(r.getMessage(), containsString("RST_STREAM")); + } + + @Test + void testGetOutputStreamTryWithResourcesThenError_expect_CustomErrorHandlerMessage() { + var response = request("/get-outputStream-tryWithResources"); + + assertEquals(418, response.statusCode()); + assertThat(response.body(), is("TeaPotIAm")); + assertThat(response.headers().firstValue(ERROR_HEADER_NAME.lowerCase()), is(Optional.of("err"))); + assertThat(response.headers().firstValue(MAIN_HEADER_NAME.lowerCase()), is(emptyOptional())); + } + + private static HttpClient http2Client() { + return HttpClient.newBuilder() + .version(HttpClient.Version.HTTP_2) + .connectTimeout(Duration.ofSeconds(30)) + .build(); + } + + private HttpResponse request(String uriSuffix) { + try { + return httpClient.send(httpRequest(uriSuffix), HttpResponse.BodyHandlers.ofString()); + } catch (Exception e) { + throw new RuntimeException(e); + } + } + + private HttpRequest httpRequest(String uriSuffix) { + return HttpRequest.newBuilder() + .timeout(Duration.ofSeconds(30)) + .uri(URI.create("http://localhost:" + plainPort + uriSuffix)) + .GET() + .build(); + } private static class CustomRoutingHandler implements ErrorHandler { @Override @@ -214,10 +223,6 @@ private static class CustomException extends RuntimeException { } - public static Matcher> emptyOptional() { - return new EmptyOptionalMatcher<>(); - } - static class EmptyOptionalMatcher extends BaseMatcher> { private Optional optionalActual; diff --git a/nima/tests/integration/http2/server/src/test/resources/logging-test.properties b/nima/tests/integration/http2/server/src/test/resources/logging-test.properties index 7c489a00289..1a57678ee5e 100644 --- a/nima/tests/integration/http2/server/src/test/resources/logging-test.properties +++ b/nima/tests/integration/http2/server/src/test/resources/logging-test.properties @@ -18,4 +18,4 @@ java.util.logging.ConsoleHandler.level=FINEST java.util.logging.SimpleFormatter.format=%1$tY.%1$tm.%1$td %1$tH:%1$tM:%1$tS.%1$tL %5$s%6$s%n # Global logging level. Can be overridden by specific loggers .level=INFO -io.helidon.nima.level=FINEST +io.helidon.nima.level=INFO diff --git a/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ErrorHandlingWithOutputStreamTest.java b/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ErrorHandlingWithOutputStreamTest.java index 2ee81499680..337d5f76745 100644 --- a/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ErrorHandlingWithOutputStreamTest.java +++ b/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ErrorHandlingWithOutputStreamTest.java @@ -16,8 +16,10 @@ package io.helidon.nima.tests.integration.server; +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; + import io.helidon.common.buffers.DataReader; -import io.helidon.common.http.Headers; import io.helidon.common.http.Http; import io.helidon.nima.testing.junit5.webserver.ServerTest; import io.helidon.nima.testing.junit5.webserver.SetUpRoute; @@ -27,10 +29,8 @@ import io.helidon.nima.webserver.http.HttpRouting; import io.helidon.nima.webserver.http.ServerRequest; import io.helidon.nima.webserver.http.ServerResponse; -import org.junit.jupiter.api.Test; -import java.io.OutputStream; -import java.nio.charset.StandardCharsets; +import org.junit.jupiter.api.Test; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; @@ -76,35 +76,42 @@ static void router(HttpRouting.Builder router) { os.flush(); throw new CustomException(); }) + .get("get-outputStream-tryWithResources", (req, res) -> { + res.header(MAIN_HEADER_NAME, "x"); + try (OutputStream os = res.outputStream()) { + os.write("This should not be sent immediately".getBytes(StandardCharsets.UTF_8)); + throw new CustomException(); + } + }) .get((req, res) -> res.send("ok")); } @Test void testOk() { - var response = client.get().request(); - - assertThat(response.status(), is(Http.Status.OK_200)); - assertThat(response.entity().as(String.class), is("ok")); + try (var response = client.get().request()) { + assertThat(response.status(), is(Http.Status.OK_200)); + assertThat(response.entity().as(String.class), is("ok")); + } } @Test void testGetOutputStreamThenError_expect_CustomErrorHandlerMessage() { - var response = client.get("/get-outputStream").request(); - - assertThat(response.status(), is(Http.Status.I_AM_A_TEAPOT_418)); - assertThat(response.entity().as(String.class), is("CustomErrorContent")); - assertThat(response.headers().contains(ERROR_HEADER_NAME), is(true)); - assertThat(response.headers().contains(MAIN_HEADER_NAME), is(false)); + try (var response = client.get("/get-outputStream").request()) { + assertThat(response.status(), is(Http.Status.I_AM_A_TEAPOT_418)); + assertThat(response.entity().as(String.class), is("CustomErrorContent")); + assertThat(response.headers().contains(ERROR_HEADER_NAME), is(true)); + assertThat(response.headers().contains(MAIN_HEADER_NAME), is(false)); + } } @Test void testGetOutputStreamWriteOnceThenError_expect_CustomErrorHandlerMessage() { - var response = client.get("/get-outputStream-writeOnceThenError").request(); - - assertThat(response.status(), is(Http.Status.I_AM_A_TEAPOT_418)); - assertThat(response.entity().as(String.class), is("CustomErrorContent")); - assertThat(response.headers().contains(ERROR_HEADER_NAME), is(true)); - assertThat(response.headers().contains(MAIN_HEADER_NAME), is(false)); + try (var response = client.get("/get-outputStream-writeOnceThenError").request()) { + assertThat(response.status(), is(Http.Status.I_AM_A_TEAPOT_418)); + assertThat(response.entity().as(String.class), is("CustomErrorContent")); + assertThat(response.headers().contains(ERROR_HEADER_NAME), is(true)); + assertThat(response.headers().contains(MAIN_HEADER_NAME), is(false)); + } } @Test @@ -119,7 +126,6 @@ void testGetOutputStreamWriteTwiceThenError_expect_invalidResponse() { @Test void testGetOutputStreamWriteFlushThenError_expect_invalidResponse() { - Headers headers; try (Http1ClientResponse response = client.method(Http.Method.GET) .uri("/get-outputStream-writeFlushThenError") .request()) { @@ -129,6 +135,19 @@ void testGetOutputStreamWriteFlushThenError_expect_invalidResponse() { } } + @Test + void testGetOutputStreamTryWithResourcesThenError_expect_CustomErrorHandlerMessage() { + try (Http1ClientResponse response = client.method(Http.Method.GET) + .uri("/get-outputStream-tryWithResources") + .request()) { + + assertThat(response.status(), is(Http.Status.I_AM_A_TEAPOT_418)); + assertThat(response.entity().as(String.class), is("CustomErrorContent")); + assertThat(response.headers().contains(ERROR_HEADER_NAME), is(true)); + assertThat(response.headers().contains(MAIN_HEADER_NAME), is(false)); + } + } + private static class CustomRoutingHandler implements ErrorHandler { @Override public void handle(ServerRequest req, ServerResponse res, CustomException throwable) { diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/ErrorHandlers.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/ErrorHandlers.java index 06b186ad0cd..7c3297bccdf 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/ErrorHandlers.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/ErrorHandlers.java @@ -66,9 +66,15 @@ public String toString() { * @param response HTTP server response * @param task task to execute */ - public void runWithErrorHandling(ConnectionContext ctx, ServerRequest request, ServerResponse response, Callable task) { + public void runWithErrorHandling(ConnectionContext ctx, + RoutingRequest request, + RoutingResponse response, + Callable task) { try { task.call(); + if (response.hasEntity()) { + response.commit(); + } } catch (CloseConnectionException | UncheckedIOException e) { // these errors must "bubble up" throw e; @@ -139,11 +145,13 @@ Optional> errorHandler(Class exceptionC private void handleRequestException(ConnectionContext ctx, ServerRequest request, - ServerResponse response, + RoutingResponse response, RequestException e) { - if (response.isSent()) { + if (!response.reset()) { ctx.log(LOGGER, System.Logger.Level.WARNING, "Request failed: " + request.prologue() + ", cannot send error response, as response already sent", e); + throw new CloseConnectionException( + "Cannot send response of an error handler, status and headers already written"); } boolean keepAlive = e.keepAlive(); if (keepAlive && !request.content().consumed()) { @@ -155,15 +163,16 @@ private void handleRequestException(ConnectionContext ctx, } } ctx.directHandlers().handle(e, response, keepAlive); + response.commit(); } - private void handleError(ConnectionContext ctx, ServerRequest request, ServerResponse response, Throwable e) { + private void handleError(ConnectionContext ctx, RoutingRequest request, RoutingResponse response, Throwable e) { errorHandler(e.getClass()) .ifPresentOrElse(it -> handleError(ctx, request, response, e, (ErrorHandler) it), () -> unhandledError(ctx, request, response, e)); } - private void unhandledError(ConnectionContext ctx, ServerRequest request, ServerResponse response, Throwable e) { + private void unhandledError(ConnectionContext ctx, ServerRequest request, RoutingResponse response, Throwable e) { if (e instanceof HttpException httpException) { handleRequestException(ctx, request, response, RequestException.builder() .cause(e) @@ -185,8 +194,8 @@ private void unhandledError(ConnectionContext ctx, ServerRequest request, Server } private void handleError(ConnectionContext ctx, - ServerRequest request, - ServerResponse response, + RoutingRequest request, + RoutingResponse response, Throwable e, ErrorHandler it) { if (!response.reset()) { @@ -196,6 +205,7 @@ private void handleError(ConnectionContext ctx, } try { it.handle(request, response, e); + response.commit(); } catch (Exception ex) { ctx.log(LOGGER, System.Logger.Level.TRACE, "Failed to handle exception.", ex); unhandledError(ctx, request, response, e); diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/HttpRouting.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/HttpRouting.java index e5daddc3eea..905127cb8d1 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/HttpRouting.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/HttpRouting.java @@ -412,6 +412,7 @@ public Void call() throws Exception { RoutingResult result = doRoute(ctx, request, response); if (result == RoutingResult.FINISH) { + response.commit(); return null; } if (result == RoutingResult.NONE) { @@ -434,6 +435,7 @@ public Void call() throws Exception { // finished and done if (result == RoutingResult.FINISH) { + response.commit(); return null; } throw new NotFoundException("Endpoint not found"); diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/RoutingResponse.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/RoutingResponse.java index 247a78e3d1e..42df13c2c2d 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/RoutingResponse.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/RoutingResponse.java @@ -55,4 +55,19 @@ public interface RoutingResponse extends ServerResponse { * @return whether has entity */ boolean hasEntity(); + + /** + * Return true if the underlying response buffers and headers can be reset and a new response can be sent. + * + * @return {@code true} if reset was successful and a new response can be created instead of the existing one, + * {@code false} if reset failed and status and headers (and maybe entity bytes) were already sent + */ + boolean reset(); + + /** + * Commit the response. This is mostly useful for output stream based responses, where we may want to delay + * closing the output stream to handle errors, when route uses try with resources. + * After this method is called, response cannot be {@link #reset()}. + */ + void commit(); } diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/ServerResponse.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/ServerResponse.java index f0d02134d0e..e9c749fc220 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/ServerResponse.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http/ServerResponse.java @@ -115,11 +115,6 @@ default void send(Optional entity) { */ boolean isSent(); - /** - * Return true if the underlying response buffers and headers can be reset. - */ - boolean reset(); - /** * Alternative way to send an entity, using an output stream. This should be used for entities that are big * and that should not be materialized into memory. diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ServerResponse.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ServerResponse.java index ebe98492e4e..6166fe899f8 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ServerResponse.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ServerResponse.java @@ -141,17 +141,6 @@ public boolean isSent() { return isSent; } - @Override - public boolean reset() { - if (isSent || outputStream != null && outputStream.totalBytesWritten() > 0) { - return false; - } - headers.clear(); - streamingEntity = false; - outputStream = null; - return true; - } - @Override public OutputStream outputStream() { if (isSent) { @@ -206,6 +195,24 @@ public boolean hasEntity() { return isSent || streamingEntity; } + @Override + public boolean reset() { + if (isSent || outputStream != null && outputStream.totalBytesWritten() > 0) { + return false; + } + headers.clear(); + streamingEntity = false; + outputStream = null; + return true; + } + + @Override + public void commit() { + if (outputStream != null) { + outputStream.commit(); + } + } + private static void writeHeaders(Headers headers, BufferData buffer) { for (HeaderValue header : headers) { header.writeHttp1Header(buffer); @@ -253,6 +260,7 @@ private static class BlockingOutputStream extends OutputStream { private final Http1ServerRequest request; private final boolean keepAlive; private final Supplier streamResult; + private final boolean forcedChunked; private BufferData firstBuffer; private boolean closed; @@ -260,7 +268,6 @@ private static class BlockingOutputStream extends OutputStream { private long contentLength; private boolean isChunked; private boolean firstByte = true; - private boolean forcedChunked; private long responseBytesTotal; private BlockingOutputStream(ServerResponseHeaders headers, @@ -312,6 +319,10 @@ public void flush() throws IOException { @Override public void close() { + // this is a noop, even when user closes the output stream, we wait for commit + } + + void commit() { if (closed) { return; } diff --git a/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http/ErrorHandlersTest.java b/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http/ErrorHandlersTest.java index 2ebf6366a6d..ac6fd0fea57 100644 --- a/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http/ErrorHandlersTest.java +++ b/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http/ErrorHandlersTest.java @@ -96,7 +96,7 @@ static Stream testData() { @ParameterizedTest @MethodSource("testData") - void testHandleFound(TestData testData) { + void testHandlerFound(TestData testData) { ErrorHandlers handlers = testData.handlers(); assertAll( @@ -120,9 +120,10 @@ void testHandler() { } private void testNoHandler(ErrorHandlers handlers, Exception e, String message) { - ServerRequest req = mock(ServerRequest.class); - ServerResponse res = mock(ServerResponse.class); ConnectionContext ctx = mock(ConnectionContext.class); + RoutingRequest req = mock(RoutingRequest.class); + RoutingResponse res = mock(RoutingResponse.class); + when(res.reset()).thenReturn(true); when(req.prologue()).thenReturn(HttpPrologue.create("http/1.0", "http", @@ -149,8 +150,8 @@ private void testNoHandler(ErrorHandlers handlers, Exception e, String message) private void testHandler(ErrorHandlers handlers, Exception e, String message) { ConnectionContext ctx = mock(ConnectionContext.class); - ServerRequest req = mock(ServerRequest.class); - ServerResponse res = mock(ServerResponse.class); + RoutingRequest req = mock(RoutingRequest.class); + RoutingResponse res = mock(RoutingResponse.class); when(res.reset()).thenReturn(true); handlers.runWithErrorHandling(ctx, req, res, () -> { diff --git a/tests/apps/bookstore/bookstore-mp/src/test/java/io/helidon/tests/apps/bookstore/mp/BookResourceTest.java b/tests/apps/bookstore/bookstore-mp/src/test/java/io/helidon/tests/apps/bookstore/mp/BookResourceTest.java index c228020e59c..6ae1b084eb4 100644 --- a/tests/apps/bookstore/bookstore-mp/src/test/java/io/helidon/tests/apps/bookstore/mp/BookResourceTest.java +++ b/tests/apps/bookstore/bookstore-mp/src/test/java/io/helidon/tests/apps/bookstore/mp/BookResourceTest.java @@ -54,7 +54,6 @@ void testBooks() { .request() .get(); assertEquals(Response.Status.OK.getStatusCode(), res.getStatus()); - assertNotNull(res.getHeaderString("content-length")); assertBookStoreSize(1); @@ -88,7 +87,6 @@ void testBooks2() { .request() .get(); assertEquals(Response.Status.OK.getStatusCode(), res.getStatus()); - assertNotNull(res.getHeaderString("content-length")); assertBookStoreSize(1);