edit lock for transactions in callbacks#27285
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (129 lines, 3 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an edit-lock guard to prevent starting transactions from within nodeChanged / treeChanged event callbacks, aligning transaction entrypoints with existing “no edits during callbacks” rules and avoiding label-tree corruption.
Changes:
- Add an
editLock.checkUnlockedguard tomountTransactionto blockrunTransaction/runTransactionAsyncduring change-event listeners. - Add/extend tests covering sync/async transaction attempts inside listeners and the resulting broken-state behavior.
- Add a changeset documenting the new runtime restriction and migration guidance.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/dds/tree/src/shared-tree/treeCheckout.ts | Adds edit-lock check before mounting a transaction to prevent listener-time nested transactions. |
| packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts | Adds regression tests for sync/async transaction attempts inside nodeChanged listeners and documents broken-state outcome. |
| .changeset/tough-lies-say.md | Documents the new behavior, error message, and migration approach. |
| // Starting a transaction during a nodeChanged/treeChanged event is forbidden for the same | ||
| // reason direct edits are: it would push a label frame and (typically) commit edits while | ||
| // the tree's invariants are mid-flight. Without this guard, nested transactions started | ||
| // from a listener leak label frames into the running outer transaction's label tree |
| expectErrorDuringEdit({ | ||
| duringEdit: (view) => | ||
| view.runTransaction(() => { | ||
| view.root.number = 4; | ||
| }), | ||
| error: "Running a transaction is forbidden during a nodeChanged or treeChanged event", | ||
| }); |
| // Subsequent operations should all throw with the broken-state message. | ||
| assert.throws( | ||
| () => (view.root.number = 5), | ||
| validateUsageError(/invalid state/), | ||
| "direct edit after listener throw should be blocked", | ||
| ); | ||
| assert.throws( | ||
| () => | ||
| view.runTransaction(() => { | ||
| view.root.number = 6; | ||
| }), | ||
| validateUsageError(/invalid state/), | ||
| "runTransaction after listener throw should be blocked", | ||
| ); |
| --- | ||
| Starting a transaction inside a tree-change event listener now produces a usage error | ||
|
|
||
| Calling `runTransaction` or `runTransactionAsync` from inside a `nodeChanged` or `treeChanged` event listener now throws a `UsageError`. |
|
|
||
| Before this change, starting a transaction from a listener would push a transaction frame onto the running outer transaction's bookkeeping before the inner edit was attempted, | ||
| leaving the tree's transaction labels in a corrupted state even when the inner edit itself threw. | ||
| The new check rejects the call before any state mutation, so the tree's transaction labels remain consistent. |
There was a problem hiding this comment.
Doesn't this leave the tree in a broken state, so it's impossible for it to matter if the transaction labels are consistent or not?
Might be better to rephase this to something like:
| The new check rejects the call before any state mutation, so the tree's transaction labels remain consistent. | |
| The updated code throws a `UsageError` before it can corrupt anything. |
| }); | ||
|
|
||
| // After — schedule the transaction outside the listener | ||
| Tree.on(view.root, "nodeChanged", () => { |
There was a problem hiding this comment.
I don't think we should recommend this approach to people. Its bad for a few reasons:
- If you do edits in response to document changes, its very easy to end up with edits triggering edits in a loop (on one client or between multiple clients) resulting in infinite edits, consuming unbounded bandwidth, storage, battery life CPU etc.
- Its easy to end up with multiple edits, one from each client connected, fighting over making the change (even if all but the first are no-ops due to merge resolution, its inefficient)
- It can lead to screwy attribution as the document show up last changed by a user who did not personally make the change
- It can screw up undo/redo, as it creates a change by the current user, that was not trigger by any of that user's actual actions.
- Its async, so its possible the edit trying to be done is no longer valid (a change could have happened in-between), the view could have been disposed, etc. If you need to do a change asynchronously, one should use a branch instead of holding onto the view or nodes async (unless they setup invalidation for the values they are holding onto which this example does not do).
I think we can simply say applications should not make edits in response to edits.
| After this error is thrown, the affected tree view enters a broken state and subsequent operations on it will throw an `Invalid use of ... after it was put into an invalid state` error. | ||
| Discard the view and acquire a fresh one to recover. | ||
|
|
||
| #### Migration |
There was a problem hiding this comment.
What actually worked before? Could you make transactions, but they would crash if there were any edits in them (in which case they could only ne no-ops), or could you actually slip transactions with real edits inside of them thorough our validation before this? If only the no-op case worked, I don't think there is anything to migrate, simply don't make no-op transactions inside events.
Description
Adds an edit lock to transactions that happen within callbacks of treeChanged/nodeChanged events.