From f0ea3e072083a80fa96f3e4d29f13490d92f95a7 Mon Sep 17 00:00:00 2001 From: Tomas Langer Date: Fri, 19 Jul 2024 19:48:24 +0200 Subject: [PATCH] 4.x: Fix problem where throwing an Error would close connection but send keep-alive (#9014) * Fix wrong keep alive when error happens. * Error handlers now correctly handle Throwable and not just Exception. The HTTP/1.1 connection now correctly sends Connection: close header on throwables that cause the connection to be closed. --- .../tests/ErrorHandlingThrowableTest.java | 89 +++++++++++++++++++ .../helidon/webserver/http/ErrorHandlers.java | 4 +- .../webserver/http1/Http1Connection.java | 8 +- 3 files changed, 96 insertions(+), 5 deletions(-) create mode 100644 webserver/tests/webserver/src/test/java/io/helidon/webserver/tests/ErrorHandlingThrowableTest.java diff --git a/webserver/tests/webserver/src/test/java/io/helidon/webserver/tests/ErrorHandlingThrowableTest.java b/webserver/tests/webserver/src/test/java/io/helidon/webserver/tests/ErrorHandlingThrowableTest.java new file mode 100644 index 00000000000..6c95b9e76c8 --- /dev/null +++ b/webserver/tests/webserver/src/test/java/io/helidon/webserver/tests/ErrorHandlingThrowableTest.java @@ -0,0 +1,89 @@ +/* + * 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.tests; + +import io.helidon.http.Header; +import io.helidon.http.HeaderName; +import io.helidon.http.HeaderNames; +import io.helidon.http.HeaderValues; +import io.helidon.webclient.api.ClientResponseTyped; +import io.helidon.webclient.http1.Http1Client; +import io.helidon.webserver.http.ErrorHandler; +import io.helidon.webserver.http.HttpRouting; +import io.helidon.webserver.http.ServerRequest; +import io.helidon.webserver.http.ServerResponse; +import io.helidon.webserver.testing.junit5.DirectClient; +import io.helidon.webserver.testing.junit5.RoutingTest; +import io.helidon.webserver.testing.junit5.SetUpRoute; + +import org.junit.jupiter.api.Test; + +import static io.helidon.common.testing.http.junit5.HttpHeaderMatcher.hasHeader; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +@RoutingTest +class ErrorHandlingThrowableTest { + private static final HeaderName CONTROL_HEADER = HeaderNames.create("X-HELIDON-JUNIT"); + private static final Header THROW = HeaderValues.create(CONTROL_HEADER, "throw"); + + private final Http1Client client; + + ErrorHandlingThrowableTest(DirectClient client) { + this.client = client; + } + + @SetUpRoute + static void routing(HttpRouting.Builder builder) { + builder.error(SomeError.class, new SomeErrorHandler()) + .get("/", ErrorHandlingThrowableTest::handler); + } + + @Test + void testOk() { + String response = client.get() + .requestEntity(String.class); + assertThat(response, is("Done")); + } + + @Test + void testSomeError() { + ClientResponseTyped response = client.get() + .header(THROW) + .request(String.class); + assertThat(response.headers(), hasHeader(HeaderValues.CONNECTION_CLOSE)); + assertThat(response.entity(), is("Handled")); + } + + private static void handler(ServerRequest req, ServerResponse res) throws Exception { + if (req.headers().contains(THROW)) { + throw new SomeError(); + } + res.send("Done"); + } + + private static class SomeErrorHandler implements ErrorHandler { + @Override + public void handle(ServerRequest req, ServerResponse res, SomeError throwable) { + res.header(HeaderValues.CONNECTION_CLOSE); + res.send("Handled"); + } + } + + private static class SomeError extends Error { + } +} diff --git a/webserver/webserver/src/main/java/io/helidon/webserver/http/ErrorHandlers.java b/webserver/webserver/src/main/java/io/helidon/webserver/http/ErrorHandlers.java index 6366ded8381..0c21e214a17 100644 --- a/webserver/webserver/src/main/java/io/helidon/webserver/http/ErrorHandlers.java +++ b/webserver/webserver/src/main/java/io/helidon/webserver/http/ErrorHandlers.java @@ -67,6 +67,7 @@ public String toString() { * @param response HTTP server response * @param task task to execute */ + @SuppressWarnings({"unchecked", "rawtypes"}) public void runWithErrorHandling(ConnectionContext ctx, RoutingRequest request, RoutingResponse response, @@ -116,7 +117,7 @@ public void runWithErrorHandling(ConnectionContext ctx, } } catch (RuntimeException e) { handleError(ctx, request, response, e); - } catch (Exception e) { + } catch (Throwable e) { if (e.getCause() instanceof SocketException se) { throw new UncheckedIOException(se); } @@ -176,6 +177,7 @@ private void handleRequestException(ConnectionContext ctx, response.commit(); } + @SuppressWarnings("unchecked") private void handleError(ConnectionContext ctx, RoutingRequest request, RoutingResponse response, Throwable e) { errorHandler(e.getClass()) .ifPresentOrElse(it -> handleError(ctx, request, response, e, (ErrorHandler) it), diff --git a/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1Connection.java b/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1Connection.java index 2a742e4218e..6bc1bc08756 100644 --- a/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1Connection.java +++ b/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1Connection.java @@ -215,7 +215,6 @@ public void handle(Semaphore requestSemaphore) throws InterruptedException { .cause(e) .type(EventType.BAD_REQUEST) .status(e.status()) - .setKeepAlive(e.keepAlive()) .build()); } catch (RequestException e) { handleRequestException(e); @@ -465,9 +464,10 @@ private void handleRequestException(RequestException e) { BufferData buffer = BufferData.growing(128); ServerResponseHeaders headers = response.headers(); - if (!e.keepAlive()) { - headers.set(HeaderValues.CONNECTION_CLOSE); - } + + // we are escaping the connection loop, the connection will be closed + headers.set(HeaderValues.CONNECTION_CLOSE); + byte[] message = response.entity().orElse(BufferData.EMPTY_BYTES); headers.set(HeaderValues.create(HeaderNames.CONTENT_LENGTH, String.valueOf(message.length)));