From 06c8510e716581abfdf77d40ac74a9b83de3a402 Mon Sep 17 00:00:00 2001 From: Martin Kouba Date: Thu, 5 Sep 2024 15:35:55 +0200 Subject: [PATCH] WebSockets next: improve default strategies for unhandled failures - introduce UnhandledFailureStrategy.LOG_AND_CLOSE - WebSocketsClientRuntimeConfig#unhandledFailureStrategy - change the default value from "close" to "log" - WebSocketsServerRuntimeConfig#unhandledFailureStrategy - change the default value from "close" to "log-and-close" - resolves #42569 --- .../asciidoc/websockets-next-reference.adoc | 4 ++-- ...ndledMessageFailureCloseStrategyTest.java} | 15 +++++++------ ...dledMessageFailureDefaultStrategyTest.java | 11 +++++----- ...nhandledOpenFailureCloseStrategyTest.java} | 17 +++++++------- ...handledOpenFailureDefaultStrategyTest.java | 13 ++++++----- .../next/UnhandledFailureStrategy.java | 8 +++++-- .../next/WebSocketsClientRuntimeConfig.java | 7 ++++-- .../next/WebSocketsServerRuntimeConfig.java | 4 ++-- .../websockets/next/runtime/Endpoints.java | 22 ++++++++++++++++--- .../next/runtime/TrafficLogger.java | 5 +++-- 10 files changed, 65 insertions(+), 41 deletions(-) rename extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/{UnhandledMessageFailureLogStrategyTest.java => UnhandledMessageFailureCloseStrategyTest.java} (68%) rename extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/{UnhandledOpenFailureLogStrategyTest.java => UnhandledOpenFailureCloseStrategyTest.java} (68%) diff --git a/docs/src/main/asciidoc/websockets-next-reference.adoc b/docs/src/main/asciidoc/websockets-next-reference.adoc index 779057a83e110..37aa9ce172276 100644 --- a/docs/src/main/asciidoc/websockets-next-reference.adoc +++ b/docs/src/main/asciidoc/websockets-next-reference.adoc @@ -424,8 +424,8 @@ The method that declares a most-specific supertype of the actual exception is se NOTE: The `@io.quarkus.websockets.next.OnError` annotation can be also used to declare a global error handler, i.e. a method that is not declared on a WebSocket endpoint. Such a method may not accept `@PathParam` paremeters. Error handlers declared on an endpoint take precedence over the global error handlers. When an error occurs but no error handler can handle the failure, Quarkus uses the strategy specified by `quarkus.websockets-next.server.unhandled-failure-strategy`. -By default, the connection is closed. -Alternatively, an error message can be logged or no operation performed. +For server endpoints, the error message is logged and the connection is closed by default. +For client endpoints, the error message is logged by default. [[serialization]] === Serialization and deserialization diff --git a/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledMessageFailureLogStrategyTest.java b/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledMessageFailureCloseStrategyTest.java similarity index 68% rename from extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledMessageFailureLogStrategyTest.java rename to extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledMessageFailureCloseStrategyTest.java index 664cbf1caf286..b4be930da36ca 100644 --- a/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledMessageFailureLogStrategyTest.java +++ b/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledMessageFailureCloseStrategyTest.java @@ -1,7 +1,6 @@ package io.quarkus.websockets.next.test.client; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import java.net.URI; @@ -12,18 +11,19 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import io.netty.handler.codec.http.websocketx.WebSocketCloseStatus; import io.quarkus.test.QuarkusUnitTest; import io.quarkus.test.common.http.TestHTTPResource; import io.quarkus.websockets.next.WebSocketClientConnection; import io.quarkus.websockets.next.WebSocketConnector; -public class UnhandledMessageFailureLogStrategyTest { +public class UnhandledMessageFailureCloseStrategyTest { @RegisterExtension public static final QuarkusUnitTest test = new QuarkusUnitTest() .withApplicationRoot(root -> { root.addClasses(ServerEndpoint.class, ClientMessageErrorEndpoint.class); - }).overrideConfigKey("quarkus.websockets-next.client.unhandled-failure-strategy", "log"); + }).overrideConfigKey("quarkus.websockets-next.client.unhandled-failure-strategy", "close"); @Inject WebSocketConnector connector; @@ -37,10 +37,11 @@ void testError() throws InterruptedException { .baseUri(testUri) .connectAndAwait(); connection.sendTextAndAwait("foo"); - assertFalse(connection.isClosed()); - connection.sendTextAndAwait("bar"); - assertTrue(ClientMessageErrorEndpoint.MESSAGE_LATCH.await(5, TimeUnit.SECONDS)); - assertEquals("bar", ClientMessageErrorEndpoint.MESSAGES.get(0)); + assertTrue(ServerEndpoint.CLOSED_LATCH.await(5, TimeUnit.SECONDS)); + assertTrue(ClientMessageErrorEndpoint.CLOSED_LATCH.await(5, TimeUnit.SECONDS)); + assertTrue(connection.isClosed()); + assertEquals(WebSocketCloseStatus.INVALID_MESSAGE_TYPE.code(), connection.closeReason().getCode()); + assertTrue(ClientMessageErrorEndpoint.MESSAGES.isEmpty()); } } diff --git a/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledMessageFailureDefaultStrategyTest.java b/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledMessageFailureDefaultStrategyTest.java index a1d80c81a021f..0d3c6b9b4f1df 100644 --- a/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledMessageFailureDefaultStrategyTest.java +++ b/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledMessageFailureDefaultStrategyTest.java @@ -1,6 +1,7 @@ package io.quarkus.websockets.next.test.client; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import java.net.URI; @@ -11,7 +12,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; -import io.netty.handler.codec.http.websocketx.WebSocketCloseStatus; import io.quarkus.test.QuarkusUnitTest; import io.quarkus.test.common.http.TestHTTPResource; import io.quarkus.websockets.next.WebSocketClientConnection; @@ -37,11 +37,10 @@ void testError() throws InterruptedException { .baseUri(testUri) .connectAndAwait(); connection.sendTextAndAwait("foo"); - assertTrue(ServerEndpoint.CLOSED_LATCH.await(5, TimeUnit.SECONDS)); - assertTrue(ClientMessageErrorEndpoint.CLOSED_LATCH.await(5, TimeUnit.SECONDS)); - assertTrue(connection.isClosed()); - assertEquals(WebSocketCloseStatus.INTERNAL_SERVER_ERROR.code(), connection.closeReason().getCode()); - assertTrue(ClientMessageErrorEndpoint.MESSAGES.isEmpty()); + assertFalse(connection.isClosed()); + connection.sendTextAndAwait("bar"); + assertTrue(ClientMessageErrorEndpoint.MESSAGE_LATCH.await(5, TimeUnit.SECONDS)); + assertEquals("bar", ClientMessageErrorEndpoint.MESSAGES.get(0)); } } diff --git a/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledOpenFailureLogStrategyTest.java b/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledOpenFailureCloseStrategyTest.java similarity index 68% rename from extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledOpenFailureLogStrategyTest.java rename to extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledOpenFailureCloseStrategyTest.java index dc5f6d41504fa..ac067fdd18c61 100644 --- a/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledOpenFailureLogStrategyTest.java +++ b/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledOpenFailureCloseStrategyTest.java @@ -1,8 +1,6 @@ package io.quarkus.websockets.next.test.client; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.net.URI; @@ -13,18 +11,19 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import io.netty.handler.codec.http.websocketx.WebSocketCloseStatus; import io.quarkus.test.QuarkusUnitTest; import io.quarkus.test.common.http.TestHTTPResource; import io.quarkus.websockets.next.WebSocketClientConnection; import io.quarkus.websockets.next.WebSocketConnector; -public class UnhandledOpenFailureLogStrategyTest { +public class UnhandledOpenFailureCloseStrategyTest { @RegisterExtension public static final QuarkusUnitTest test = new QuarkusUnitTest() .withApplicationRoot(root -> { root.addClasses(ServerEndpoint.class, ClientOpenErrorEndpoint.class); - }).overrideConfigKey("quarkus.websockets-next.client.unhandled-failure-strategy", "log"); + }).overrideConfigKey("quarkus.websockets-next.client.unhandled-failure-strategy", "close"); @Inject WebSocketConnector connector; @@ -37,11 +36,11 @@ void testError() throws InterruptedException { WebSocketClientConnection connection = connector .baseUri(testUri) .connectAndAwait(); - connection.sendTextAndAwait("foo"); - assertFalse(connection.isClosed()); - assertNull(connection.closeReason()); - assertTrue(ClientOpenErrorEndpoint.MESSAGE_LATCH.await(5, TimeUnit.SECONDS)); - assertEquals("foo", ClientOpenErrorEndpoint.MESSAGES.get(0)); + assertTrue(ServerEndpoint.CLOSED_LATCH.await(5, TimeUnit.SECONDS)); + assertTrue(ClientOpenErrorEndpoint.CLOSED_LATCH.await(5, TimeUnit.SECONDS)); + assertTrue(connection.isClosed()); + assertEquals(WebSocketCloseStatus.INVALID_MESSAGE_TYPE.code(), connection.closeReason().getCode()); + assertTrue(ClientOpenErrorEndpoint.MESSAGES.isEmpty()); } } diff --git a/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledOpenFailureDefaultStrategyTest.java b/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledOpenFailureDefaultStrategyTest.java index decf21f2b1705..4a87e81abe1a9 100644 --- a/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledOpenFailureDefaultStrategyTest.java +++ b/extensions/websockets-next/deployment/src/test/java/io/quarkus/websockets/next/test/client/UnhandledOpenFailureDefaultStrategyTest.java @@ -1,6 +1,8 @@ package io.quarkus.websockets.next.test.client; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.net.URI; @@ -11,7 +13,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; -import io.netty.handler.codec.http.websocketx.WebSocketCloseStatus; import io.quarkus.test.QuarkusUnitTest; import io.quarkus.test.common.http.TestHTTPResource; import io.quarkus.websockets.next.WebSocketClientConnection; @@ -36,11 +37,11 @@ void testError() throws InterruptedException { WebSocketClientConnection connection = connector .baseUri(testUri) .connectAndAwait(); - assertTrue(ServerEndpoint.CLOSED_LATCH.await(5, TimeUnit.SECONDS)); - assertTrue(ClientOpenErrorEndpoint.CLOSED_LATCH.await(5, TimeUnit.SECONDS)); - assertTrue(connection.isClosed()); - assertEquals(WebSocketCloseStatus.INTERNAL_SERVER_ERROR.code(), connection.closeReason().getCode()); - assertTrue(ClientOpenErrorEndpoint.MESSAGES.isEmpty()); + connection.sendTextAndAwait("foo"); + assertFalse(connection.isClosed()); + assertNull(connection.closeReason()); + assertTrue(ClientOpenErrorEndpoint.MESSAGE_LATCH.await(5, TimeUnit.SECONDS)); + assertEquals("foo", ClientOpenErrorEndpoint.MESSAGES.get(0)); } } diff --git a/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/UnhandledFailureStrategy.java b/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/UnhandledFailureStrategy.java index bdfb1f17ad2be..bb516d9602c18 100644 --- a/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/UnhandledFailureStrategy.java +++ b/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/UnhandledFailureStrategy.java @@ -5,11 +5,15 @@ */ public enum UnhandledFailureStrategy { /** - * Close the connection. + * Log the error message and close the connection. + */ + LOG_AND_CLOSE, + /** + * Close the connection silently. */ CLOSE, /** - * Log an error message. + * Log the error message. */ LOG, /** diff --git a/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/WebSocketsClientRuntimeConfig.java b/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/WebSocketsClientRuntimeConfig.java index 90c84b47a90c9..30ad1b84f474a 100644 --- a/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/WebSocketsClientRuntimeConfig.java +++ b/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/WebSocketsClientRuntimeConfig.java @@ -43,9 +43,12 @@ public interface WebSocketsClientRuntimeConfig { /** * The strategy used when an error occurs but no error handler can handle the failure. *

- * By default, the connection is closed when an unhandled failure occurs. + * By default, the error message is logged when an unhandled failure occurs. + *

+ * Note that clients should not close the WebSocket connection arbitrarily. See also RFC-6455 + * section 7.3. */ - @WithDefault("close") + @WithDefault("log") UnhandledFailureStrategy unhandledFailureStrategy(); /** diff --git a/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/WebSocketsServerRuntimeConfig.java b/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/WebSocketsServerRuntimeConfig.java index 650067a60aa41..a5df5c23dd104 100644 --- a/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/WebSocketsServerRuntimeConfig.java +++ b/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/WebSocketsServerRuntimeConfig.java @@ -49,9 +49,9 @@ public interface WebSocketsServerRuntimeConfig { /** * The strategy used when an error occurs but no error handler can handle the failure. *

- * By default, the connection is closed when an unhandled failure occurs. + * By default, the error message is logged and the connection is closed when an unhandled failure occurs. */ - @WithDefault("close") + @WithDefault("log-and-close") UnhandledFailureStrategy unhandledFailureStrategy(); /** diff --git a/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/Endpoints.java b/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/Endpoints.java index 12a2b327fa6b1..587dfe047bba1 100644 --- a/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/Endpoints.java +++ b/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/Endpoints.java @@ -8,8 +8,10 @@ import org.jboss.logging.Logger; +import io.netty.handler.codec.http.websocketx.WebSocketCloseStatus; import io.quarkus.arc.ArcContainer; import io.quarkus.arc.InjectableContext; +import io.quarkus.runtime.LaunchMode; import io.quarkus.security.AuthenticationFailedException; import io.quarkus.security.ForbiddenException; import io.quarkus.security.UnauthorizedException; @@ -253,18 +255,32 @@ public void handle(Void event) { private static void handleFailure(UnhandledFailureStrategy strategy, Throwable cause, String message, WebSocketConnectionBase connection) { switch (strategy) { - case CLOSE -> closeConnection(cause, connection); + case LOG_AND_CLOSE -> logAndClose(cause, message, connection); + case CLOSE -> closeConnection(cause, message, connection); case LOG -> logFailure(cause, message, connection); case NOOP -> LOG.tracef("Unhandled failure ignored: %s", connection); default -> throw new IllegalArgumentException("Unexpected strategy: " + strategy); } } - private static void closeConnection(Throwable cause, WebSocketConnectionBase connection) { + private static void logAndClose(Throwable cause, String message, WebSocketConnectionBase connection) { + logFailure(cause, message, connection); + closeConnection(cause, message, connection); + } + + private static void closeConnection(Throwable cause, String message, WebSocketConnectionBase connection) { if (connection.isClosed()) { return; } - connection.close(CloseReason.INTERNAL_SERVER_ERROR).subscribe().with( + CloseReason closeReason; + int statusCode = connection instanceof WebSocketClientConnectionImpl ? WebSocketCloseStatus.INVALID_MESSAGE_TYPE.code() + : WebSocketCloseStatus.INTERNAL_SERVER_ERROR.code(); + if (LaunchMode.current().isDevOrTest()) { + closeReason = new CloseReason(statusCode, cause.getMessage()); + } else { + closeReason = new CloseReason(statusCode); + } + connection.close(closeReason).subscribe().with( v -> LOG.debugf("Connection closed due to unhandled failure %s: %s", cause, connection), t -> LOG.errorf("Unable to close connection [%s] due to unhandled failure [%s]: %s", connection.id(), cause, t)); diff --git a/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/TrafficLogger.java b/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/TrafficLogger.java index 4a2a86dd9eadf..de977e0828936 100644 --- a/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/TrafficLogger.java +++ b/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/TrafficLogger.java @@ -84,9 +84,10 @@ void binaryMessageSent(WebSocketConnectionBase connection, Buffer payload) { void connectionClosed(WebSocketConnectionBase connection) { if (LOG.isDebugEnabled()) { - LOG.debugf("%s connection closed, Connection[%s]", + LOG.debugf("%s connection closed, Connection[%s], %s", typeToString(), - connectionToString(connection)); + connectionToString(connection), + connection.closeReason()); } }