From 3d03339d43aff12bb11b9cf8fa4eeec4bdddb627 Mon Sep 17 00:00:00 2001 From: Lachlan Date: Tue, 21 Nov 2023 07:58:38 +1100 Subject: [PATCH] Issue #10749 - WebSocketClient should expose upgrade request/response (#10761) * Allow UpgradeListener to see Response in case of non-successful upgrade * Using a boolean to track whether request was upgraded * Improved exception handling in HttpUpgraderOverHTTP. * Avoid using log warnings in CoreClientUpgradeRequest. * Delayed setting of this.upgraded after the last throw statement. Signed-off-by: Lachlan Roberts Signed-off-by: Simone Bordet Co-authored-by: Simone Bordet --- .../core/client/CoreClientUpgradeRequest.java | 29 +++-- .../client/internal/HttpUpgraderOverHTTP.java | 13 +- .../tests/client/ClientResponseTest.java | 123 ++++++++++++++++++ 3 files changed, 154 insertions(+), 11 deletions(-) create mode 100644 jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientResponseTest.java diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-client/src/main/java/org/eclipse/jetty/websocket/core/client/CoreClientUpgradeRequest.java b/jetty-core/jetty-websocket/jetty-websocket-core-client/src/main/java/org/eclipse/jetty/websocket/core/client/CoreClientUpgradeRequest.java index 9edf55c14122..251c91b7fa19 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-client/src/main/java/org/eclipse/jetty/websocket/core/client/CoreClientUpgradeRequest.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-client/src/main/java/org/eclipse/jetty/websocket/core/client/CoreClientUpgradeRequest.java @@ -84,6 +84,7 @@ public FrameHandler getFrameHandler() private final Configuration.ConfigurationCustomizer customizer = new Configuration.ConfigurationCustomizer(); private final List upgradeListeners = new ArrayList<>(); private List requestedExtensions = new ArrayList<>(); + private boolean upgraded; public CoreClientUpgradeRequest(WebSocketCoreClient webSocketClient, URI requestURI) { @@ -237,19 +238,28 @@ public CompletableFuture sendAsync() return futureCoreSession; } - @SuppressWarnings("Duplicates") @Override public void onComplete(Result result) { if (LOG.isDebugEnabled()) - { LOG.debug("onComplete() - {}", result); - } URI requestURI = result.getRequest().getURI(); + Request request = result.getRequest(); Response response = result.getResponse(); - int responseStatusCode = response.getStatus(); - String responseLine = responseStatusCode + " " + response.getReason(); + int status = response.getStatus(); + String responseLine = status + " " + response.getReason(); + + if (!upgraded) + { + // We have failed to upgrade but have received a response, so notify the listener. + Throwable listenerError = notifyUpgradeListeners((listener) -> listener.onHandshakeResponse(request, response)); + if (listenerError != null) + { + if (LOG.isDebugEnabled()) + LOG.debug("error from listener", listenerError); + } + } if (result.isFailed()) { @@ -266,15 +276,15 @@ public void onComplete(Result result) Throwable failure = result.getFailure(); boolean wrapFailure = !(failure instanceof IOException) && !(failure instanceof UpgradeException); if (wrapFailure) - failure = new UpgradeException(requestURI, responseStatusCode, responseLine, failure); + failure = new UpgradeException(requestURI, status, responseLine, failure); handleException(failure); return; } - if (responseStatusCode != HttpStatus.SWITCHING_PROTOCOLS_101) + if (status != HttpStatus.SWITCHING_PROTOCOLS_101) { // Failed to upgrade (other reason) - handleException(new UpgradeException(requestURI, responseStatusCode, + handleException(new UpgradeException(requestURI, status, "Failed to upgrade to websocket: Unexpected HTTP Response Status Code: " + responseLine)); } } @@ -290,7 +300,7 @@ protected void handleException(Throwable failure) } catch (Throwable t) { - LOG.warn("FrameHandler onError threw", t); + LOG.info("FrameHandler onError threw", t); } } } @@ -485,6 +495,7 @@ else if (values.length == 1) Throwable listenerError = notifyUpgradeListeners((listener) -> listener.onHandshakeResponse(request, response)); if (listenerError != null) throw new WebSocketException("onHandshakeResponse error", listenerError); + upgraded = true; // Now swap out the connection try diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-client/src/main/java/org/eclipse/jetty/websocket/core/client/internal/HttpUpgraderOverHTTP.java b/jetty-core/jetty-websocket/jetty-websocket-core-client/src/main/java/org/eclipse/jetty/websocket/core/client/internal/HttpUpgraderOverHTTP.java index fd2c66662782..e181ad891fdd 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-client/src/main/java/org/eclipse/jetty/websocket/core/client/internal/HttpUpgraderOverHTTP.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-client/src/main/java/org/eclipse/jetty/websocket/core/client/internal/HttpUpgraderOverHTTP.java @@ -89,11 +89,20 @@ public void upgrade(Response response, EndPoint endPoint, Callback callback) String respHash = responseHeaders.get(HttpHeader.SEC_WEBSOCKET_ACCEPT); if (expectedHash.equalsIgnoreCase(respHash)) { - clientUpgradeRequest.upgrade(response, endPoint); - callback.succeeded(); + try + { + clientUpgradeRequest.upgrade(response, endPoint); + callback.succeeded(); + } + catch (Throwable x) + { + callback.failed(x); + } } else + { callback.failed(new HttpResponseException("Invalid Sec-WebSocket-Accept hash (was: " + respHash + " expected: " + expectedHash + ")", response)); + } } else { diff --git a/jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientResponseTest.java b/jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientResponseTest.java new file mode 100644 index 000000000000..ee6d4112ab64 --- /dev/null +++ b/jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientResponseTest.java @@ -0,0 +1,123 @@ +// +// ======================================================================== +// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.websocket.tests.client; + +import java.net.URI; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; + +import org.eclipse.jetty.client.Request; +import org.eclipse.jetty.client.Response; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.server.handler.ContextHandler; +import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.websocket.api.exceptions.UpgradeException; +import org.eclipse.jetty.websocket.client.ClientUpgradeRequest; +import org.eclipse.jetty.websocket.client.JettyUpgradeListener; +import org.eclipse.jetty.websocket.client.WebSocketClient; +import org.eclipse.jetty.websocket.server.WebSocketCreator; +import org.eclipse.jetty.websocket.server.WebSocketUpgradeHandler; +import org.eclipse.jetty.websocket.tests.EchoSocket; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class ClientResponseTest +{ + private Server _server; + private ServerConnector _connector; + private WebSocketClient _client; + + public void before(WebSocketCreator creator) throws Exception + { + _server = new Server(); + _connector = new ServerConnector(_server); + _server.addConnector(_connector); + + ContextHandler contextHandler = new ContextHandler(); + WebSocketUpgradeHandler upgradeHandler = WebSocketUpgradeHandler.from(_server, contextHandler); + contextHandler.setHandler(upgradeHandler); + _server.setHandler(contextHandler); + upgradeHandler.configure(container -> container.addMapping("/", creator)); + + _server.start(); + + _client = new WebSocketClient(); + _client.start(); + } + + @AfterEach + public void after() throws Exception + { + _client.stop(); + _server.stop(); + } + + @Test + public void testResponseOnUpgradeFailure() throws Exception + { + before((req, resp, cb) -> + { + resp.setStatus(HttpStatus.IM_A_TEAPOT_418); + resp.getHeaders().put("specialHeader", "value123"); + resp.write(true, BufferUtil.toBuffer("failed by test"), cb); + return null; + }); + + EchoSocket clientEndpoint = new EchoSocket(); + URI uri = URI.create("ws://localhost:" + _connector.getLocalPort()); + ClientUpgradeRequest upgradeRequest = new ClientUpgradeRequest(); + + CompletableFuture responseFuture = new CompletableFuture<>(); + CompletableFuture contentFuture = new CompletableFuture<>(); + JettyUpgradeListener upgradeListener = new JettyUpgradeListener() + { + @Override + public void onHandshakeRequest(Request request) + { + request.onResponseContentSource((resp, source) -> + assertDoesNotThrow(() -> contentFuture.complete(IO.toString(Content.Source.asInputStream(source))))); + } + + @Override + public void onHandshakeResponse(Request request, Response response) + { + responseFuture.complete(response); + } + }; + + Throwable t = assertThrows(Throwable.class, () -> + _client.connect(clientEndpoint, uri, upgradeRequest, upgradeListener).get(5, TimeUnit.SECONDS)); + assertThat(t, instanceOf(ExecutionException.class)); + assertThat(t.getCause(), instanceOf(UpgradeException.class)); + assertThat(t.getCause().getMessage(), containsString("Failed to upgrade to websocket: Unexpected HTTP Response Status Code: 418")); + + Response response = responseFuture.get(5, TimeUnit.SECONDS); + String content = contentFuture.get(5, TimeUnit.SECONDS); + assertThat(response.getStatus(), equalTo(HttpStatus.IM_A_TEAPOT_418)); + assertThat(response.getHeaders().get("specialHeader"), equalTo("value123")); + assertThat(content, equalTo("failed by test")); + } +}