SynchronizedCollection & LINQ vs thread-safety

W bibliotece System.ServiceModel.dll znajdziemy ciekawą klasę SynchronizedCollection<T>. Szybkie zerknięcie w Reflectora:

i nad łysiejącą łepetyną zapaliła się żaróweczka: przecież to idealna klasa do cache dostępnego z różnych wątków aplikacji eliminująca ciągłe pisanie lock!

W takim przypadku od stanu wymyślone do stanu zrobione potrzeba jedynie kilkudziesięciu uderzeń w klawiaturę:

  1:  public class SampleCache<T>
  2:  {
  3:  	private readonly IList<T> _items = new SynchronizedCollection<T>();
  4:  
  5:  	public ICollection<T> GetAll()
  6:  	{
  7:  		return new ReadOnlyCollection<T>(_items);
  8:  	}
  9:  
 10:  	public void Add(T item)
 11:  	{
 12:  		_items.Add(item);
 13:  	}
 14:  }

To oczywiście tylko przykład, ale idea jest jasna: możemy dodać element do cache (w celu jego inicjalizacji) oraz pobrać wszystkie umieszczone tam elementy. Pobranej kolekcji raczej nie powinno się modyfikować z zewnątrz, stąd ReadOnlyCollection<T>.

No i dobra, wszystko sobie działało bez problemu. Po kilku miesiącach w obszarze wykorzystującym taki prowizoryczny cache wyskoczył jednak wyjątek, którego teoretycznie być nie powinno:

System.InvalidOperationException: Collection was modified; enumeration operation may not execute.

Hę?

Ale wkrótce wszystko stało się jasne, ponieważ kod który wygenerował ów wyjątek (JEDEN RAZ podczas ponad pół roku tworzenia systemu) wyglądał analogicznie do tego:

  1:  var searchResult = cache.GetAll().Where(...);

Spójrzmy zatem w Reflectora jeszcze raz:

"Przeciez jest lock!" -> takie głupie zdanie z mego otworu gębowego się wyrwało. Czemu głupie? Po kilku sekundach konfuzji dotarło do mnie, że przecież po to Anders wprowadził yield, aby kod z Where(…) został wykonany PO wyjściu z tego bloku.

OK, zatem teoretycznie poznałem przyczynę błędu. Ale skoro wystąpił on RAZ przez X miesięcy to skąd mam wiedzieć że się nie mylę? Może to coś zupełnie innego?

Potrzebny jest test jednostkowy, który wykaże problem przy wykonywaniu zapytań LINQ z wielu wątków nawet na kolekcji tak otoczonej lockami jak to tylko możliwe! Okazało się, że napisanie takiego testu nie było wcale… banalne. Ale po jakimś czasie kombinowania się udało, czego wynik z satysfakcją prezentuję:

  1:  [TestFixture]
  2:  public class SampleCacheTests
  3:  {
  4:  	public interface ITestCacheItem
  5:  	{
  6:  		Guid Id { get; }
  7:  	}
  8:  
  9:  	private ManualResetEvent _idFetched;
 10:  
 11:  	[SetUp]
 12:  	public void SetupResetEvent()
 13:  	{
 14:  		_idFetched = new ManualResetEvent(false);
 15:  	}
 16:  
 17:  	private ITestCacheItem NewItem()
 18:  	{
 19:  		var item = MockRepository.GenerateMock<ITestCacheItem>();
 20:  
 21:  		item
 22:  			.Stub(x => x.Id)
 23:  			.Return(Guid.NewGuid())
 24:  			.WhenCalled(x =>
 25:  				{
 26:  					// signal that search in progress
 27:  					_idFetched.Set();
 28:  					// give control to bg thread
 29:  					Thread.Sleep(0);
 30:  				});
 31:  
 32:  		return item;
 33:  	}
 34:  
 35:  	[Test]
 36:  	public void WhenQuerying_IsThreadSafe()
 37:  	{
 38:  		var cache = new SampleCache<ITestCacheItem>();
 39:  
 40:  		// fill cache with some items
 41:  		cache.Add(NewItem());
 42:  		cache.Add(NewItem());
 43:  		var lastItem = NewItem();
 44:  		cache.Add(lastItem);
 45:  
 46:  		// register another item on separate thread to modify cache's inner state
 47:  		ThreadPool.QueueUserWorkItem(delegate
 48:  			{
 49:  				// wait until signaled that search in progress
 50:  				_idFetched.WaitOne();
 51:  
 52:  				cache.Add(NewItem());
 53:  			});
 54:  
 55:  		// issue a query
 56:  		var searchResult = cache.GetAll().Where(x => x.Id == lastItem.Id).SingleOrDefault();
 57:  
 58:  		Assert.AreEqual(lastItem, searchResult);
 59:  	}
 60:  }

Git – test wykłada się z takim samym wyjątkiem.

Rozwiązanie tego problemu jest dość proste, wystarczy zwrócić KOPIĘ danych, nową dla każdego wywołania:

  1:  public ICollection<T> GetAll()
  2:  {
  3:  	return new List<T>(_items);
  4:  }

No i kwestia ostatnia: przecież już na samym początku zwracaliśmy nową ReadOnlyCollection, dlaczego więc błąd tak czy siak wystąpił? Ano dlatego:

Akurat ta implementacja kolekcji nie kopiuje nigdzie elementów, po prostu przekazuje część wywołań do oryginalnej instancji.

Dość długaśny opis dość interesującego przypadku dobiegł końca. W skrócie: pamiętajcie, zapytania LINQ nie są thread-safe, i sekcja lock nie ratuje sytuacji!

EDIT
Upomniany przez Gutka uściślam ostatnie zdanie: chodzi oczywiście o wykonanie zapytań LINQ na kolekcjach przechowywanych w pamięci, czyli powodujących iterowanie po poszczególnych elementach zwracanych przez implementację interfejsu IEnumerable.