Skip to content

Commit 9f894fb

Browse files
tsegismonttsegismont
authored
Fix intermittent failures in CI (#2928)
* Synchronize SockJSSession.register() to prevent concurrent handler invocation When multiple verticle instances share the SockJS session map, register() can be called concurrently from different event loop threads. The openWritten field is a plain boolean with no memory visibility guarantee, so both threads can read it as false and invoke the socket handler twice. Synchronize the critical section on `this`, consistent with close(). Signed-off-by: Thomas Segismont <tsegismont@gmail.com> * Fix intermittent failure in RouterTest on Windows testDoNotUseSemicolonDelimiter closes then reopens the server on port 8080. On Windows, TIME_WAIT delays port release causing bind failures. Use an ephemeral port for the reopened server. Signed-off-by: Thomas Segismont <tsegismont@gmail.com> * Fix intermittent timeout in EventbusBridge testHookCreateSocketRejected The TransportClient close handler can fire before the BridgeClient sets its closeHandler callback via onSuccess, causing the close event to be silently dropped. Track a pending close and replay it when the handler is set. Signed-off-by: Thomas Segismont <tsegismont@gmail.com> * Fix intermittent timeout in SockJSRawTransportTest The server writes data immediately in the socket handler, but the data frame can arrive at the client (in the same TCP segment as the handshake response) before the frame handler is set. Have the server wait for a client message before writing, so the frame handler is guaranteed to be in place. Signed-off-by: Thomas Segismont <tsegismont@gmail.com> --------- Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
1 parent 164393d commit 9f894fb

4 files changed

Lines changed: 70 additions & 47 deletions

File tree

‎vertx-web/src/main/java/io/vertx/ext/web/handler/sockjs/impl/SockJSSession.java‎

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -364,36 +364,38 @@ void register(HttpServerRequest req, TransportListener lst) {
364364
this.remoteAddress = req.remoteAddress();
365365
this.uri = req.uri();
366366
this.headers = BaseTransport.removeCookieHeaders(req.headers());
367-
if (closed) {
368-
// Closed by the application
369-
writeClosed(lst);
370-
// And close the listener request
371-
lst.close();
372-
} else if (this.listener != null) {
373-
writeClosed(lst, 2010, "Another connection still open");
374-
// And close the listener request
375-
lst.close();
376-
} else {
367+
synchronized (this) {
368+
if (closed) {
369+
// Closed by the application
370+
writeClosed(lst);
371+
// And close the listener request
372+
lst.close();
373+
} else if (this.listener != null) {
374+
writeClosed(lst, 2010, "Another connection still open");
375+
// And close the listener request
376+
lst.close();
377+
} else {
377378

378-
cancelTimer();
379+
cancelTimer();
379380

380-
this.listener = lst;
381+
this.listener = lst;
381382

382-
if (!openWritten) {
383-
writeOpen(lst);
384-
sockHandler.handle(this);
385-
handleCalled = true;
386-
}
383+
if (!openWritten) {
384+
writeOpen(lst);
385+
sockHandler.handle(this);
386+
handleCalled = true;
387+
}
387388

388-
if (listener != null) {
389-
if (closed) {
390-
// Could have already been closed by the user
391-
writeClosed(lst);
392-
listener = null;
393-
lst.close();
394-
} else {
395-
if (!pendingWrites.isEmpty()) {
396-
writePendingMessages();
389+
if (listener != null) {
390+
if (closed) {
391+
// Could have already been closed by the user
392+
writeClosed(lst);
393+
listener = null;
394+
lst.close();
395+
} else {
396+
if (!pendingWrites.isEmpty()) {
397+
writePendingMessages();
398+
}
397399
}
398400
}
399401
}

‎vertx-web/src/test/java/io/vertx/ext/web/tests/RouterTest.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3108,7 +3108,7 @@ public void testEscapeURIParam() throws Exception {
31083108
@Test
31093109
public void testDoNotUseSemicolonDelimiter() throws Exception {
31103110

3111-
HttpServerConfig config = new HttpServerConfig(getHttpServerOptions())
3111+
HttpServerConfig config = new HttpServerConfig(getHttpServerOptions().setPort(0))
31123112
.setQueryParamConfig(new QueryParamDecoderConfig().setUseSemicolonAsDelimiter(false));
31133113

31143114
server.close().await();
@@ -3124,7 +3124,11 @@ public void testDoNotUseSemicolonDelimiter() throws Exception {
31243124
assertEquals("b;c", params.get("a"));
31253125
rc.end();
31263126
});
3127-
testRequest(HttpMethod.GET, "/?a=b;c", 200, "OK");
3127+
testRequest(new RequestOptions()
3128+
.setMethod(HttpMethod.GET)
3129+
.setPort(server.actualPort())
3130+
.setHost("localhost")
3131+
.setURI("/?a=b;c"), 200, "OK", null);
31283132
}
31293133

31303134
@Test

‎vertx-web/src/test/java/io/vertx/ext/web/tests/handler/EventbusBridgeTest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1399,6 +1399,7 @@ Future<TransportClient> connect(WebSocketClient client, String address) {
13991399
return client.connect("/eventbus/400/8ne8e94a/websocket").map(ws -> new TransportClient() {
14001400
private Handler<JsonObject> handler;
14011401
private Handler<Void> closeHandler;
1402+
private boolean closePending;
14021403

14031404
{
14041405
ws.handler(buff -> {
@@ -1422,6 +1423,8 @@ Future<TransportClient> connect(WebSocketClient client, String address) {
14221423
ws.closeHandler(v -> {
14231424
if (closeHandler != null) {
14241425
closeHandler.handle(v);
1426+
} else {
1427+
closePending = true;
14251428
}
14261429
});
14271430
}
@@ -1434,6 +1437,10 @@ public void handler(Handler<JsonObject> handler) {
14341437
@Override
14351438
public void closeHandler(Handler<Void> handler) {
14361439
this.closeHandler = handler;
1440+
if (closePending && handler != null) {
1441+
closePending = false;
1442+
handler.handle(null);
1443+
}
14371444
}
14381445

14391446
@Override
@@ -1464,6 +1471,7 @@ Future<TransportClient> connect(WebSocketClient client, String address) {
14641471
return client.connect(address).map(ws -> new TransportClient() {
14651472
private Handler<JsonObject> handler;
14661473
private Handler<Void> closeHandler;
1474+
private boolean closePending;
14671475

14681476
{
14691477
ws.handler(buff -> {
@@ -1477,6 +1485,8 @@ Future<TransportClient> connect(WebSocketClient client, String address) {
14771485
ws.closeHandler(v -> {
14781486
if (closeHandler != null) {
14791487
closeHandler.handle(v);
1488+
} else {
1489+
closePending = true;
14801490
}
14811491
});
14821492
}
@@ -1489,6 +1499,10 @@ public void handler(Handler<JsonObject> handler) {
14891499
@Override
14901500
public void closeHandler(Handler<Void> handler) {
14911501
this.closeHandler = handler;
1502+
if (closePending && handler != null) {
1503+
closePending = false;
1504+
handler.handle(null);
1505+
}
14921506
}
14931507

14941508
@Override

‎vertx-web/src/test/java/io/vertx/ext/web/tests/handler/sockjs/SockJSRawTransportTest.java‎

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
package io.vertx.ext.web.tests.handler.sockjs;
1717

1818
import io.vertx.core.buffer.Buffer;
19+
import io.vertx.core.http.WebSocket;
1920
import io.vertx.core.http.WebSocketConnectOptions;
2021
import io.vertx.ext.web.handler.sockjs.SockJSHandlerOptions;
2122
import io.vertx.junit5.Checkpoint;
@@ -126,31 +127,33 @@ public void badOrigin(Checkpoint checkpoint) throws Exception {
126127
private void testWrite(boolean text, Checkpoint checkpoint) throws Exception {
127128
String expected = TestUtils.randomAlphaString(64);
128129
socketHandler = () -> socket -> {
129-
if (text) {
130-
socket.write(expected);
131-
} else {
132-
socket.write(Buffer.buffer(expected));
133-
}
130+
socket.handler(msg -> {
131+
if (text) {
132+
socket.write(expected);
133+
} else {
134+
socket.write(Buffer.buffer(expected));
135+
}
136+
});
134137
socket.endHandler(v -> {
135138
checkpoint.flag();
136139
});
137140
};
138141
startServers(new SockJSHandlerOptions());
139-
wsClient.connect("/test/websocket").onComplete(TestUtils.onSuccess(ws -> {
140-
ws.frameHandler(frame -> {
141-
if (frame.isClose()) {
142-
//
142+
WebSocket ws = wsClient.connect("/test/websocket").await();
143+
ws.frameHandler(frame -> {
144+
if (frame.isClose()) {
145+
//
146+
} else {
147+
if (text) {
148+
assertTrue(frame.isText());
149+
assertEquals(expected, frame.textData());
143150
} else {
144-
if (text) {
145-
assertTrue(frame.isText());
146-
assertEquals(expected, frame.textData());
147-
} else {
148-
assertTrue(frame.isBinary());
149-
assertEquals(Buffer.buffer(expected), frame.binaryData());
150-
}
151-
ws.end();
151+
assertTrue(frame.isBinary());
152+
assertEquals(Buffer.buffer(expected), frame.binaryData());
152153
}
153-
});
154-
}));
154+
ws.end();
155+
}
156+
});
157+
ws.writeTextMessage("ready");
155158
}
156159
}

0 commit comments

Comments
 (0)