Bug Fix: Synchronization Issues in MergeManyChangeSet operators

[dwcullop]

Description

• Fixes issue where the resulting ChangeSet Observable from MergeManyChangeSets contains more items than expected when a parent is removed around the time that an item from its child container is removed in a multi-threaded situation
• Cleans up code for each implementation so they are in sync for easier future maintenance
• MergeChangeSets operators are not affected because they have to deal with the removal of parent items

Scenario

(This is just a theory, but it explains why the current implementation is experiencing the issue described.)

Thread A removes the parent item from the source collection (cache/list) while Thread B removes an item from that same item's child collection.

This creates a race condition that results in that child item never being removed from the downstream changeset.

Sequence:

1. Thread A removes an item from the parent list. Inside of the lock, MergeMany starts to unsubscribe from changes to that item's child container.
2. Thread B removes an item to that same child's list. The change is behind the same lock, so it is blocked.
3. Thread A continues, completes the unsubscribe, and releases the lock (which activates Thread B), and then proceeds to the OnItemRemoved handler.
4. Thread B continues with the remove message is already in flight. It acquires the lock and removes the item from the operator's cached version of the child container. Since MergeMany has released its subscription to changes from this container, the removal does not get reflected downstream. The lock is then released.
5. Thread A continues with its OnItemRemoved handler but has to wait on the lock (which was just grabbed by Thread B). Once acquired, the parent item removal process is triggered, which emits a changeset that removes all the child items (using the locally cached copy of that parent item's container, but that copy no longer has the item that was just removed by Thread B).

Expected Result:

When a parent item is removed from the source changeset, the downstream changeset should see a changeset that removes all of the children added from that parent.

Actual Result:

Because the item was removed between the unsubscribe from the MergeMany and the OnItemRemoved handler, both chances to emit the Remove change were missed and so it is never observed downstream which causes the result changeset to contain an extra item.

Solution Details

The MergeMany and OnItemRemoved need to be atomic together (not separately), so that another thread can't slip between them and cause a remove change to be missed. This can be achieved by adjusting the placement of Synchronize calls so that those operations are handled together.

[ChrisPulman]

Somehow there's a difference of 103 failing the tests now.

[dwcullop]

Somehow there's a difference of 103 failing the tests now.

Yeah, the order of operations changed which broke some things. I've updated things to use the original order of operations and now all of the tests are passing.

[ChrisPulman]

Its failed on the stress test again 🤯

[dwcullop]

Its failed on the stress test again 🤯

Yeah, but I can't repro it. If I have to, I'll disable that test before I go on vacation and fix it when I get back. This PR is still an improvement over not having these changes.

[ChrisPulman]

Same stress test issue, 1 missing 😕

[ChrisPulman]

Successful 👏👏 we can come back to fix the test

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.