Common Multithreading Mistakes in C# – Unsafe Assumptions
benbowen.blog
benbowen.blog
What is specific to C# (applies to Java too) is having an implicit mutex/condition pair in each object. I think this is a terrible design mistake because it's not very practical and it's confusing to newbies (a big stumbling block for students when I studied concurrency at the uni).
It's not very practical because in most concurrency tasks I've dealt with (not using C# or Java), there's typically more conditions than there are mutexes (typically 2-4 + O(n) conditions per mutex). In Java/C# land, the typical solution would be to have a complex conditional expression, all the threads spinning on a .wait() there and then over-use of .notifyAll() instead of .notify() causing lots of spurious wakeups and wasting precious cpu cycles.
It's confusing because of the reason above, it's easy to go "I'll solve this by adding another mutex". Unfortunately this is seldom the correct solution (to any problem), and a much better result would be achieved adding more condition variables to wait on.
I wouldn't mind too much about a design mistake in a language I almost never use (Java/C#) if it wasn't the de facto language for learning about concurrency at universities. This has produced so many engineer with a twisted view on concurrency. I understand that "Java is easy" and "C is hard" but when we're already talking about memory models, multi-core, cache coherency and atomicity, C-the-language isn't the hard part and Java-the-language does very little to help with those parts.
The typical solution is to use the higher-level constructs provided by the standard library that take care of these details for you. In Java, at least, explicit mutexes, notify, etc in application code are roughly the equivalent of typing 'AES'. I'm somewhat surprised this C# article happily tells you to just keep adding locks until you think things work. "I mean if you have a hunchback, just throw a little glitter on it, honey, and go dancing."
And libraries exposing APIs through observables is a great idea.
"task.Wait()" is to "await task" as "Task.WaitAll(tasks)" is to "await Task.WhenAll(tasks)"?
Maybe it's just me, but I sure did.
I could have sworn it was WaitAll, but heck - clearly not! Sorry :-).
Each mutex you take has a not-trivial cost but doesn't show up well on a profiler since the individual call is small and spread across an invocation path.
I've seen so much code that just runs TaskFactory.StartNew some arbitrary number of times in a loop to run some CPU intensive task with no/effectively no I/O, presupposing a performance improvement as a result.
Understand runtime performance. Understand the different failure modes inherent in multi-threaded approaches, and lastly: test, profile, test and profile again!
This, this and this. I see so much code where threading and caching are thrown around as a solution to a performance and no one ever tests to see if it's actually an improvement. I saw one the other day that used multiple threads to call a micro service which locked execution to a single caller and most of the bottleneck was serialization/deserialization. Making the microservice a library would have been much more performant.
for(int i = 0; i < Environment.ProcessorCount; i++)
Task.Factory.StartNew(...);
?My last boss had the same approach to threading. Although I understand the reasoning (usually it's based on some bad experiences with messy multithreaded code), I still strongly disagree with the premise.
Of course, the approach can be fine depending on the requirements (in which case, of course, it's preferrable), but generalizing the statement can be a huge mistake.
Most of the threading code I've seen got so horrible precisely because it was tucked on after the fact. If you start your codebase on the premise that everything is single-threaded, switching the important bits to something remotely concurrency-compatible often requires major rewrites. Thus, I consider it to be an important architectural decision one should make early on. You don't have to go through with it and start with a huge degree of concurrency, but while you're implementing the core of your application, you should be aware where it makes sense to design around the possibility of concurrency. Once the API is suited in that way, you avoided a major headache while trying to make your glacial code faster in a hurry.
Of course, I'm referring to concurrency on the architecture level, not the trivial cases in which individual operations can be sped up. Having those issues in the back of your head while designing is one of the things that makes a good software architect. Too often it's omissions like this that will be the cause for explanation like "I know it's a mess, but it has grown organically and we can't change it now." down the line if you're explaining your project to a newcomer.
Edit: As an addendum: The problem with switching to multithreaded code also doesn't only lie in the amount of work it entails and the amount of code you'll have to touch to do it, but also in the amount of code you forget to touch. It's very easy to introduce very subtle race conditions that only show in very specific edge cases. But when they occur, you'll have a major firefighting job on your hand. I feel those are easier to avoid if you try to design for concurrency up front instead of trying to remember everything that could potentially break after the fact.
- Edward Lee's "The Problem with Threads": https://www2.eecs.berkeley.edu/Pubs/TechRpts/2006/EECS-2006-...
- The occasionally hilarious chapter on concurrency (especially the "Concurrentgate" section) from Andrei Alexandrescu's "The D Programming Language", available in its entirety here: http://www.informit.com/articles/article.aspx?p=1609144
I think MS made great progress with the TPL and kickstarted an industry-wide movement with async/await. But certain aspects of C# still drive me nuts and will allow a junior dev to blow a leg off (I'd give my left arm for C++-style const references and/or compiler-enforced immutability). At one point I went so far as to play around with a Rosyln analyzer to tackle the problem (https://github.com/markwaterman/CondensedDotNet/blob/master/...), but gave up after realizing anything more then a token effort would be a huge undertaking.
Other languages are nibbling away at the edge of the concurrency problem with language-level support for CSP (golang), actors, etc., but, outside of the functional world (Erlang), I don't seen anyone working to address concurrency from the ground up.
Concurrent programming is hard and complicated, even in languages with superior safety guarantees, especially for non-trivial, non-toy implementations. The harder it is to implement a solution, the more care and consideration for whether it's necessary or some less difficult or complex solution might suffice instead, in my opinion.
But now I live on Erlang's BEAM (via Elixir) and I freaking love it. The real gain for me is that I found I didn't need mutexes, locks, critical sections, etc., because the super lightweight thread-like Erlang processes (not OS processes) themselves run in parallel but each one runs in a single-threaded manner. This effectively turns each process itself into its own critical section, and it's this aspect that I personally have found extremely valuable.
I think perhaps what you are getting at is that there is still need for coordination for parallel processes, and that is totally correct. But each BEAM(!) process has its own non-shared memory and runs on a single thread. This is my point in that it doesn't need the mutex because the process' execution is its own critical section. I've structured my ibGib engine to be more functional and the need for the mutex/critical sections has disappeared for me.
For example, check out this SO post that I just googled: http://stackoverflow.com/questions/28554114/applying-a-mutex...
In the answer, he tells the OP (who thinks he needs a mutex) that what he can actually do is just do the work in the process. This kind of understanding of single-threaded "bounded context"-like execution, combined with ubiquitous immutability has been massively helpful for me with ibGib.
In C# for example, I was trying to apply `Task<ib>` all over the place (yes it is non-idiomatic lowercasing of the interface...totally aware of this sin in C#). And so even with async/await awesomeness, the code is just riddled with async/await, blah blah blah. The same goes for using Rx in C# for a "microservices" engine (I started it before that was a fad and I called them autonomous services lol) and my more recent POC with Rxjs in TypeScript/JavaScript. The point is the whole approach of concurrency and whatnot are "addons" in the form of mutexes, semaphores, critical sections, locks, barriers (I've used quite a few over the years). But with programming on the BEAM(!) with Elixir and Erlang, parallel coding is just a more natural experience, because it's just how it's done.
In my view, this largely stems from the aspect I mentioned: that they have parallel processes, each running its own memory and on a single thread, communicating with message passing (now called the Actor Model but they didn't know that at the time). On top of this fundamental design decision, they built up an awesome infrastructure with OTP, Supervision trees, and more. But anyway, I didn't mean to write that much - but it's just been a delightful experience, since my very first application in Delphi was a transcription app with a from-scratch multi-threaded realtime document checker (like a spell checker, but more pluggable and geared towards transcription with text expansion which is a totally different use case than existing off-the-shelf spell checkers).
From the article linked where they mention volatile and the C# memory model. Seems like volatile may not actually have worked as intended for this case!
This is really just fundamental concurrency stuff though. Something which is sadly in short supply in some people's skill sets, but then I guess not everyone spent four years working on massively multithreaded C++ software early in their career like I did.
I'd really prefer it if C# made you share memory explicitly - default shared memory concurrency is just asking for trouble, in this and many other languages, because you have to do extra to do things right, rather than extra to do things wrong.
Things may have changed, but when I last looked there are very few books on multithreading, especially when it comes to a particular language. A lot of it seems to just be learned on the job using reference docs
This is, obviously, the answer to the GP's question.
I expected the following to be safe:
Dim V(99) As Integer
Parallel.For(0, 100, Sub(i)
V(i) = i
End Sub)
Dim Result = V.Sum
Dim V(99) As Integer
Parallel.For(0, 100, Sub(i)
V(i) = i
End Sub)
Threading.Thread.MemoryBarrier()
Dim Result = V.SumTake a look at "[t]he following implicitly generate full fences" here: http://www.albahari.com/threading/part4.aspx.
Parallel.For would probably be covered by "[a]nything that relies on signaling." There's more info scattered about in some Stack Overflow answers. The content is useful, just unofficial.
E.g. http://stackoverflow.com/a/6932271/242520, http://stackoverflow.com/a/681872/242520
Does anyone know where it is? It was very informative..
There are two others:
- http://benbowen.blog/post/cmmics_i/
- http://benbowen.blog/post/cmmics_ii/
It's well worth learning about the Interlocked class if you want to do parallel programming. If you get it wrong then you will see incorrect/corrupt results. It's also worth keeping in mind that for simple tasks it can be quicker to do them single threaded, due to the overheads involved. I demonstrated this in a simple benchmark app [0] that I wrote for a chapter of my book on ASP.NET Core [1].
[0]: https://github.com/PacktPublishing/ASP.NET-Core-1.0-High-Per...
Only need lock() and the full Monitor classes if there's more to be done within the locked statement.
In my experience another mistake made by novices and journeymen alike (at least occasionally by the latter) is to reach for this tool without careful consideration of whether it's even necessary.
"A conforming CLI shall guarantee that read and write access to properly aligned memory locations no larger than the native word size (the size of type native int) is atomic (see §I.12.6.2) when all the write accesses to a location are the same size."
Since references are the native word size, the CLR will ensure that there is no tearing.
Note that it is possible to produce a torn read variables that are larger than a native-int, even of native types such as decimal[1].
[0] - http://www.ecma-international.org/publications/files/ECMA-ST... section I.12.6.6, page 102
[1] - http://stackoverflow.com/questions/23262513/reproduce-torn-r...
You can't. Dictionary can be read concurrently, but once you start writing to it concurrently all bets are off. ConcurrentDictionary implements IDictionary and allows concurrent writes.
Also, Entity Framework. We had an icky bug coming from somebody storing the datacontext in a member variable. Don't do that.
Wound up seeing some REALLY weird issues... HttpContext.Current.Items is your friend there.
https://msdn.microsoft.com/en-us/library/dd287191(v=vs.110)....
// In multiple threads:
foreach(element in collection) { ... }
Try this instead, and you'll guaranteed see issues:
// Before:
IEnumerator enumerator = collection.getEnumerator();
// In multiple threads:
while(enumerator.Move()) {
var element = enumerator.Current;
}Here's another simple way to think about it: Handling a request usually invokes a couple of layers of your code until you hit some IO - database, file system, network, etc. If that IO is done in a blocking fashion, this request is consuming server resources THE WHOLE TIME, even while it's doing nothing but waiting for a network response. On the other hand, if the IO is non-blocking (async), then this request consumes no resources while it's waiting.
So imagine you have a request that takes 2000ms to service, and 1700ms of that time is spent waiting on the database to fetch some data. Without async, that request will consume 2000ms of CPU time. With async, only 300ms of CPU time is used.
It's very possible to stuff it up, to have a conventional thread pool end up competing with your async-handling threads, you can get priority inversions where the OS thread scheduling competes with the task scheduling, and plenty of other pitfalls. But done right async/await should be significantly faster than threading.
I don't believe that you pay a significant additional cost per await - i.e. if you have one await in your code, then you might as well use as many as you can to chop the operation up into small parts so that operations can flow through better.
If you "must wait for a thread to become available" for any significant amount of time then you have run out of threads despite being async, and you have other problems.
This article talks about the mechanics of how the operation's completion bubbles back up to your code, in some detail: http://blog.stephencleary.com/2013/11/there-is-no-thread.htm...
See also, avoiding some basic mistakes: http://www.anthonysteele.co.uk/AsyncBasicMistakes
in UI code, there is a guideline that "operations that could take over 50ms should be async" (1) so as to not lock the UI thread. But it's clear that this does not apply to web server code where it's all thread pool threads already.
1) http://blog.stephencleary.com/2013/04/ui-guidelines-for-asyn...
A simple table would have worked fine.
So it is better to treat them as multithreaded always... which falls back on the original post.
they will mostly work (like multiple threads reading, or 1 writes while another retrieves), except when two threads try to simultaneously write, or when 1 writes while another enumerates.
When that happens, all bets are off, and you will silently get obtuse errors such as null return values or enumerating past the end of the collection, or a corrupt collection that throws exceptions from other (seemingly) random and unrelated areas.
I believe the new concurrent collections take care of this, but it is still so easy for beginners to shoot themselves with async programming. very much in stark contrast to how dev-friendly the rest of the CLR is.
.NET 4 is over eight years old now.
And for all these years, the docs have explicitly informed about lack of thread safety in the non-concurrent versions and referenced the concurrent equivalents.
The more difficult problems to detect are those unrelated to collection versions + enumerators. For example accidentally reading a Dictionary<K, V> while someone is adding to it on another thread (this requires true concurrency). This one has no builtin detection for modification. Does the Java dictionary really check thread identifiers for who is modifying? Or use some kind of entry counter to detect concurrency?
I'd love to have that in debug builds in C#.
The result in C# is usually that it works 99% of the time and corrupts silently in the edge case that someone happened to read the hash table at the same time as an internal array was resized or similar. Typical consequence is that it reports a count of N and Contains(x) is true, but when listing the contents we see some other number of items, and no sign of x...
It's that customer report with a stack trace you say "that's impossible, foo is never null there". Or that unit test that only fails one build of 1000 and you blame cosmic rays first...
As you said that will not always reliably detect the error and show it the developer. But that's imho the biggest challenge in concurrent and multithreaded programming in general: Errors are mostly not visible - until they totally tear down the software in all imaginable ways. And the root cause if often very very hard to find. There's also no general solution for improving the situation. E.g. making all collections concurrent collections would mean they would not throw exceptions anymore if misused, but most likely the application behavior would be silently broken - and the user would take a performance hit. Imho the best solution that's currently in use is preventing shared mutable objects between threads at all - like Erlang (and also Javascript!) are doing. But for general purpose languages like C# and Java people would complain that this is too opinionated and disallows several optimizations.
C# makes it very easy to avoid the normal threading issues by using Task<T> and other high level helpers - which is excellent. The problem I'm usually facing is isolating what the code within the task can "reach". Normally the main program code is a huge amount of code that must be single threaded. Tons of caches using vanilla dictionaries and so on. Now there is a small perf critical thing I need to do with Tasks. The failure mode is that the code from in the task somehow indirectly (e.g through some property on an object ending up in a cache backed by a dictionary) ends up accessing shared mutable state. Soparating the code used concurrently from the rest becomes a mental overhead. So anything that helps me do that would help.