-
Notifications
You must be signed in to change notification settings - Fork 562
feat(tree) asynchronous transactions #26233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds alpha-level support for asynchronous transactions in the tree package, allowing transaction bodies to include asynchronous operations while maintaining transaction atomicity.
Changes:
- Adds
runAsyncTransactionAPI toTreeBranchAlphainterface with two overload signatures for handling transactions with typed success/failure values or void returns - Updates transaction infrastructure to track whether transactions are synchronous or asynchronous, preventing async transactions from starting within sync transactions
- Modifies transaction commit/abort handling to support rebasing and view updates when commits occur during async transactions
- Adds comprehensive test coverage for async transaction behaviors including nesting, interleaved commits, and enrichment
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md | Exposes new runAsyncTransaction API overloads in the public alpha API |
| packages/dds/tree/api-report/tree.alpha.api.md | Exposes new runAsyncTransaction API overloads in the tree package alpha API |
| packages/dds/tree/src/simple-tree/api/tree.ts | Defines TypeScript interface for runAsyncTransaction with detailed JSDoc documentation describing async transaction semantics |
| packages/dds/tree/src/shared-tree/schematizingTreeView.ts | Implements runAsyncTransaction by refactoring transaction mounting/unmounting into reusable methods |
| packages/dds/tree/src/shared-tree-core/transaction.ts | Core implementation changes: tracks async state in transaction stack frames, validates sync/async nesting, and handles view updates during commit/abort |
| packages/dds/tree/src/shared-tree/treeCheckout.ts | Updates transaction callbacks to apply view updates when async transactions complete with rebased changes |
| packages/dds/tree/src/shared-tree/tree.ts | Updates runTransactionInCheckout to explicitly start synchronous transactions |
| packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts | Adds comprehensive tests for async transaction behavior, nesting scenarios, and enrichment |
| packages/dds/tree/src/test/shared-tree/sharedTree.spec.ts | Tests async transactions with interleaved remote commits (committed and aborted cases) |
| packages/dds/tree/src/test/shared-tree-core/transaction.spec.ts | Tests transaction stack validation that prevents async transactions within sync transactions |
| Multiple test files | Updates all existing transaction.start() calls to transaction.start(false) for explicit synchronous transaction behavior |
| * for the outermost transaction which includes all inner transactions. | ||
| * - Undo will undo the outermost transaction and all inner transactions. | ||
| */ | ||
| runAsyncTransaction<TSuccessValue, TFailureValue>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think runTransactionAsync would be more conventional - even though the way that it is currently is how I'd say it most naturally in my head as a ST dev. Also, it has a subtly different meaning, perhaps. Is an "async transaction" a concept unique from a non-async transaction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, subtle. I don't have a strong opinion. Will make the change.
| public start(isAsync: boolean): void { | ||
| this.ensureNotDisposed(); | ||
| const onPushCurrent = hasSome(this.#stack) ? getLast(this.#stack).onPush : this.#onPush; | ||
| const last = getLast(this.#stack); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: getLast does not provide much value if not being used in combination with hasSome - but I guess it saves a few characters over this.#stack[this.#stack.length - 1]. Someday we'll be able to use at(-1)... 😵💫
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my eyes bleed a little every time I see stuff like this.#stack[this.#stack.length - 1].
Using hasSome proved cumbersome for the logic here, but I admittedly didn't search very long for a factoring where it might not.
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Description
Adds
@alphasupport for asynchronous transactions.Future Work
Utilities to help application authors detect interleaved transactions.
Breaking Changes
None