fix: resume client to server communication after web socket reconnection #20283

mcollovati · 2024-10-18T10:42:07Z

Description

When a websocket PUSH connection is closed and re-established because of a network failure, the RequestResponseTracker.hasActiveRequest is not reset, prenvint the Flow client to send additional messages to the server.
This change will reset the flag on reconnection. It also will track unsent PUSH message over websocket, to retry the delivery once the connection is re-established, preventing client resynchronization. In addition, it sets a default value of 12 for the Atmospehere maxWebsocketErrorRetries setting, to ensure that the Flow client will attempt to reconnect with web socket transport several times, instead of immediately downgrade to long-polling after first failed connection.

Fixes #20213

Type of change

Bugfix
Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended.
New and existing tests are passing locally with my change.
I have performed self-review and corrected misspellings.

Additional for `Feature` type of change

Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

When a websocket PUSH connection is closed and re-established because of a network failure, the RequestResponseTracker.hasActiveRequest is not reset, prenvint the Flow client to send additional messages to the server. This change will reset the flag on reconnection. It also will track unsent PUSH message over websocket, to retry the delivery once the connection is re-established, preventing client resynchronization. In addition, it sets a default value of 12 for the Atmospehere maxWebsocketErrorRetries setting, to ensure that the Flow client will attempt to reconnect with web socket transport several times, instead of immediately downgrade to long-polling after first failed connection. Fixes #20213

github-actions · 2024-10-18T10:56:53Z

Test Results

1 142 files +26 1 142 suites +26 1h 26m 31s ⏱️ -31s
7 474 tests +26 7 424 ✅ +26 50 💤 ±0 0 ❌ ±0
7 813 runs +22 7 753 ✅ +22 60 💤 ±0 0 ❌ ±0

Results for commit c39d00a. ± Comparison against base commit f1c9673.

♻️ This comment has been updated with latest results.

Artur- · 2024-10-22T08:13:36Z

flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java

+            if (pushPendingMessage != null
+                    && (int) pushPendingMessage.getNumber(
+                            ApplicationConstants.CLIENT_TO_SERVER_ID) < nextExpectedId) {
+                pushPendingMessage = null;


What happens if the message reaches the server and then the channel is disconnected? It looks like a duplicate of the last message will be sent - what happens after that?

If the message is the same as the last processed one (both client message id and hash must match) then it is ignored on the server side; the response should then have a next expected message id greater than one of the cached message, and pushPendingMessage should be nullified.

However, if client id is too old, then the "Unexpected message id from the client ..." error will be raised.

mshabarov

The patch looks reasonable to me, but I didn't verify it with application.

sonarcloud · 2024-10-28T14:46:10Z

Quality Gate passed

Issues
86 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mshabarov · 2024-11-07T08:28:39Z

Successfully tested with the Ubuntu in Virtual Box by connecting/disconnectiong the network adapter:

reconnect.mov

I used

@Push(transport = Transport.WEBSOCKET)

mshabarov · 2024-11-07T08:29:16Z

@mcollovati I think we can move to ready for review and merge.

…ion (#20283) * fix: resume client to server communication after web socket reconnection When a websocket PUSH connection is closed and re-established because of a network failure, the RequestResponseTracker.hasActiveRequest is not reset, prenvint the Flow client to send additional messages to the server. This change will reset the flag on reconnection. It also will track unsent PUSH message over websocket, to retry the delivery once the connection is re-established, preventing client resynchronization. In addition, it sets a default value of 12 for the Atmospehere maxWebsocketErrorRetries setting, to ensure that the Flow client will attempt to reconnect with web socket transport several times, instead of immediately downgrade to long-polling after first failed connection. Fixes #20213 * upgrade to atmosphere javascript 4.0.1 with reconnection fixes --------- Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>

…ion (#20283) (#20423) * fix: resume client to server communication after web socket reconnection When a websocket PUSH connection is closed and re-established because of a network failure, the RequestResponseTracker.hasActiveRequest is not reset, prenvint the Flow client to send additional messages to the server. This change will reset the flag on reconnection. It also will track unsent PUSH message over websocket, to retry the delivery once the connection is re-established, preventing client resynchronization. In addition, it sets a default value of 12 for the Atmospehere maxWebsocketErrorRetries setting, to ensure that the Flow client will attempt to reconnect with web socket transport several times, instead of immediately downgrade to long-polling after first failed connection. Fixes #20213 * upgrade to atmosphere javascript 4.0.1 with reconnection fixes --------- Co-authored-by: Marco Collovati <marco@vaadin.com> Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>

…ion (#20283) (#20424) * fix: resume client to server communication after web socket reconnection When a websocket PUSH connection is closed and re-established because of a network failure, the RequestResponseTracker.hasActiveRequest is not reset, prenvint the Flow client to send additional messages to the server. This change will reset the flag on reconnection. It also will track unsent PUSH message over websocket, to retry the delivery once the connection is re-established, preventing client resynchronization. In addition, it sets a default value of 12 for the Atmospehere maxWebsocketErrorRetries setting, to ensure that the Flow client will attempt to reconnect with web socket transport several times, instead of immediately downgrade to long-polling after first failed connection. Fixes #20213 * upgrade to atmosphere javascript 4.0.1 with reconnection fixes --------- Co-authored-by: Marco Collovati <marco@vaadin.com> Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>

vaadin-bot added the +0.0.1 label Oct 18, 2024

mcollovati requested a review from Artur- October 18, 2024 10:56

upgrade to atmosphere javascript 4.0.1 with reconnection fixes

a804f85

vaadin-bot added +1.0.0 and removed +0.0.1 labels Oct 22, 2024

Artur- reviewed Oct 22, 2024

View reviewed changes

mshabarov self-requested a review October 28, 2024 11:48

mshabarov approved these changes Oct 28, 2024

View reviewed changes

Merge branch 'main' into issues/20213_websocket_network_disconnection

c39d00a

vaadin-bot added +0.0.1 and removed +1.0.0 labels Oct 28, 2024

mshabarov added target/24.4 target/24.5 labels Nov 7, 2024

mcollovati marked this pull request as ready for review November 7, 2024 08:52

mshabarov merged commit b72e395 into main Nov 7, 2024
26 checks passed

mshabarov deleted the issues/20213_websocket_network_disconnection branch November 7, 2024 09:07

vaadin-bot added the cherry-picked-24.5 label Nov 7, 2024

vaadin-bot added the cherry-picked-24.4 label Nov 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resume client to server communication after web socket reconnection #20283

fix: resume client to server communication after web socket reconnection #20283

mcollovati commented Oct 18, 2024

github-actions bot commented Oct 18, 2024 •

edited

Loading

Artur- Oct 22, 2024

mcollovati Oct 22, 2024

mcollovati Oct 22, 2024

mshabarov left a comment

sonarcloud bot commented Oct 28, 2024

mshabarov commented Nov 7, 2024

mshabarov commented Nov 7, 2024

fix: resume client to server communication after web socket reconnection #20283

fix: resume client to server communication after web socket reconnection #20283

Conversation

mcollovati commented Oct 18, 2024

Description

Type of change

Checklist

Additional for Feature type of change

github-actions bot commented Oct 18, 2024 • edited Loading

Test Results

Artur- Oct 22, 2024

Choose a reason for hiding this comment

mcollovati Oct 22, 2024

Choose a reason for hiding this comment

mcollovati Oct 22, 2024

Choose a reason for hiding this comment

mshabarov left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 28, 2024

Quality Gate passed

mshabarov commented Nov 7, 2024

mshabarov commented Nov 7, 2024

Additional for `Feature` type of change

github-actions bot commented Oct 18, 2024 •

edited

Loading