concurrent array editing and iteration error hadling #21760

daesunp · 2024-07-02T22:36:39Z

Description

This PR adds a custom values() function to arrayNode so that we throw an error when we concurrently iterate and edit the array.

daesunp · 2024-07-02T22:40:11Z

@Josmithr Since we will now error when concurrently iterating and editing the array, would this be considered a breaking change? Just wanted to double check with someone from the API council :)

Josmithr · 2024-07-02T22:44:23Z

@Josmithr Since we will now error when concurrently iterating and editing the array, would this be considered a breaking change? Just wanted to double check with someone from the API council :)

Strictly speaking, yes. If we allowed a usage pattern before, and no longer allow it, then it's breaking. That said, was the behavior well-defined before? Could users safely concurrently iterate and edit, or could that lead to unsafe behavior? If so, that might be a reasonable argument to make the change anyways. I would defer to @CraigMacomber or @taylorsw04 on that 🙂

Another way of putting it: if we are adding new safeguards to help identify potential existing issues in app authors' code, then this might be an okay change to make.

CraigMacomber · 2024-07-02T23:29:57Z

@Josmithr Since we will now error when concurrently iterating and editing the array, would this be considered a breaking change? Just wanted to double check with someone from the API council :)

Strictly speaking, yes. If we allowed a usage pattern before, and no longer allow it, then it's breaking. That said, was the behavior well-defined before? Could users safely concurrently iterate and edit, or could that lead to unsafe behavior? If so, that might be a reasonable argument to make the change anyways. I would defer to @CraigMacomber or @taylorsw04 on that 🙂

Another way of putting it: if we are adding new safeguards to help identify potential existing issues in app authors' code, then this might be an okay change to make.

This change may impact users (even if we consider those users unsupported) in a way they would like to know about, so it should get a changeset explaining its impact (even if we consider it non breaking: this is a useful feature we should document and get included in release notes)

This impacts an API, so yes, API council review is appropriate.

As for is this a breaking change and thus not allowed to be merged without a major version bump? We do not have official policy about how to make that call. The closest I know is my draft document. I think the most relevant case is this one about undocumented behaviour https://github.com/microsoft/FluidFramework/pull/14421/files#diff-c45b354bea51ccd8727afb1db811eb0764306f87f21982d9199cd04e633098a5R62

My opinion (and that's not what matters, but perhaps API council will agree), is that the documentation we inherit from ReadonlyArray (which is where we get the iteration related methods) is not truly clear about this.

JavaScript arrays are clear on the behavior of their iteration methods. If someone relies on that behavior, they should be doing array.prorotype.values.call(node) (which is unaffected by your change) since nothing guarantees implementations of ReadonlyArray exactly match how arrays behave: they only have to match arrays up to the details covered in that type.

ReadonlyArray.values is only documented as:

    /**
     * Returns an iterable of values in the array
     */
    values(): IterableIterator<T>;

This does not clarify how this behaves across edits.

The other methods seem similar. I didn't check ReadonlyArray[Symbol.iterator] as my IDE provides no way to find definitions or docs for members which are indexed, but I suspect tis about the same as well.

Therefor I think this case fall under undocumented behavior, and thus is allowed to change and be considered non breaking, for supported customer (which requires them to not rely on undocumented behavior), and thus can be merged.

CraigMacomber · 2024-07-02T23:33:18Z

packages/dds/tree/src/simple-tree/arrayNode.ts

@@ -850,6 +854,23 @@ abstract class CustomArrayNodeBase<const T extends ImplicitAllowedTypes>
 			destinationFieldPath,
 			index,
 		);
+		this.#generationNumber += 1;


this is unneeded. Performing an edit sends a change event.

Suggested change

this.#generationNumber += 1;

Suggested change

this.#generationNumber += 1;

this.#generationNumber += 1;

Updated with suggested change

CraigMacomber · 2024-07-02T23:34:22Z

packages/dds/tree/src/simple-tree/arrayNode.ts

@@ -672,6 +672,10 @@ abstract class CustomArrayNodeBase<const T extends ImplicitAllowedTypes>

 	protected abstract get simpleSchema(): T;

+	#generationNumber: number = 0;


This field should have a doc comment explaining why it exists and how its used.

Updated field with doc comment

CraigMacomber · 2024-07-02T23:41:42Z

packages/dds/tree/src/simple-tree/arrayNode.ts

@@ -672,6 +672,10 @@ abstract class CustomArrayNodeBase<const T extends ImplicitAllowedTypes>

 	protected abstract get simpleSchema(): T;

+	#generationNumber: number = 0;
+	#updateGenerationNumber = getFlexNode(this).on("nodeChanged", () => {


I think there are two issues here:

#updateGenerationNumber is undocumented, and its name is confusing. This is a function, but calling it does not update the generation number. Calling this unregisters generation number updates! Consider doing this in the constructor. If you really want to do it during property static initialization time, use a static initialization block https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Static_initialization_blocks , or save it to a property with a more clear name if you actually do need to unregister at some point (I don't think we do)

I think this might not handle unhydrated nodes correctly. Using Tree.on instead of the flex tree APis should fix that as I believe that workes correctly in this case. Ensure you have a test which creates a node thats unhydrated, then as a second step inserts it into the tree. Make sure that case still works correctly, even though the underlying flex tree dummy node gets replaced with a real one (which would break your events here)

I have tried to use a static initialization block, but unfortunately due to how scoping works with them, I couldn't get them to work. I have also tried to create a constructor within the class and initialize it there (after calling super()), but it wasn't really obvious where the constructor of the inherited class was.

As for the use of flex tree APIs, I have tried using Tree.on which led to a cyclic dependency, so I left it as is and added testing for unhydrated nodes and checked that they passed.

Would there be any other test cases where using the flex tree apis, may lead to errors that I should add test coverage for?

Updated to be initialized in the constructor without storing it into a variable

CraigMacomber · 2024-07-02T23:42:37Z

packages/dds/tree/src/test/simple-tree/arrayNode.spec.ts

@@ -590,4 +590,28 @@ describe("ArrayNode", () => {
 			);
 		});
 	});
+
+	describe("Iteration", () => {
+		it("Concurrently iterating and editing should throw an error.", () => {


When testing that something throws, you want to put as little as possible in the assert.throws block so you can test that the correct code throws the assert, and not the constructor , hydrate etc.

Removed unnecessary code from assert.throws block

CraigMacomber · 2024-07-02T23:44:14Z

packages/dds/tree/src/test/simple-tree/arrayNode.spec.ts

+	describe("Iteration", () => {
+		it("Concurrently iterating and editing should throw an error.", () => {
+			assert.throws(
+				// False positive


I'm confused by this comment. To me this implies that the error detection here is a false positive, meaning its detecting an error when it should not. This case however should (and if this test passes) does error, so true positive for the detection.

removed incorrect comment

noencke · 2024-07-08T18:26:18Z

packages/dds/tree/src/simple-tree/arrayNode.ts

+	public *values(): Generator<TreeNodeFromImplicitAllowedTypes<T>> {
+		const initialLastUpdatedStamp = this.#generationNumber;
+		let index = 0;
+		while (true) {


You can have a for-loop that operates over two variables rather than one, if you prefer (and if the linter prefers - in the past it has complained about while (true) loops, though I'm not sure if that's still the case). IMO the for loop makes it more clear that the two variables are progressing together in lockstep.

Suggested change

while (true) {

for (let i = 0, value = this.at(0); i < this.length && value !== undefined; i++, value = this.at(i)) {

Though, I am curious - under what circumstances is value undefined? If we only iterate up to length, and we know that length isn't going to fluctuate during the iteration, then aren't we guaranteed that value is always defined? Since we don't allow undefined in our arrays as legitimate elements.

The reasoning behind the while(true) was that if we want to throw an error when we detect that the array has been edited during the iteration.

For example (one of the test cases in this PR), if we used the for-loop above, and deleted a bunch of the array elements during the iteration, the this.length will have changed and value could be undefined. This unfortunately exits the for loop instead of making the check and throwing the error within the loop.

Would it be better to store the "original" array length into a local variable in the values method, and using this length in the for-loop while checking value === undefined and i >= this.length (which could be different from the original array length) and throw the error there instead of using the while(true) loop?

I see, interesting. Yes, I suppose you could save the array length. I think you could also just be a little more careful about where you do the assert. The only time that the concurrent modification can happen is at the yield statement. So, instead of checking for the modification at the start of the loop, do it right after the yield. Then, you can write your loop however you want (e.g. as a for-loop, even using this.length) and you'll be fine.

One other thing this made me realize is that the generator code only runs lazily when the first item from the generator is requested. So, the baseline generation number that you save in initialLastUpdatedStamp is not necessarily the generation number when values() is called. Rather, it is the generation number when the iterator returned by values() is first read. So, a user could e.g. do this:

const values = myArray.values(); myArray.removeRange(); for (const v of values) { // ... No error }

Basically, it allows modifications if the user has created, but not read from, the generator. Is that... okay? Seems maybe inconsistent to me? If you want to prevent that, you need to have values() not be a generator, but set initialLastUpdatedStamp and then pass it to another iterator (which could be a generator).

Updated to for-loop for readability, added separate *generateValues method to pass in the correct generationNumber, and added test case for calling values, editing the array, and iterating

packages/dds/tree/src/simple-tree/arrayNode.ts

msfluid-bot · 2024-07-08T23:21:32Z

⯅ @fluid-example/bundle-size-tests: +1.73 KB

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	457.12 KB	457.16 KB	⯅ +35 Bytes
azureClient.js	555.1 KB	555.14 KB	⯅ +49 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	258.33 KB	258.35 KB	⯅ +14 Bytes
fluidFramework.js	391.41 KB	392.17 KB	⯅ +776 Bytes
loader.js	134.1 KB	134.11 KB	⯅ +14 Bytes
map.js	42.17 KB	42.17 KB	⯅ +7 Bytes
matrix.js	145.43 KB	145.44 KB	⯅ +7 Bytes
odspClient.js	522.86 KB	522.91 KB	⯅ +49 Bytes
odspDriver.js	97.17 KB	97.19 KB	⯅ +21 Bytes
odspPrefetchSnapshot.js	42.27 KB	42.29 KB	⯅ +14 Bytes
sharedString.js	162.51 KB	162.52 KB	⯅ +7 Bytes
sharedTree.js	381.87 KB	382.63 KB	⯅ +769 Bytes
Total Size	3.26 MB	3.27 MB	⯅ +1.73 KB

Baseline commit: 381a32f

Generated by 🚫 dangerJS against 40fb3a0

noencke · 2024-07-09T17:58:20Z

packages/dds/tree/src/simple-tree/arrayNode.ts

+	 * This function is only created to initialize the event listener to increment the generationNumber.
+	 * However, it should not be called anywhere else, as it will unregister generation number updates.
+	 */
+	#initializeGenerationNumberEvent = getFlexNode(this).on("nodeChanged", () => {


If I understand the doc comment correctly, this is an event registration that is never unregistered, right? Why have the initializeGenerationNumberEvent variable at all, then?

Added constructor to CustomArrayNodeBase and removed initializeGenerationNumberEvent variable

Craig mentioned that we should double check with you and add some comments justifying that

This would not cause issues because of how registration goes for raw to flex nodes (I did add some test cases with unhydrated nodes)

Never unregistering won't cause any memory leaks

Do you happen to know if there would be anything we should mention in the comments for calling this in the constructor?

getFlexNode(this).on("nodeChanged", () => { this.#generationNumber += 1; });

As of a few weeks ago we will copy events from unhydrated nodes to hydrated upon insertion, so that should be okay. As for unregistering, I believe the registration will live so long as the anchornode stays alive. I don't think we have a way to dispose (in practice) flex nodes currently. If we did, you'd want to do the deregistration there. But, since we're about to do a lot of work to change the flex layer, I wouldn't worry about it here.

noencke · 2024-07-09T18:02:13Z

packages/dds/tree/src/simple-tree/arrayNode.ts

+			throw new UsageError(`Concurrent editing and iteration is not allowed.`);
+		}
+		for (
+			let i = 0, value = this.at(0);


Is it safe to call this.at(0) if the array is empty? Does it throw, or does it just return undefined?

I think thats not safe. We should make sure we have and iteration test for the empty case.

It looks like the this.at(0) on an empty array returns undefined. I also added an iteration test for the empty case

noencke · 2024-07-09T18:03:00Z

packages/dds/tree/src/simple-tree/arrayNode.ts

+	private *generateValues(
+		initialLastUpdatedStamp: number,
+	): Generator<TreeNodeFromImplicitAllowedTypes<T>> {
+		if (initialLastUpdatedStamp < this.#generationNumber) {


nit:

Suggested change

if (initialLastUpdatedStamp < this.#generationNumber) {

if (initialLastUpdatedStamp !== this.#generationNumber) {

Updated with suggested change

noencke · 2024-07-09T18:04:33Z

packages/dds/tree/src/simple-tree/arrayNode.ts

+		for (
+			let i = 0, value = this.at(0);
+			i < this.length && value !== undefined;
+			i++, value = this.at(i)


Suggested change

i++, value = this.at(i)

value = this.at(++i)

Updated with suggested change

noencke · 2024-07-09T18:07:27Z

packages/dds/tree/src/simple-tree/arrayNode.ts

+		}
+		for (
+			let i = 0, value = this.at(0);
+			i < this.length && value !== undefined;


Suggested change

i < this.length && value !== undefined;

i < this.length;

value will always be defined since we don't support sparse arrays, right?

I did remove the && value !== undefined, but typescript seems to be complaining in the for loop that we cannot yield undefined values (we can only return TreeNodeFromImplicitAllowedTypes<T> because of our return type).

I updated to use null assertions, but wasn't sure if this was the best way to do it. Would it be better if I added an assert statement in the for loop instead?

Discussed offline.

noencke · 2024-07-11T17:49:29Z

packages/dds/tree/src/simple-tree/arrayNode.ts

+		}
+		for (let i = 0; i < this.length; i++) {
+			yield this.at(i) ?? fail("Index is out of bounds");
+			if (initialLastUpdatedStamp < this.#generationNumber) {


Suggested change

if (initialLastUpdatedStamp < this.#generationNumber) {

if (initialLastUpdatedStamp !== this.#generationNumber) {

To match the other one

Updated with suggested change

noencke · 2024-07-11T17:54:49Z

packages/dds/tree/src/test/simple-tree/arrayNode.spec.ts

+			assert.deepEqual(result, []);
+		});
+
+		it("Iterates through the values of two different iterators", () => {


Suggested change

it("Iterates through the values of two different iterators", () => {

it("Iterates through the values of two concurrent iterators", () => {

Updated with suggested change

noencke · 2024-07-11T18:00:31Z

packages/dds/tree/src/test/simple-tree/arrayNode.spec.ts

+			values2.next();
+		});
+
+		it("Concurrently iterating and editing after inserting into an unhydrated array node errors.", () => {


I don't think that either of these "unhydrated" tests are actually testing anything interesting. Once the nodes are hydrated, they're hydrated. Whether they used to be unhydrated, or never existed in an unhydrated state, doesn't matter.

The interesting test would be to create an iterator for an unhydrated node, then hydrate it, and then check that the iterator keeps working. I expect that this will not work with the hydrate function, because our hydrate function doesn't actually hydrate unhydrated nodes :) I have a task to fix this but I've been unsuccessful so far.

In the meantime, you can use a real SharedTree from a TestTreeProvider to test this case. I'll go try to get hydration via hydrate working in the meantime now that I know we have an actual use for it.

Updated test case to:

create an unhydrated array node, create an iterator, initialize a tree with the unhydrated node (to hydrate it) and checks that the iterator works

make an edit to the node, and checks that it throws a usage error during iteration

## Description This PR adds a custom `values()` function to `arrayNode` so that we throw an error when we concurrently iterate and edit the array.

concurrent array editing and iteration error

eb2ebf4

daesunp requested a review from a team as a code owner July 2, 2024 22:36

daesunp assigned CraigMacomber Jul 2, 2024

github-actions bot added area: dds Issues related to distributed data structures area: dds: tree base: main PRs targeted against main branch labels Jul 2, 2024

daesunp assigned Josmithr Jul 2, 2024

CraigMacomber reviewed Jul 2, 2024

View reviewed changes

PR review

1fdde64

daesunp requested a review from a team as a code owner July 8, 2024 17:36

github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct public api change Changes to a public API labels Jul 8, 2024

noencke reviewed Jul 8, 2024

View reviewed changes

packages/dds/tree/src/simple-tree/arrayNode.ts Outdated Show resolved Hide resolved

daesunp added 2 commits July 8, 2024 22:34

Merge branch 'main' into concurrent-array-error-handling

edc04d8

PR review

bf4a168

PR review

64c0f54

noencke reviewed Jul 9, 2024

View reviewed changes

daesunp added 4 commits July 11, 2024 16:47

PR review

1b8709c

Merge branch 'main' into concurrent-array-error-handling

7033933

PR review

ff74f47

Merge branch 'main' into concurrent-array-error-handling

e41c024

noencke reviewed Jul 11, 2024

View reviewed changes

daesunp added 3 commits July 11, 2024 21:56

PR review

adb7a64

Add comment to test case

1427708

Added docstring for values()

40fb3a0

noencke approved these changes Jul 11, 2024

View reviewed changes

daesunp merged commit 6fd320c into microsoft:main Jul 11, 2024
30 checks passed

		@@ -672,6 +672,10 @@ abstract class CustomArrayNodeBase<const T extends ImplicitAllowedTypes>

		protected abstract get simpleSchema(): T;

		#generationNumber: number = 0;

	while (true) {
	for (let i = 0, value = this.at(0); i < this.length && value !== undefined; i++, value = this.at(i)) {

	if (initialLastUpdatedStamp < this.#generationNumber) {
	if (initialLastUpdatedStamp !== this.#generationNumber) {

	it("Iterates through the values of two different iterators", () => {
	it("Iterates through the values of two concurrent iterators", () => {

concurrent array editing and iteration error hadling #21760

concurrent array editing and iteration error hadling #21760

Conversation

daesunp commented Jul 2, 2024

Description

daesunp commented Jul 2, 2024 • edited Loading

Josmithr commented Jul 2, 2024 • edited Loading

CraigMacomber commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msfluid-bot commented Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daesunp Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

daesunp commented Jul 2, 2024 •

edited

Loading

Josmithr commented Jul 2, 2024 •

edited

Loading

msfluid-bot commented Jul 8, 2024 •

edited

Loading

daesunp Jul 11, 2024 •

edited

Loading