I'm designing a class that I wish to make readonly after a main thread is done configuring it, i.e. "freeze" it. Eric Lippert calls this popsicle immutability. After it is frozen, it can be accessed by multiple threads concurrently for reading.
My question is how to write this in a thread safe way that is realistically efficient, i.e. without trying to be unnecessarily clever.
Attempt 1:
public class Foobar { private Boolean _isFrozen; public void Freeze() { _isFrozen = true; } // Only intended to be called by main thread, so checks if class is frozen. If it is the operation is invalid. public void WriteValue(Object val) { if (_isFrozen) throw new InvalidOperationException(); // write ... } public Object ReadSomething() { return it; } }
Eric Lippert seems to suggest this would be OK in this post. I know writes have release semantics, but as far as I understand this only pertains to ordering, and it doesn't necessarily mean that all threads will see the value immediately after the write. Can anyone confirm this? This would mean this solution is not thread safe (this may not be the only reason of course).
Attempt 2:
The above, but using Interlocked.Exchange
to ensure the value is actually published:
public class Foobar { private Int32 _isFrozen; public void Freeze() { Interlocked.Exchange(ref _isFrozen, 1); } public void WriteValue(Object val) { if (_isFrozen == 1) throw new InvalidOperationException(); // write ... } }
Advantage here would be that we ensure the value is published without suffering the overhead on every read. If none of the reads are moved before the write to _isFrozen as the Interlocked method uses a full memory barrier I would guess this is thread safe. However, who knows what the compiler will do (and according to section 3.10 of the C# spec that seems like quite a lot), so I don't know if this is threadsafe.
Attempt 3:
Also do the read using Interlocked
.
public class Foobar { private Int32 _isFrozen; public void Freeze() { Interlocked.Exchange(ref _isFrozen, 1); } public void WriteValue(Object val) { if (Interlocked.CompareExchange(ref _isFrozen, 0, 0) == 1) throw new InvalidOperationException(); // write ... } }
Definitely thread safe, but it seems a little wasteful to have to do the compare exchange for every read. I know this overhead is probably minimal, but I'm looking for a reasonably efficient method (although perhaps this is it).
Attempt 4:
Using volatile
:
public class Foobar { private volatile Boolean _isFrozen; public void Freeze() { _isFrozen = true; } public void WriteValue(Object val) { if (_isFrozen) throw new InvalidOperationException(); // write ... } }
But Joe Duffy declared "sayonara volatile", so I won't consider this a solution.
Attempt 5:
Lock everything, seems a bit overkill:
public class Foobar { private readonly Object _syncRoot = new Object(); private Boolean _isFrozen; public void Freeze() { lock(_syncRoot) _isFrozen = true; } public void WriteValue(Object val) { lock(_syncRoot) // as above we could include an attempt that reads *without* this lock if (_isFrozen) throw new InvalidOperationException(); // write ... } }
Also seems definitely thread safe, but has more overhead than using the Interlocked approach above, so I would favour attempt 3 over this one.
And then I can come up with at least some more (I'm sure there are many more):
Attempt 6: use Thread.VolatileWrite
and Thread.VolatileRead
, but these are supposedly a little on the heavy side.
Attempt 7: use Thread.MemoryBarrier
, seems a little too internal.
Attempt 8: create an immutable copy - don't want to do this
Summarising:
EDIT:
Perhaps my question wasn't clear, but I am looking in particular for reasons as to why the above attempts are good or bad. Note that I am talking here about a scenario of one single writer that writes then freezes before any concurrent reads. I believe attempt 1 is OK but I'd like to know exactly why (as I wonder if reads could be optimized away somehow, for example). I care less about whether or not this is good design practice but more about the actual threading aspect of it.
Many thanks for the response the question received, but I have chosen to mark this as an answer myself because I feel that the answers given do not quite answer my question and I do not want to give the impression to anyone visiting the site that the marked answer is correct simply because it was automatically marked as such due to the bounty expiring. Furthermore I do not think the answer with the highest number of votes was overwhelmingly voted for, not enough to mark it automatically as an answer.
I am still leaning to attempt #1 being correct, however, I would have liked some authoritative answers. I understand x86 has a strong model, but I don't want to (and shouldn't) code for a particular architecture, after all that's one of the nice things about .NET.
If you are in doubt about the answer, go for one of the locking approaches, perhaps with the optimizations shown here to avoid a lot of contention on the lock.
Maybe slightly off topic but just out of curiosity :) Why don't you use "real" immutability? e.g. making Freeze() return an immutable copy (without "write methods" or any other possibility to change the inner state) and using this copy instead of the original object. You could even go without changing the state and return a new copy (with the changed state) on each write operation instead (afaik the string class works this). "Real immutability" is inherently thread safe.
I vote for Attempt 5, use the lock(this) implementation.
This is the most reliable means of making this work. Reader/writer locks could be employed, but to very little gain. Just go with using a normal lock.
If necessary you could improve the 'frozen' performance by first checking _isFrozen
and then locking:
void Freeze() { lock (this) _isFrozen = true; } object ReadValue() { if (_isFrozen) return Read(); else lock (this) return Read(); } void WriteValue(object value) { lock (this) { if (_isFrozen) throw new InvalidOperationException(); Write(value); } }
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With