I have the following F# types
type ThreadSafeEnumerator<'T>(inner: IEnumerator<'T>, readWriteLock: ReaderWriterLockSlim) =
do readWriteLock.EnterReadLock()
interface IEnumerator<'T> with
member __.Current: 'T = inner.Current
member __.Current: obj = inner.Current
member __.Dispose() =
inner.Dispose()
readWriteLock.ExitReadLock()
member __.MoveNext() = inner.MoveNext()
member __.Reset() = inner.Reset()
type ThreadSafeObject() =
let _lock = new ReaderWriterLockSlim()
member __.Lock = _lock
member __.EnterReadLock() = _lock.EnterReadLock()
member __.EnterWriteLock() = _lock.EnterWriteLock()
member __.ExitReadLock() = _lock.ExitReadLock()
member __.ExitWriteLock() = _lock.ExitWriteLock()
member __.EnterUpgradeableReadLock() = _lock.EnterUpgradeableReadLock()
member __.ExitUpgradeableReadLock() = _lock.ExitUpgradeableReadLock()
member this.SafeRead reader = this.EnterReadLock(); try reader() finally this.ExitReadLock()
member this.SafeWrite writer = this.EnterWriteLock(); try writer() finally this.ExitWriteLock()
member this.SafeReadWrite readerWriter =
this.EnterUpgradeableReadLock();
try readerWriter (fun writer -> this.SafeWrite(writer))
finally this.ExitUpgradeableReadLock()
member this.Dispose() =
_lock.Dispose()
GC.SuppressFinalize(this)
override this.Finalize() = this.Dispose()
interface IDisposable with
member this.Dispose() = this.Dispose()
type ThreadSafeList<'v>([<Optional; DefaultParameterValue(10)>] initialCapacity: int) =
inherit ThreadSafeObject()
let _container = ResizeArray<'v>(initialCapacity)
member this.Clear() = this.SafeWrite(fun() -> _container.Clear())
member this.Count = this.SafeRead(fun() -> _container.Count)
member this.Add(value: 'v) = this.SafeWrite(fun() -> _container.Add(value))
member this.Contains(value: 'v) = this.SafeRead(fun() -> _container.Contains(value))
member this.Remove(value: 'v) = this.SafeWrite(fun() -> _container.Remove(value) |> ignore)
member this.Item
with get(index: int) = this.SafeRead(fun() -> _container.[index])
and set(index: int) (value: 'v) = this.SafeWrite(fun() -> _container.[index] <- value)
member this.Trim(index: int) = this.SafeWrite(fun() -> _container.Trim(index))
member this.Iter(action: 'v -> unit) = this.SafeRead(fun() -> _container |> Seq.iter action)
member this.Map(action: 'v -> 'r) = this.SafeRead(fun() -> _container |> Seq.map action)
member private this.GetEnumerator() = new ThreadSafeEnumerator<'v>(_container.GetEnumerator(), this.Lock)
interface Collections.IEnumerable with
member this.GetEnumerator() = this.GetEnumerator() :> Collections.IEnumerator
interface IEnumerable<'v> with
member this.GetEnumerator() = this.GetEnumerator()
Iterating a ThreadSafeList using the method Iter is thread safe. However, if I iterate using the Enumerator (implicitly through a Seq.iter in F# or a foreach in C#)
---- System.InvalidOperationException : Collection was modified; enumeration operation may not execute.
is thrown.
Can someone help me understand why the ThreadSafeEnumerator implementation is not thread safe?
Take a close look at your GetEnumerator implementation:
member private this.GetEnumerator() = new ThreadSafeEnumerator<'v>(_container.GetEnumerator(), this.Lock)
A thread that calls this method will perform the following steps:
_container.GetEnumerator()
.After Step 3, the read lock will be held until the enumeration completes and the ThreadSafeEnumerator is disposed, preventing any other thread from modifying the list.
Unfortunately, because Step 1 is executed outside the lock, another thread could sneak in right after it and mutate the list. Then, when the first thread resumes execution, the inner enumerator will throw InvalidOperationException.
To resolve the problem, you must call _container.GetEnumerator()
only after obtaining the read lock. One way to do this is to change ThreadSafeEnumerator to accept an IEnumerable instead of an IEnumerator:
type ThreadSafeEnumerator<'T>(enumerable: IEnumerable<'T>, readWriteLock: ReaderWriterLockSlim) =
do readWriteLock.EnterReadLock()
let inner = enumerable.GetEnumerator()
Then, in ThreadSafeList's GetEnumerator method, pass _container
instead of _container.GetEnumerator
:
member private this.GetEnumerator() = new ThreadSafeEnumerator<'v>(_container, this.Lock)