Details
Tested later in the stack but:
- Making sure that sending text messages or thread creation (or any other DM ops) to a peer that is not in the aux user store is delivered.
- Notifs works.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- land
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
When I tested a similar approach as part of my work on ENG-9269, I found that notifs didn't work correctly. The issue was that the call to processAndSendDMOperation had bound in an outdated version of AuxUserStore, that didn't include the new devices that are fetched inside of it.
To make this work correctly, I opted for an approach where I first insert the users into AuxUserStore, then wait in an effect here for AuxUserStore to be populated before calling processAndSendDMOperation. This guarantees that the call to processAndSendDMOperation has the latest version of AuxUserStore bound in.
An alternative approach would be to find a way to "drill" the necessary updated data (updatedPeers?) through to the notifs code that needs it.
| lib/shared/dm-ops/dm-op-utils.js | ||
|---|---|---|
| 159–162 ↗ | (On Diff #44594) | Are we concerned at all about the performance implications here? When I evaluated this approach here, I pointed out:
Is it possible that we find ourselves in a situation where one particular user is missing from the identity service, and we end up querying for them over and over before sending messages? |
| lib/tunnelbroker/peer-to-peer-context.js | ||
| 32 ↗ | (On Diff #44594) | Nit: this new type could've been introduced in a separate diff |
I did consider that this could have an impact on notifications, but after analyzing the code it should work, I also confirmed this by testing.
Here is what is going on here:
- useProcessDMOperation is called.
- We generated messages to peers here.
- If there are any peers missing we call and await useGetAndUpdateDeviceListsForUsers which here dispatch action to update AuxUserStore.
- Later, we create notificationsCreationData here but note that it is not using AuxUserStore at all.
- Then, we dispatch processDMOpsActionType here.
- As a result, this effect is triggered and call processOutboundMessages which send P2P messages, generates and send notifs.
Note that to correctly generate notifications we need updated AuxUserStore after point 6, which is the result of point 5 and it takes place after 3 when we're dispatching action to update AuxUserStore.
Could describe your approach in more detail? Maybe I am missing something but I think we don't need an updated AuxUserStore in processAndSendDMOperation
An alternative approach would be to find a way to "drill" the necessary updated data (updatedPeers?) through to the notifs code that needs it.
If my assessments above are falsy, and we need a different approach, this is a lot easier to implement than waiting for the updated AuxUserStore in code responsible for creating threads and sending text messages.
Requesting review to get some clarification before starting updating the code.
| lib/shared/dm-ops/dm-op-utils.js | ||
|---|---|---|
| 159–162 ↗ | (On Diff #44594) | I am aware of that, it's more of a corner case, I was planning to create a follow-up task on how to handle this after the stack is ready. I know this is not an ideal solution... I was trying to deliver this on time with the option to improve it, |
Thanks for explaining @kamil. It's good to hear that you've tested it.
Could describe your approach in more detail? Maybe I am missing something but I think we don't need an updated AuxUserStore in processAndSendDMOperation
The approach I tested was a little different. In my testing, I avoided changing processAndSendDMOperation – instead, I had a callback that would first call getAndUpdateDeviceListsForUsers, and then after it returned it would call processAndSendDMOperation. Do you know why this approach would lead to different results?
Note that to correctly generate notifications we need updated AuxUserStore after point 6, which is the result of point 5 and it takes place after 3 when we're dispatching action to update AuxUserStore.
Is there a risk that the updated AuxUserStore is not there in step 6? In my approach, this risk is mitigated because the effect knows to wait until a specific set of users are populated in AuxUserStore.
Will wait on a response to this before accepting :)
I did some testing on my own and confirmed that this approach works. I was able to significantly simplify useUpdateRelationships: D13496 :)
Would still love an answer to this question though:
Note that to correctly generate notifications we need updated AuxUserStore after point 6, which is the result of point 5 and it takes place after 3 when we're dispatching action to update AuxUserStore.
Is there a risk that the updated AuxUserStore is not there in step 6? In my approach, this risk is mitigated because the effect knows to wait until a specific set of users are populated in AuxUserStore.
Hmm, I don't have an idea, perhaps some missing await on getAndUpdateDeviceListsForUsers so processDMOpsActionType was dispatched before setPeerDeviceListsActionType?
I think not, because to start processing notifs we first need at least one render cycle to take this from queue here which is a result of processDMOpsActionType which is dispatched after setPeerDeviceListsActionType and we using synchronous dispatch, not e.g. disptachActionPromise.
But if we still afraid of this, we can always implement what you suggested here:
An alternative approach would be to find a way to "drill" the necessary updated data (updatedPeers?) through to the notifs code that needs it.
This is fairly easy to add, just to add missing data to processDMOpsActionType to put this on the queue along with generated P2P messages.
| lib/shared/dm-ops/dm-op-utils.js | ||
|---|---|---|
| 159–162 ↗ | (On Diff #44594) | created ENG-9424. |
| lib/tunnelbroker/peer-to-peer-context.js | ||
| 32 ↗ | (On Diff #44594) | fair point, moved to D13515 |