[phantom-presence] handle stray websockets #207
Merged
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
|
View Vercel preview at instant-www-js-socket-issues-jsv.vercel.app. |
stopachka
changed the title
stopachka
commented
Sep 13, 2024
| (let [pooled (.getData message)] | ||
| (try | ||
| (let [payload (.getResource pooled)] | ||
| (on-message {:channel (channel-wrapper channel) | ||
| :data (Util/toArray payload)})) | ||
| (finally (.free pooled))))) | ||
| (onPong [^WebSocketChannel channel ^StreamSourceFrameChannel channel] |
There was a problem hiding this comment.
You may be wondering:
How does the client know to send a pong, we did not change Reactor.
Most clients now can handle websocket pings by default. So since the server sends ping, chrome will automatically respond with a pong.
(Tested this with chrome and expo iOS)
dwwoelfel
approved these changes
Sep 13, 2024
| @@ -548,7 +533,6 @@ | |||
| :http-req http-req | |||
| :ws-conn ws-conn | |||
| :receive-q receive-q | |||
| :ping-job (start-ping-job store-conn id) | |||
| :pending-handlers pending-handlers}] | |||
| (on-open store-conn socket))) | |||
| :on-message (fn [{:keys [data]}] | |||
There was a problem hiding this comment.
Should we update the atomic-last-received-at in on-message also?
There was a problem hiding this comment.
I pushed this down to the caller of on-message, so this already happens. Took the inspiration of updating on every message from Precursor : )
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.
This PR:
Context
Kosmik reported a curious bug: If you went offline and went back online, you would get a phantom presence object. What happened?
Bugs
The fundamental bug was this: tcp connections stayed 'active', even when we went offline.
We assumed that the underlying websocket would close when we went offline. But this is not the case. Google Chrome does not close the socket automatically when you go offline. [1]
What happens to idle TCP connections really depends on the machine [2]. Sometimes it can take minutes to close the connection.
Solutions
I did two things in this PR:
1) Client: Close previous connections when starting new ones
In Reactor, when we go back online, we run _startSocket. We used to assume that previous
_wswould have been closed. Now no more: when we start a new socket, we'll make sure to close the previous one.2) Server: Start ping/pong
Having the client close stray connections is a good step. But it doesn't quite cover it. For example, if the user goes offline, and never goes back online, then what?
The server could keep the connection around for minutes. Here's what we do now: the server sends
pingmessages every 5 seconds. If we don't receive some message within 10 seconds, we close the connection.[1] Here's an interesting chrome bug report
[2] Interesting microsoft article hiredman shared with us
I kicked off a conversation about this on the clojurians slack