Reconnect overhaul #792
Merged
Reconnect overhaul #792
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
lgrahl
force-pushed
the
push-overhaul
branch
5 times, most recently
from
330d3f7 to
a708140
Compare
lgrahl
force-pushed
the
push-overhaul
branch
3 times, most recently
from
3ccd7a8 to
9faa97c
Compare
This comment has been minimized.
This comment has been minimized.
|
⬆️ Fixed. |
dbrgn
approved these changes
May 8, 2019
The PushService can spawn a Push instance which... - Retransmits the Push until an undefined goal has been achieved - Applies a TTL to the push message - Collapses push messages by applying a collapse key
The texts of the welcome page have been updated and are being mostly reused for the "device unreachable" dialog. Further changes: - Removed fixed height requirement for the welcome page. - Removed dead code in welcome page CSS
Remove all the timeout/retry logic for reconnecting Android
This allows the push server to collapse all pushes from the same session since the device is only interested in the most recent push.
An attempt to re-establish a connection may fail for different reasons than pushes. For example, WebRTC could be blocked by a plugin or the WebSocket connection fails. If the connection cannot be established after 3 tries, the "device unreachable" dialog will be shown.
This allows the app to detect whether consecutive pushes are referring to the same session connection attempt.
When the connection to the app is already lost but there was no explicit closure, we previously had to wait until the SaltyRTC server or the underlying transport recognised that the connection has been lost. With this change, we request a connectionAck and send a push after 3 seconds of silence (no incoming chunks). This has a cooldown phase of 10 seconds.
|
Does this tackle #106? |
|
Dozens of things have been discussed in #106. Please elaborate. 🙂 |
|
@lgrahl: Today I could again reproduce #106 (12%) with https://web-beta.threema.ch. However, in this case the session could not even reconnect when I touched the smartphone and opened Threema. The session I cannot reconnect to is displayed as follows: Here's the log (while it's stil at 12 %): Relevant message_log.txtRelevant logs from web application |
|
@ovalseven8 this is out of scope regarding the client-side implementation, but a quick summary: It seems that the app still thought it was connected when it received the pushes, and therefore ignored them. We have a system of storing "pending wakeups" in the app though, that should be processed once the connection is actually lost. However, in this case the state machine goes directly from CONNECTING to ERROR. Unfortunately, the pending wakeups are processed in the DISCONNECTED state, which is skipped (since there was never a connection). Furthermore, the connect timeout in the app is quite long (42s). Maybe that could be reduced by a pending wakeup. @lgrahl maybe you want to take a closer look. I've filed an internal issue for this. |
|
Umbrella issue: #802 |
|
@MarcoZehe It would be great to get your feedback regarding the accessibility of the newly introduced dialog that pops up when the connection could not be re-established. You can enforce that dialog by switching to airplane mode on your mobile once the web client has established a connection and you're in the messenger view. |
|
@lgrahl This dialog talks very, very nicely! Also the keyboard focus lands on the Retry button, and the dialog text gets read automatically. Very well done! |
|
I would have expected issues but I'll take a pleasant surprise. Many thanks! |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Push Session (aka Continuous Pushes)
Add a push session instance will send pushes continuously until an undefined goal has been achieved which needs to call the
.donemethod to stop pushes.The push session will stop and reject the returned promise in case the push relay determined a client error (e.g. an invalid push token). In any other case, it will continue sending pushes. Thus, it is crucial to call
.doneeventually!With default settings, the push session will send a push in the following intervals: 0s, 2s, 4s, 8s, 16s, 30s (maximum), 30s, ...
The first push will use a TTL of 0, the second push a TTL of 15s, and all subsequent pushes will use a TTL of 90s.
The default settings intend to wake up the app immediately by the first push which uses a TTL of 0, indicating the push server to deliver now or never. The mid TTL tries to work around potential issues with FCM clients misinterpreting the TTL as don't need to dispatch until expired. And the TTL of 90s acts as a last resort mechanism to wake up the app eventually.
Device Unreachable Dialog
Instead of redirecting to the welcome page (Android) or doing nothing and letting the web client remain in a limbo state (iOS), the push session will retry waking the device three times. If that is unsuccessful, it will show a new dialog that allows to either retry or reload page. This dialog will disappear automatically once a connection to the device has been established.
Fast Session Recovery
When the connection to the app is already lost but there was no explicit closure, we previously had to wait until the SaltyRTC server or the underlying transport recognised that the connection has been lost.
With this change, we request a connectionAck and send a push after 3 seconds of silence (no incoming chunks). This has a cooldown phase of 10 seconds.
Note: This only affects iOS at the moment.
To Do
Resolves #354