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

ObserverManager: fix Collection was modified #8707

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ScarletKuro
Copy link
Contributor

@ScarletKuro ScarletKuro commented Nov 4, 2023

If you add a subscription while Notify is in process of enumerating the observers it can lead to exception: System.InvalidOperationException Collection was modified; enumeration operation may not execute.

This problem was also mentioned here #8284 (comment) in 3rd paragraph

This can be checked with a simple unit test (without grains):

[Fact]
public async Task CollectionModified()
{
	// Arrange
	var observerManager = new ObserverManager<int, int>(TimeSpan.FromHours(1), NullLogger.Instance);

	for (var i = 0; i < 1000; i++)
	{
		observerManager.Subscribe(i, i);
	}

	bool Predicate(int observer)
	{
		// Using predicate for an easier simulation of inserting item while enumerating
		if (observer == 500)
		{
			observerManager.Subscribe(1001, 1001);
		}

		return true;
	}

	Task NotificationAsync(int observer) => Task.CompletedTask;

	 // Act
 	var ex = await Record.ExceptionAsync(()=> observerManager.Notify(NotificationAsync, Predicate));

	 // Assert
 	Assert.Null(ex);
}

I did not add this to the current tests, since we don't really have any dedicated ObserverManager tests and it's unclear in what namespace to add it.

I'm not fan that we are creating a new list, but this is the simplest way to fix it, I guess.

Microsoft Reviewers: Open in CodeFlow

@ReubenBond
Copy link
Member

This is an expensive approach. I wonder if we can come up with a cheaper one, possibly at the expense of complexity

@ScarletKuro
Copy link
Contributor Author

Using ConcurrentDictionary can also fix this, but it's probably even more costly than creating a copy list in Notify.

@KSemenenko
Copy link

@KSemenenko
Copy link

Using ConcurrentDictionary can also fix this, but it's probably even more costly than creating a copy list in Notify.

we need some BenchmarkDotNet tests to check performance.

Apparently, we can use lock here, but the question is about the 'notify' method, which is asynchronous and can take a long time to execute.
Maybe this method should be run somewhere in a parallel thread?
Or to make a simple lock to make a list of Tasks and then run them in batches?

@slawomirpiotrowski
Copy link

I think it's not that easy to come up with a benchmark as relationship beetween amount of reads and writes would change the result.

Speaking about complexity: it might be efficient to cache ToList result and invalidate that cache whenever any writes to the dictionary are applied.

BUT It would be nice to commit any fix for the error if finding out the fastest solution is not that easy. Any fix is better than current (throwing exceptions) code.

@jamescarter-le
Copy link
Contributor

This is still occurring in Orleans v8.1

@ScarletKuro
Copy link
Contributor Author

Benchmark, copying the observers vs ConcurrentDictionary:

| Method                | NumberOfObservers | Mean         | Error       | StdDev      | Rank | Gen0    | Gen1    | Gen2    | Allocated |
|---------------------- |------------------ |-------------:|------------:|------------:|-----:|--------:|--------:|--------:|----------:|
| NotifyAsyncList       | 100               |     698.2 ns |     7.65 ns |     7.16 ns |    1 |  0.0315 |       - |       - |    1624 B |
| NotifyAsyncConcurrent | 100               |   1,122.6 ns |     9.34 ns |     8.28 ns |    2 |       - |       - |       - |      64 B |
| NotifyAsyncList       | 1000              |   6,849.2 ns |   120.44 ns |   152.32 ns |    3 |  0.3128 |       - |       - |   16024 B |
| NotifyAsyncConcurrent | 1000              |  10,430.6 ns |    34.06 ns |    28.44 ns |    4 |       - |       - |       - |      64 B |
| NotifyAsyncConcurrent | 10000             | 103,772.6 ns |   812.36 ns |   759.89 ns |    5 |       - |       - |       - |      64 B |
| NotifyAsyncList       | 10000             | 132,063.2 ns | 1,317.01 ns | 1,231.94 ns |    6 | 49.8047 | 49.8047 | 49.8047 |  160041 B |

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants