[lib] update Tunnelbroker's `sendMessage` to be based on ref
ClosedPublic
Actions

Authored by kamil on Feb 18 2025, 5:00 PM.

Details

Reviewers

tomek
bartek
angelika

Commits

rCOMM2a359bd969e0: [lib] update Tunnelbroker's `sendMessage` to be based on ref

Summary

Part of ENG-10186 and overall improvement.

There are a couple of places in the codebase where we try to send a message and it could fail with Tunnelbroker not connected but only because using an outdated or not yet re-rendered version of sendMessage, for example:

There is an asynchronous useCallback which is doing a lot of things, and after calls sendMessage, it starts its execution when Tunnelbroker is disconnected.
During execution, a socket connection is established.
In the callback mentioned above, sendMessage is called but it fails because it is bound to the old socketState even though the socket is connected. This diff fixes this.

Not sure if this isn't too hacky, curious about alternatives. We still want to keep the state to return it and allow adding it to dependency where needed.

Test Plan

Simulate the case descrbed above

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Feb 18 2025, 5:00 PM

kamil held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 18 2025, 5:00 PM

kamil added a child revision: D14387: [native] wait for Tunnelbroker connection when adding secondary device.Feb 18 2025, 5:12 PM

kamil published this revision for review.Feb 18 2025, 5:13 PM

kamil edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B33545: Diff 47151.Feb 18 2025, 5:19 PM

angelika added inline comments.Feb 19 2025, 11:13 AM

lib/tunnelbroker/tunnelbroker-context.js
163 ↗	(On Diff #47151)	Are you sure the `useEffect` will fire before `sendMessage` is called and doesn't wait for example to re-render? When I had similar problem Ashoat showed me the "step-based approach" https://linear.app/comm/issue/ENG-9946/cannot-read-properties-of-undefined-reading-members#comment-05db78ce Maybe something similar would be suitable here?

kamil added inline comments.Feb 19 2025, 11:42 AM

lib/tunnelbroker/tunnelbroker-context.js
163 ↗	(On Diff #47151)	The problem with the step-based approach is that it would need to be added not here (here it will require even more re-renders and make it worse), but in all places where we use `sendMessageToDevice`, `sendMessageToTunnelbroker`, `sendNotif` etc., as those places would need to wait for the updated value of callbacks. (there are a lot of them). Fair point with `useEffect` being fired after `sendMessage`, the point of my solution for to just improve it (it's not ideal but better than previously). But when thinking about this now, alternatively we can remove the effect and set the ref in all places where we call `setSocketState` which should make it even more reliable, does it sound okay?

tomek accepted this revision.Feb 19 2025, 12:03 PM

tomek added inline comments.

lib/tunnelbroker/tunnelbroker-context.js
163 ↗	(On Diff #47151)	In the context of this problem, the approach where we set a ref in an effect and the approach where we set ref immediately are the same. It comes from the fact that setting the state and calling `sendMessage` shouldn't happen in the same rendering cycle. In general, the approach with the effect is more correct. If we have a function that 1. sets the state, 2. reads the state, and 3. reads the ref, then in the current approach both reads would give the same value, but with immediate ref setting the values will be inconsistent (the ref would be ahead of the state).