Switch backing store for callback-array to an immutable structure so it cannot change as we are enumerating over it.#61671
Conversation
…it cannot change as we are enumerating over it.
| /// The callbacks that want to be notified when our <see cref="TextViews"/> change visibility. Stored as an | ||
| /// <see cref="ImmutableHashSet{T}"/> so we can enumerate it safely without it changing underneath us. | ||
| /// </summary> | ||
| public ImmutableHashSet<Action> Callbacks = ImmutableHashSet<Action>.Empty; |
There was a problem hiding this comment.
The actual line this impacts is https://github.com/dotnet/roslyn/pull/61671/files#diff-79b25f4e4cd83abb8562b182bca60ef8c82335a76f2735897bcd644f82dd2b73R184, where we enumerate the set.
jmarolf
left a comment
There was a problem hiding this comment.
Can we add a test where we enumerate the collection that would fail before this change?
|
Sure. Let me write a unit test that directly interacts with this. |
| /// The callbacks that want to be notified when our <see cref="TextViews"/> change visibility. Stored as an | ||
| /// <see cref="ImmutableHashSet{T}"/> so we can enumerate it safely without it changing underneath us. | ||
| /// </summary> | ||
| public ImmutableHashSet<Action> Callbacks { get; private set; } = ImmutableHashSet<Action>.Empty; |
There was a problem hiding this comment.
❔ Do we care about order here? Normal events always and HashSet<T> often preserves "registration order is callback order", but ImmutableHashSet<T> will not.
There was a problem hiding this comment.
Good question. I hadn't ever thought it was relevant, but I'm ok adding that constraint.
There was a problem hiding this comment.
For now, i'm ok with this as is. We coudl def make this an array in teh future though if we think it's appropriate.
Fixes AB#1545234
There's been an intermittent stability issue that was caused by the view-visibility work i added a while back. The core issue here is that we were enumerating a mutable collection of callbacks, invoking them during iteration. It was possible for the callback to then perform work that would cause a mutation of the collection being iterated.
An example of this was a callback that set a TaskCompletionSource to completed. When completed, TPL could inline execution of the continuations of that task onto that thread, and those continuations could then register/unregister callbacks.
Fix here is to switch to immutable collections so that iteration is always safe. (Another option was to make a copy of the collection while iterating, but i felt this was cleaner).