I’ve used volatile where I’m not sure it is necessary.
Let me be very clear on this point:
If you are not 100% clear on what volatile means in C# then do not use it. It is a sharp tool that is meant to be used by experts only. If you cannot describe what all the possible reorderings of memory accesses are allowed by a weak memory model architecture when two threads are reading and writing two different volatile fields then you do not know enough to use volatile safely and you will make mistakes, as you have done here, and write a program that is extremely brittle.
I was pretty sure a lock would be overkill in my situation
First off, the best solution is to simply not go there. If you don’t write multithreaded code that tries to share memory then you don’t have to worry about locking, which is hard to get correct.
If you must write multithreaded code that shares memory, then the best practice is to always use locks. Locks are almost never overkill. The price of an uncontended lock is on the order of ten nanoseconds. Are you really telling me that ten extra nanoseconds will make a difference to your user? If so, then you have a very, very fast program and a user with unusually high standards.
The price of a contended lock is of course arbitrarily high if the code inside the lock is expensive. Do not do expensive work inside a lock, so that the probability of contention is low.
Only when you have a demonstrated performance problem with locks that cannot be solved by removing contention should you even begin to consider a low-lock solution.
I added “volatile” to make sure that there is no misalignment occurring: reading only 32 bits of the variable and the other 32 bits on another fetch which can be broken in two by a write in the middle from another thread.
This sentence tells me that you need to stop writing multithreaded code right now. Multithreaded code, particularly low-lock code, is for experts only. You have to understand how the system actually works before you start writing multithreaded code again. Get a good book on the subject and study hard.
Your sentence is nonsensical because:
First off, integers already are only 32 bits.
Second, int accesses are guaranteed by the specification to be atomic! If you want atomicity, you’ve already got it.
Third, yes, it is true that volatile accesses are always atomic, but that is not because C# makes all volatile accesses into atomic accesses! Rather, C# makes it illegal to put volatile on a field unless the field is already atomic.
Fourth, the purpose of volatile is to prevent the C# compiler, jitter and CPU from making certain optimizations that would change the meaning of your program in a weak memory model. Volatile in particular does not make ++ atomic. (I work for a company that makes static analyzers; I will use your code as a test case for our “incorrect non-atomic operation on volatile field” checker. It is very helpful to me to get real-world code that is full of realistic mistakes; we want to make sure that we are actually finding the bugs that people write, so thanks for posting this.)
Looking at your actual code: volatile is, as Hans pointed out, totally inadequate to make your code correct. The best thing to do is what I said before: do not allow these methods to be called on any thread other than the main thread. That the counter logic is wrong should be the least of your worries. What makes the serialization thread safe if code on another thread is modifying the fields of the object while it is being serialized? That is the problem you should be worried about first.