Skip to content

Commit a2b9af8

Browse files
committed
[fix] [#417] Fix broken server->client broadcast on client IP change (@Naomarik)
Huge thanks to @Naomarik for reporting and diagnosing this issue! BEFORE THIS COMMIT The following scenario was possible: t0. Client WebSocket connects to server with unique client ID. State in `conns_` atom: `{<client-id1> [<ws-sch:port-for-ip1> <udt>]}`. t1. Client's IP changes (for example, by switching from wifi to cellular network). t2. No `:on-close` event is triggered on server. t3. New client WebSocket connects to server with the SAME client ID but new IP. State in `conns_` atom: `{<client-id1> [<ws-sch:port-for-ip2> <udt>]}`. t4. Server-side connection timeout triggers for ORIGINAL connection (IP1). This unintentionally removes sch for the NEW connection (IP2). State in `conns_` atom: `{<client-id1> [nil <udt>]}`. t5. At this point: - The client has a working WebSocket connection to server. - But the server `conns_` state is bad (has a nil sch for client). - Which means that server->client broadcasts all fail. - This broken state persists until if/when something causes the client to reconnect. But note that this may not happen anytime soon since the client believes (accurately) that it IS successfully connected. IMPLEMENTATION DETAILS Each connection to server establishes the following: - An `:on-open` handler that modifies state in `conns_` for <client-id> - An `:on-close` handler that modifies state in `conns_` for <client-id> The bad behaviour occurs when: - Conn1's delayed `:on-close` triggers after Conn2's `:on-open`. Because Conn1 and Conn2 share the same client-id, they're mutating the same state. AFTER THIS COMMIT We introduce a simple compare-and-swap (CAS) mechanism in the `:on-close` handler so that its state mutations will noop if the current server-ch does not = the server-ch added by the corresponding `:on-open` handler. I.e. a given `:on-close` will now only remove the SAME server-ch added by its corresponding `:on-open`. In the example above, this means that the timeout at t4 will noop.
1 parent 82fc83d commit a2b9af8

File tree

1 file changed

+15
-8
lines changed

1 file changed

+15
-8
lines changed

src/taoensso/sente.cljc

+15-8
Original file line numberDiff line numberDiff line change
@@ -457,21 +457,27 @@
457457
send-buffers_ (atom {:ws {} :ajax {}}) ; {<uid> [<buffered-evs> <#{ev-uuids}>]}
458458
connected-uids_ (atom {:ws #{} :ajax #{} :any #{}}) ; Public
459459

460-
upd-conn!
460+
upd-conn! ; Update client entry in conns_
461461
(fn
462-
([conn-type uid client-id] ; Update udt
462+
([conn-type uid client-id] ; Update only udt
463463
(swap-in! conns_ [conn-type uid client-id]
464464
(fn [?v]
465-
(let [[?sch _udt] ?v
465+
(let [[?sch _?udt] ?v
466466
new-udt (enc/now-udt)]
467467
(enc/swapped
468468
[?sch new-udt]
469469
{:init? (nil? ?v) :udt new-udt :?sch ?sch})))))
470470

471-
([conn-type uid client-id new-?sch] ; Update sch + udt
471+
([conn-type uid client-id old-?sch new-?sch] ; Update sch & udt
472472
(swap-in! conns_ [conn-type uid client-id]
473473
(fn [?v]
474-
(let [new-udt (enc/now-udt)]
474+
(let [[?sch _?udt] ?v
475+
new-udt (enc/now-udt)
476+
new-?sch
477+
(if (or (= old-?sch :any) (identical? old-?sch ?sch))
478+
new-?sch ; CAS successful, Ref. #417
479+
?sch)]
480+
475481
(enc/swapped
476482
[new-?sch new-udt]
477483
{:init? (nil? ?v) :udt new-udt :?sch new-?sch}))))))
@@ -708,6 +714,7 @@
708714
params (get ring-req :params)
709715
client-id (get params :client-id)
710716
uid (user-id-fn ring-req client-id)
717+
;; ?ws-key (get-in ring-req [:headers "sec-websocket-key"])
711718

712719
receive-event-msg! ; Partial
713720
(fn self
@@ -748,7 +755,7 @@
748755

749756
;; WebSocket handshake
750757
(let [_ (tracef "New WebSocket channel: %s (%s)" uid sch-uuid)
751-
updated-conn (upd-conn! :ws uid client-id server-ch)
758+
updated-conn (upd-conn! :ws uid client-id :any server-ch)
752759
udt-open (:udt updated-conn)]
753760

754761
(when (connect-uid! :ws uid)
@@ -774,7 +781,7 @@
774781

775782
;; Ajax handshake/poll
776783
(let [_ (tracef "New Ajax handshake/poll: %s (%s)" uid sch-uuid)
777-
updated-conn (upd-conn! :ajax uid client-id server-ch)
784+
updated-conn (upd-conn! :ajax uid client-id :any server-ch)
778785
udt-open (:udt updated-conn)
779786
handshake? (or (:init? updated-conn) (:handshake? params))]
780787

@@ -814,7 +821,7 @@
814821
(if websocket? "WebSocket" "Ajax")
815822
uid sch-uuid)
816823

817-
updated-conn (upd-conn! conn-type uid client-id nil)
824+
updated-conn (upd-conn! conn-type uid client-id server-ch nil)
818825
udt-close (:udt updated-conn)]
819826

820827
;; Allow some time for possible reconnects (repoll,

0 commit comments

Comments
 (0)