Skip to content
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

fix(merge-tree sequence): rb tree crash #17395

Merged
merged 20 commits into from Oct 17, 2023

Conversation

connorskees
Copy link
Member

@connorskees connorskees commented Sep 20, 2023

Resolves a crash in our red-black tree implementation that occurs when multiple references slide off the tree at once.

ADO#4477

@connorskees connorskees requested a review from a team as a code owner September 20, 2023 17:25
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: sharedstring base: next PRs targeted against next branch labels Sep 20, 2023
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Sep 20, 2023

@fluid-example/bundle-size-tests: -723 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 442.37 KB 442.13 KB -241 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 238.34 KB 238.34 KB No change
loader.js 178.18 KB 178.18 KB No change
map.js 48.05 KB 48.05 KB No change
matrix.js 139.12 KB 138.89 KB -241 Bytes
odspDriver.js 90.2 KB 90.2 KB No change
odspPrefetchSnapshot.js 41.75 KB 41.75 KB No change
sharedString.js 159.86 KB 159.62 KB -241 Bytes
sharedTree2.js 266.29 KB 266.29 KB No change
Total Size 1.71 MB 1.71 MB -723 Bytes

Baseline commit: 1031b62

Generated by 🚫 dangerJS against aaea07a

@@ -114,6 +118,70 @@ describe("SharedString op-reentrancy", () => {
}
});

it("is empty after deleting reference pos in reentrant callback", () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think the work item here wasn't talking about op reentrancy. I interpreted it as issues with LocalReferencePosition.link and LocalReferenceCollection.removeLocalRef mutually calling each other, leading to issues.

Not really against these tests though, I guess they do add some coverage for actions taken as part of a sequence delta callback, which is a reasonable thing applications might do. I think we have some coverage of this type of thing at the merge-tree level but not really at the sequence level.

Copy link
Contributor

@Abe27342 Abe27342 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me. It'd be good to have Tony look at it before merging as he might recall some more of the subtleties with why we didn't originally link to undefined.

Copy link
Contributor

@Abe27342 Abe27342 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me. It'd be good to have Tony look at it before merging as he might recall some more of the subtleties with why we didn't originally link to undefined.

@github-actions github-actions bot added the public api change Changes to a public API label Oct 1, 2023
@connorskees connorskees requested a review from a team as a code owner October 17, 2023 20:16
@connorskees connorskees enabled auto-merge (squash) October 17, 2023 21:02
@connorskees connorskees merged commit fa7b085 into microsoft:next Oct 17, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: sharedstring area: dds Issues related to distributed data structures base: next PRs targeted against next branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants