Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
minichma opened this issue Jan 18, 2022 · 0 comments
Open

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

minichma opened this issue Jan 18, 2022 · 0 comments

Comments

@minichma
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant