From 791375699664194a4bd69dc15f439ad244083a29 Mon Sep 17 00:00:00 2001 From: Kay Werndli Date: Mon, 23 Sep 2024 15:42:37 +0200 Subject: [PATCH] 4.x: Unbuffered OutputStream in Http1ServerResponse (#9276) Currently, having a write buffer size of 0 always leads to an exception when an Http1 request handler tries to create an OutputStream. --- .../webserver/http1/Http1ServerResponse.java | 48 +++++++--- .../webserver/http1/WriteBufferTest.java | 94 +++++++++++++++++++ 2 files changed, 128 insertions(+), 14 deletions(-) create mode 100644 webserver/webserver/src/test/java/io/helidon/webserver/http1/WriteBufferTest.java diff --git a/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerResponse.java b/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerResponse.java index 40e6d09b228..c94049e66d1 100644 --- a/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerResponse.java +++ b/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerResponse.java @@ -75,7 +75,7 @@ class Http1ServerResponse extends ServerResponseBase { private boolean streamingEntity; private boolean isSent; - private ClosingBufferedOutputStream outputStream; + private ResponseOutputStream outputStream; private long bytesWritten; private String streamResult = ""; private final boolean validateHeaders; @@ -395,7 +395,11 @@ private OutputStream outputStream(boolean skipEncoders) { validateHeaders); int writeBufferSize = ctx.listenerContext().config().writeBufferSize(); - outputStream = new ClosingBufferedOutputStream(bos, writeBufferSize); + if (writeBufferSize > 0) { + outputStream = new ClosingBufferedOutputStream(bos, writeBufferSize); + } else { + outputStream = bos; + } OutputStream encodedOutputStream = outputStream; if (!skipEncoders) { @@ -405,7 +409,29 @@ private OutputStream outputStream(boolean skipEncoders) { return outputStreamFilter == null ? encodedOutputStream : outputStreamFilter.apply(encodedOutputStream); } - static class BlockingOutputStream extends OutputStream { + abstract static class ResponseOutputStream extends OutputStream { + abstract long totalBytesWritten(); + + abstract void commit(); + + /** + * This is a noop, even when user closes the output stream, we wait for the + * call to {@link this#commit()}. + */ + @Override + public void close() { + // no-op + } + + /** + * Calls the {@link OutputStream#close()}, which is currently a no-op. + */ + void superClose() throws IOException { + super.close(); + } + } + + static class BlockingOutputStream extends ResponseOutputStream { private final ServerResponseHeaders headers; private final WritableHeaders trailers; private final Supplier status; @@ -490,15 +516,6 @@ public void flush() throws IOException { } } - /** - * This is a noop, even when user closes the output stream, we wait for the - * call to {@link this#commit()}. - */ - @Override - public void close() { - // no-op - } - /** * Informs output stream that closing phase has started. Special handling * for {@link this#flush()}. @@ -542,12 +559,13 @@ void commit() { responseCloseRunnable.run(); try { - super.close(); + superClose(); } catch (IOException e) { throw new ServerConnectionException("Failed to close server response stream.", e); } } + @Override long totalBytesWritten() { return responseBytesTotal; } @@ -728,7 +746,7 @@ private void writeContent(BufferData buffer) throws IOException { * of close logic. Note that due to some locking issues in the JDK, this class * must use delegation with {@link BufferedOutputStream} instead of subclassing. */ - static class ClosingBufferedOutputStream extends OutputStream { + static class ClosingBufferedOutputStream extends ResponseOutputStream { private final BlockingOutputStream delegate; private final BufferedOutputStream bufferedDelegate; @@ -768,10 +786,12 @@ public void close() { } } + @Override long totalBytesWritten() { return delegate.totalBytesWritten(); } + @Override void commit() { try { flush(); diff --git a/webserver/webserver/src/test/java/io/helidon/webserver/http1/WriteBufferTest.java b/webserver/webserver/src/test/java/io/helidon/webserver/http1/WriteBufferTest.java new file mode 100644 index 00000000000..8dfedcecb4a --- /dev/null +++ b/webserver/webserver/src/test/java/io/helidon/webserver/http1/WriteBufferTest.java @@ -0,0 +1,94 @@ +/* + * Copyright (c) 2024 Oracle and/or its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.helidon.webserver.http1; + +import io.helidon.webserver.WebServer; +import io.helidon.webserver.http.Handler; +import io.helidon.webserver.http1.Http1ServerResponse.BlockingOutputStream; +import io.helidon.webserver.http1.Http1ServerResponse.ClosingBufferedOutputStream; +import org.junit.jupiter.api.Test; + +import java.io.OutputStream; +import java.net.HttpURLConnection; +import java.net.URI; +import java.net.URL; +import java.nio.charset.StandardCharsets; + +import io.helidon.webserver.http.ServerResponse; + +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +class WriteBufferTest { + + /** + * Test that a simple response can be sent using the {@link ServerResponse#outputStream()} using the default + * (non-zero) write buffer. + */ + @Test + void defaultWriteBufferTest() throws Exception { + String path = "/test"; + String response = "Hello World!"; + Handler handler = (req, res) -> { + try(OutputStream out = res.outputStream()) { + assertThat(out, instanceOf(ClosingBufferedOutputStream.class)); + out.write(response.getBytes(StandardCharsets.UTF_8)); + } + }; + WebServer server = WebServer.builder().port(0).routing(it -> it.get(path, handler)).build().start(); + try { + URL url = new URI("http://localhost:" + server.port() + path).toURL(); + HttpURLConnection conn = (HttpURLConnection)url.openConnection(); + conn.setRequestMethod("GET"); + assertThat(conn.getResponseCode(), is(200)); + String received = new String(conn.getInputStream().readAllBytes(), StandardCharsets.UTF_8); + assertThat(received, is(response)); + } finally { + server.stop(); + } + } + + /** + * Test that a simple response can be sent using the {@link ServerResponse#outputStream()} using no write buffer + * (i.e. the write buffer size was set to {@code 0}). + */ + @Test + void noWriteBufferTest() throws Exception { + String path = "/test"; + String response = "Hello World!"; + Handler handler = (req, res) -> { + try(OutputStream out = res.outputStream()) { + assertThat(out, instanceOf(BlockingOutputStream.class)); + out.write(response.getBytes(StandardCharsets.UTF_8)); + } + }; + WebServer server = WebServer.builder().port(0).writeBufferSize(0) + .routing(it -> it.get(path, handler)).build().start(); + try { + URL url = new URI("http://localhost:" + server.port() + path).toURL(); + HttpURLConnection conn = (HttpURLConnection)url.openConnection(); + conn.setRequestMethod("GET"); + assertThat(conn.getResponseCode(), is(200)); + String received = new String(conn.getInputStream().readAllBytes(), StandardCharsets.UTF_8); + assertThat(received, is(response)); + } finally { + server.stop(); + } + } + +}