Use the queue instead of processing operations in a loop.
https://linear.app/comm/issue/ENG-9470/mitigate-risks-of-effects-running-on-outdated-data
Details
Details
Disabled inbound messages processing by commenting out handleOlmMessageToDevice in usePeerToPeerMessageHandler.
On one device, created a thread, changed its name, and then its description. These operations weren't processed on another device. Refreshed the app so that the operations are read from the DB and processed. Made sure that the result is correct.
Checked what was the behavior before this diff. Surprisingly, it was the same. Using the queue here sounds more correct and feels safer, but the previous approach also works correctly.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
I feel like we duplicating logic here now, having two queues, the best solution here will be to use only useActionsQueue and get rid off
const [messagesQueue, setMessagesQueue] = React.useState<
$ReadOnlyArray<{
+peerToPeerMessage: PeerToPeerMessage,
+messageID: string,
+localSocketSessionCounter: number,
}>,
>([]);The only change will be to update tunnelbrokerMessageListener, instead of calling setMessagesQueue should call enqueue. Looks easy, but I guess there is some complexity so curious about @tomek's perspective, and requesting changes to discuss this.
BTW the initial pseudocode for useActionQueue suggested in https://phab.comm.dev/D13563#380765 was inspired by this very logic based on const [messagesQueue, setMessagesQueue] = React.useState...
| lib/tunnelbroker/peer-to-peer-message-handler.js | ||
|---|---|---|
| 173–199 | This could cause issues, we should start queueing messages from TB from the very start - the fix should be easy because we don't care about messages order anymore | |