TakahikoKawasaki / nv-websocket-client

High-quality WebSocket client implementation in Java.
Apache License 2.0
2.03k stars 292 forks source link

Race between WebSocketAdapter.onConnected() and .onTextMessage(). #237

Open minichma opened 2 years ago

minichma commented 2 years ago

The order in which the methods WebSocketAdapter.onConnected() and WebSocketAdapter.onTextMessage() are called is not deterministic.

When listening to a WebSocket via a WebSocketAdatpter, one would expect that for every successful web socket connection onConnected() would be called before actual data is delivered via onTextMessage() et al, which is not the case. In some rare cases, if the server starts sending data right after the connection is established, onTextMessage(), etc. can be called before onConnected() is called.

Consider the following code:

        WebSocketFactory webSocketFactory = new WebSocketFactory();
        WebSocket ws = webSocketFactory.createSocket("wss://foo.bar");

        List<String> events = new ArrayList<>();
        ws.addListener(new WebSocketAdapter() {
            @Override
            public void onConnected(WebSocket websocket, Map<String, List<String>> headers) throws Exception {
                super.onConnected(websocket, headers);

                // delaying here helps reproducing the issue, but is not strictly required.
                Thread.sleep(10);
                synchronized (events) {
                    events.add("onConnected");
                }
            }

            @Override
            public void onTextMessage(WebSocket websocket, String text) throws Exception {
                super.onTextMessage(websocket, text);
                synchronized (events) {
                    events.add("onTextMessage");
                }
            }
        });

        try {
            ws.connectAsynchronously();
            Thread.sleep(1000);
            ws.disconnect();
        } catch (Exception e) {
        }

        if (events.size() >= 2) {
            Assert.assertEquals("onConnected", events.get(0));
            Assert.assertEquals("onTextMessage", events.get(1));
        }

The order in which onConnected and onTextMessage are added to the events list is not deterministic.

I'm not sure, whether the order of the callbacks is documented, so one could argue that this is not a bug. But in any case the current behavior is very unexpected and makes state handling significantly harder. Moreover it happens only very rarely, that the onConnected would not be called first, which makes this issue a candidate for causing very subtle and hard to find bugs.

As a workaround one could listen to the onStateChanged(OPEN) event, which is called from the connection thread and therefore always before the other discussed events (which are called form the reader/writer threads which are only started after onStateChanged). But this is only a workaround and most people probably won't realize that the order of one is deterministic while the other one's isn't.