It is possible that we need an updated state when processing the next operation, so we need to make sure that the new state is propagated.
https://linear.app/comm/issue/ENG-9470/mitigate-risks-of-effects-running-on-outdated-data
Details
Details
Tested that this diff doesn't introduce a regression:
- Receiving an edit entry operation before create entry
- Receiving a change thread settings operation before create thread
- Receiving a change thread subscription before adding as a member
Each of these produces the correct state and clears the Redux queue.
Also tested that we have a bug that this diff fixes. Created three operations that were processed in the following order:
- Change thread name at timestamp T+1
- Change thread description at T+1
- Create thread at T
Before this diff, there were robotexts displayed for each of these, but the thread name wasn't changed. After this diff both thread name and description are updated.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- master
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
There seem to be new risks from this approach, It's unclear to me what issue is being mitigated here. See my message from chat:
I'm not sure this has the same risk. it looks like DMOpsQueueHandler doesn't dequeue from a ref, and instead it dequeues from a useSelector result. doesn't that mean that newly-enqueued operations won't be processed by old effects?
| lib/hooks/actions-queue.js | ||
|---|---|---|
| 20 ↗ | (On Diff #44823) | In the past, it seems like we would clear the operation from the queue even if processDMOperation threw an exception. It looks like this diff changes the behavior, and now the queue will be stuck if an operation is queued that consistently throws an exception. Was this change intentional? It seems risky |
| 35 ↗ | (On Diff #44823) | If you agree that we should avoid changing the behavior when queue operations throw exceptions, I'd consider moving the setQueue operation here. Given we dequeue here, I think it makes sense to remove here as well. If we want to match prior behavior, I think we could simply remove the item from the queue directly, instead of separating accessing it and then later removing it. |
| lib/shared/dm-ops/dm-ops-queue-handler.react.js | ||
| 74 ↗ | (On Diff #44823) | Should this be processItem now? |
Comment Actions
Replied on the chat, but also copying here for the documenting purposes
The problem with DMOpsQueueHandler is that we're processing all the operations waiting on one condition in a loop. In this loop we're using one bound state, so if the result of one operation depends on a result of another, they can we wont handle that correctly. Also, when we're updating a thread, we're using UPDATE_THREAD update that overrides the previous state. So my theory is that if we have e.g. add members and update thread settings operations queued, the result of the first one would be overriden by the second one. I haven't tested that theory though.
| lib/hooks/actions-queue.js | ||
|---|---|---|
| 20 ↗ | (On Diff #44823) | Good catch, it wasn't intentional. |
| 35 ↗ | (On Diff #44823) | Ok, that makes sense. |
| lib/shared/dm-ops/dm-ops-queue-handler.react.js | ||
| 74 ↗ | (On Diff #44823) | Yes! |
| lib/hooks/actions-queue.js | ||
|---|---|---|
| 35 ↗ | (On Diff #44823) | If we decide to remove the item directly, then we would need to replace isProcessing ref with a state, so that the effect fires after the processing is done. This has the downside of the possibility that the effect will be run because of another enqueue call before setIsProcessing was called (not 100% sure about this, though). A safer approach might be to simply wrap performAction call with try-finally block. What do you think? |
Comment Actions
Makes sense to me! Before landing, please:
- Validate that there is a concrete issue being solved with this
- Fill in the test plan