[lib] Wait with processing inbound messages for Tunnelbroker connection
ClosedPublic
Actions

Authored by angelika on Oct 25 2024, 2:09 PM.

Details

Reviewers

tomek
kamil

Commits

rCOMM9360b1f7fff5: [lib] Wait with processing inbound messages for Tunnelbroker connection

Summary

https://linear.app/comm/issue/ENG-9804/wait-with-processing-inbound-messages-for-tunnelbroker-connection

Depends on D13802

Test Plan

Comment out code in removeInboundP2PMessages() function
Send some messages
Now getAllInboundP2PMessages() should always return something
Restart the app
Verify that:
before the patch in the logs there are errors like "Error while sending confirmation: Tunnelbroker not connected [Error: Tunnelbroker not connected]"
after the patch the errors should disappear

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

angelika created this revision.Oct 25 2024, 2:09 PM

Herald added a subscriber: ashoat. · View Herald TranscriptOct 25 2024, 2:09 PM

Harbormaster completed remote builds in B32376: Diff 45392.Oct 25 2024, 2:26 PM

angelika requested review of this revision.Oct 25 2024, 2:26 PM

angelika mentioned this in D13802: [lib] Make sure PeerToPeerMessageHandler is run after persisted state is read.Oct 29 2024, 4:52 AM

I would prefer a slightly different approach, where we could read Inbound messages and add them to the queue, there is no point in deffering this - but instead wait with actual processing, to achieve that we might need to modify useActionsQueue to be able to conditionally start processing, what do you think?

Additionally, this could mitigate ENG-9713 - when there is no TB connection, we won't decrypt message which could not be confirmed to Tunnelbroker.

Requesting to discuss this - but I am open to accepting this too if you think what I suggested has some drawbacks.

This revision now requires changes to proceed.Oct 29 2024, 6:12 AM

Rebase and implement requested changes

angelika edited the summary of this revision. (Show Details)Oct 29 2024, 7:00 PM

angelika added a parent revision: D13802: [lib] Make sure PeerToPeerMessageHandler is run after persisted state is read.

Harbormaster completed remote builds in B32422: Diff 45449.Oct 29 2024, 7:28 PM

kamil accepted this revision.Oct 30 2024, 3:08 AM

kamil added inline comments.

lib/tunnelbroker/peer-to-peer-message-handler.js
109 ↗	(On Diff #45449)	shouldn't we memoize it?

This revision is now accepted and ready to land.Oct 30 2024, 3:08 AM

ashoat added inline comments.Oct 30 2024, 4:45 AM

lib/tunnelbroker/peer-to-peer-message-handler.js
109 ↗	(On Diff #45449)	I don't think there's any point to memoizing this. There are two reasons for memoizing something in React using `useMemo`: (More important reason) to avoid regenerating non-primitives. React does a lot of comparisons to determine if things change, so if you're generating a new object, array, etc. each time, you'll force React to re-render a bunch of things If you have an expensive calculation, memoization can let you avoid doing it multiple times unnecessarily.