Page MenuHomePhabricator

[lib] Use the actions queue in the peer to peer context
ClosedPublic

Authored by tomek on Oct 11 2024, 10:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 10:14 PM
Unknown Object (File)
Thu, Nov 14, 9:49 PM
Unknown Object (File)
Wed, Nov 13, 8:06 PM
Unknown Object (File)
Wed, Nov 13, 12:59 PM
Unknown Object (File)
Wed, Nov 13, 8:59 AM
Unknown Object (File)
Wed, Nov 13, 7:29 AM
Unknown Object (File)
Wed, Nov 13, 7:02 AM
Unknown Object (File)
Wed, Nov 13, 6:37 AM
Subscribers
None

Details

Summary

In D13563 we've introduced a regression described in https://linear.app/comm/issue/ENG-9470/mitigate-risks-of-effects-running-on-outdated-data#comment-48bb85bb which caused sending messages to sometimes stuck. This diff replaces the approach with a queue.

https://linear.app/comm/issue/ENG-9470/mitigate-risks-of-effects-running-on-outdated-data

Test Plan

Sent a lot of messages in quick successions and haven't encountered any issues.
Tested leaving thick threads and the original issue still reproduces - looking for a solution.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/tunnelbroker/peer-to-peer-context.js
306 ↗(On Diff #45095)

The fact that this callback depends on sendPushNotifs makes it so the notifs processing still uses an outdated store.

lib/tunnelbroker/peer-to-peer-context.js
306 ↗(On Diff #45095)

If necessary, we can use a "step-based approach" where we wait for the appropriate data to appear in Redux that would make us confident that sendPushNotifs is up-to-date. This sort of approach is complicated to get right, but we use it in login-hooks.js and registration-server-call.js, and previously used it in relationship-hooks.js before D13496 (see D13443 for original approach). If we take this approach, we should make sure to implement a timeout so that if the data never appears in Redux, we still eventually reject the returned promise

Delete unnecessary dependencies

lib/tunnelbroker/peer-to-peer-context.js
306 ↗(On Diff #45095)

Actually, this works correctly, and the fact that sendPushNotifs throws doesn't indicate that our solution is incorrect. When we leave a chat, it is expected that after processing the operation, the thread and its members aren't present in the store.

The issue here is quite different: when we reach this code, we should somehow know to whom we should send the notifs. It might be too late to read this from the store, so we should find an alternative way of passing this information.

This might be a bit complicated because when we add a new member to a thread, we should use the state from after the processing. And when we leave a thread, we should use the state from before.

lib/tunnelbroker/peer-to-peer-context.js
248 ↗(On Diff #45175)

Simplify the queueing code. This revision still has an issue where leaving a thread casues an exception while sending the notifs. Regardless, it is an improvement comparing to the master, because it fixes sending messages in quick succession. The notifs issue will be fixed in the next diff.

Revert actions queue change

ashoat removed a reviewer: inka. ashoat added 1 blocking reviewer(s): kamil.

I didn't notice any issues, but I'm not very familiar with this logic – would appreciate if @kamil could also take a look

This revision is now accepted and ready to land.Tue, Oct 29, 4:44 AM