From f04809d513e0ee1b0060c4763de3b2c4e54c8cd9 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Fri, 15 Sep 2023 14:28:24 +0100 Subject: [PATCH] [4.x] Fix websocket reconnect race condition (#7815) (#7817) * Fix websocket reconnect race condition (#7815) (cherry picked from commit f62fd4787653d8dd9035f6727c253b6cd900e0fd) * Attempt to fix a flaky test * Evict connection pool a second time after tests (#7819) (cherry picked from commit e2344c727761651c2cc8d7523a13944a117b9411) * Simplify --- .../kotlin/okhttp3/OkHttpClientTestRule.kt | 3 +- .../okhttp3/internal/ws/RealWebSocket.kt | 2 +- .../internal/ws/WebSocketHttpTest.java | 81 +++++++++++++++++-- 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/okhttp-testing-support/src/main/kotlin/okhttp3/OkHttpClientTestRule.kt b/okhttp-testing-support/src/main/kotlin/okhttp3/OkHttpClientTestRule.kt index 1c4c5d2dd945..c783bc3bb4c3 100644 --- a/okhttp-testing-support/src/main/kotlin/okhttp3/OkHttpClientTestRule.kt +++ b/okhttp-testing-support/src/main/kotlin/okhttp3/OkHttpClientTestRule.kt @@ -153,7 +153,8 @@ class OkHttpClientTestRule : TestRule { println("After delay: " + connectionPool.connectionCount()) } - assertEquals(0, connectionPool.connectionCount()) + connectionPool.evictAll() + assertEquals("Still ${connectionPool.connectionCount()} connections open", 0, connectionPool.connectionCount()) } } diff --git a/okhttp/src/main/kotlin/okhttp3/internal/ws/RealWebSocket.kt b/okhttp/src/main/kotlin/okhttp3/internal/ws/RealWebSocket.kt index 7934222ff008..f048f8488c56 100644 --- a/okhttp/src/main/kotlin/okhttp3/internal/ws/RealWebSocket.kt +++ b/okhttp/src/main/kotlin/okhttp3/internal/ws/RealWebSocket.kt @@ -170,9 +170,9 @@ class RealWebSocket( checkUpgradeSuccess(response, exchange) streams = exchange!!.newWebSocketStreams() } catch (e: IOException) { - exchange?.webSocketUpgradeFailed() failWebSocket(e, response) response.closeQuietly() + exchange?.webSocketUpgradeFailed() return } diff --git a/okhttp/src/test/java/okhttp3/internal/ws/WebSocketHttpTest.java b/okhttp/src/test/java/okhttp3/internal/ws/WebSocketHttpTest.java index 82a2bfb9ec58..708c5e482a0e 100644 --- a/okhttp/src/test/java/okhttp3/internal/ws/WebSocketHttpTest.java +++ b/okhttp/src/test/java/okhttp3/internal/ws/WebSocketHttpTest.java @@ -22,10 +22,13 @@ import java.net.ProtocolException; import java.net.SocketTimeoutException; import java.time.Duration; +import java.util.ArrayList; +import java.util.List; import java.util.Random; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import okhttp3.ConnectionPool; import okhttp3.OkHttpClient; import okhttp3.OkHttpClientTestRule; import okhttp3.Protocol; @@ -315,11 +318,16 @@ private OkHttpClientTestRule configureClientTestRule() { webServer.enqueue(new MockResponse() .setResponseCode(101) .setHeader("Upgrade", "websocket") - .setHeader("Sec-WebSocket-Accept", "ujmZX4KXZqjwy6vi1aQFH5p4Ygk=")); - newWebSocket(); + .setHeader("Sec-WebSocket-Accept", "ujmZX4KXZqjwy6vi1aQFH5p4Ygk=") + ); + webServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); + + RealWebSocket webSocket = newWebSocket(); clientListener.assertFailure(101, null, ProtocolException.class, "Expected 'Connection' header value 'Upgrade' but was 'null'"); + + webSocket.cancel(); } @Test public void wrongConnectionHeader() throws IOException { @@ -328,10 +336,14 @@ private OkHttpClientTestRule configureClientTestRule() { .setHeader("Upgrade", "websocket") .setHeader("Connection", "Downgrade") .setHeader("Sec-WebSocket-Accept", "ujmZX4KXZqjwy6vi1aQFH5p4Ygk=")); - newWebSocket(); + webServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); + + RealWebSocket webSocket = newWebSocket(); clientListener.assertFailure(101, null, ProtocolException.class, "Expected 'Connection' header value 'Upgrade' but was 'Downgrade'"); + + webSocket.cancel(); } @Test public void missingUpgradeHeader() throws IOException { @@ -339,10 +351,14 @@ private OkHttpClientTestRule configureClientTestRule() { .setResponseCode(101) .setHeader("Connection", "Upgrade") .setHeader("Sec-WebSocket-Accept", "ujmZX4KXZqjwy6vi1aQFH5p4Ygk=")); - newWebSocket(); + webServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); + + RealWebSocket webSocket = newWebSocket(); clientListener.assertFailure(101, null, ProtocolException.class, "Expected 'Upgrade' header value 'websocket' but was 'null'"); + + webSocket.cancel(); } @Test public void wrongUpgradeHeader() throws IOException { @@ -351,10 +367,14 @@ private OkHttpClientTestRule configureClientTestRule() { .setHeader("Connection", "Upgrade") .setHeader("Upgrade", "Pepsi") .setHeader("Sec-WebSocket-Accept", "ujmZX4KXZqjwy6vi1aQFH5p4Ygk=")); - newWebSocket(); + webServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); + + RealWebSocket webSocket = newWebSocket(); clientListener.assertFailure(101, null, ProtocolException.class, "Expected 'Upgrade' header value 'websocket' but was 'Pepsi'"); + + webSocket.cancel(); } @Test public void missingMagicHeader() throws IOException { @@ -362,10 +382,14 @@ private OkHttpClientTestRule configureClientTestRule() { .setResponseCode(101) .setHeader("Connection", "Upgrade") .setHeader("Upgrade", "websocket")); - newWebSocket(); + webServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); + + RealWebSocket webSocket = newWebSocket(); clientListener.assertFailure(101, null, ProtocolException.class, "Expected 'Sec-WebSocket-Accept' header value 'ujmZX4KXZqjwy6vi1aQFH5p4Ygk=' but was 'null'"); + + webSocket.cancel(); } @Test public void wrongMagicHeader() throws IOException { @@ -374,10 +398,14 @@ private OkHttpClientTestRule configureClientTestRule() { .setHeader("Connection", "Upgrade") .setHeader("Upgrade", "websocket") .setHeader("Sec-WebSocket-Accept", "magic")); - newWebSocket(); + webServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); + + RealWebSocket webSocket = newWebSocket(); clientListener.assertFailure(101, null, ProtocolException.class, "Expected 'Sec-WebSocket-Accept' header value 'ujmZX4KXZqjwy6vi1aQFH5p4Ygk=' but was 'magic'"); + + webSocket.cancel(); } @Test public void clientIncludesForbiddenHeader() throws IOException { @@ -800,6 +828,45 @@ private OkHttpClientTestRule configureClientTestRule() { webSocket.close(1000, null); } + /** https://github.com/square/okhttp/issues/7768 */ + @Test public void reconnectingToNonWebSocket() throws InterruptedException { + for (int i = 0; i < 30; i++) { + webServer.enqueue(new MockResponse() + .setBodyDelay(100, TimeUnit.MILLISECONDS) + .setBody("Wrong endpoint") + .setResponseCode(401)); + } + + Request request = new Request.Builder() + .url(webServer.url("/")) + .build(); + + CountDownLatch attempts = new CountDownLatch(20); + + List webSockets = new ArrayList<>(); + + WebSocketListener reconnectOnFailure = new WebSocketListener() { + @Override + public void onFailure(WebSocket webSocket, Throwable t, Response response) { + if (attempts.getCount() > 0) { + clientListener.setNextEventDelegate(this); + webSockets.add(client.newWebSocket(request, clientListener)); + attempts.countDown(); + } + } + }; + + clientListener.setNextEventDelegate(reconnectOnFailure); + + webSockets.add(client.newWebSocket(request, clientListener)); + + attempts.await(); + + for (WebSocket webSocket: webSockets) { + webSocket.cancel(); + } + } + @Test public void compressedMessages() throws Exception { successfulExtensions("permessage-deflate"); }