A sneaky little critter of a bug and I forget to use common sense to debug

Last night, I decided to put my code of late (the custom TaskScheduler and the pattern I’ve come up with using several TaskScheduler implementations) through some more rigorous testing.

Imagine my surprise when a nasty bug showed up right away… I happened to open Process Explorer and had a look at the thread count while running the application. Disaster! Every time I traversed a directory, or even refreshed the current one, new threads would be added, and they would never be removed.

This is roundabout where I lost my common sense.

Common sense says: If your code is spinning up hundreds of threads, open the Threads debug window, and look at what code they are running.

So what do I do? I jump to conclusions… Panicking, thinking “Oh no. I have written two articles about TaskSchedulers and managing your async threads, and published unspeakably stupid code. Oh, woe is me!”

Half an hour later, after going down completely the wrong road and trying to fix code that was never broken, I finally looked at the Threads window.

All of the rogue threads were in WorkStealingTaskScheduler.DispatchLoop.

It turns out my code that updates the status strip, which is called in several places, including when refreshing the directory, called Task.Factory.StartNew with an overload that accepts a TaskScheduler parameter and created a new instance of a WorkStealingTaskScheduler each time. While there is no recommendation that says you must not create multiple instances of the same scheduler type, common sense does dictate that if you should decide to do so, at least understand what the code does. (Usually if the framework expects you only to create one instance of some type, there will be a clue in the pattern in which it is consumed. For example, ToolStripManager.Renderer in Windows Forms indicates that if you create a custom renderer, you should set it to replace the global default renderer, which implies there should only be a single instance.)

WorkStealingTaskScheduler is initialized with a maxConcurrencyLevel parameter, which defaults to Environment.ProcessorCount * 2. It then spins up that number of threads (i.e. instances of the Thread type), which will wait for work to process in the DispatchLoop method. Thus every time the files were refreshed, a new scheduler would be added, adding another Environment.ProcessorCount * 2 threads. The scheduler would never be used again, but would also never be garbage collected. (I don’t know enough about garbage collection to know if a TaskScheduler would be cleaned up otherwise, but I’m guessing it would.)

I was deliberately creating new instances, because of some paranoid fear that an instance of the scheduler might be too busy, and I want status info always updated immediately, regardless of how busy the application is with other tasks. But thinking about it, this design didn’t make sense. If all your threads are busy, or worse, deadlocked, it of course indicates that something else is wrong.

Anyway, the fix was obviously not to create multiple schedulers of this type. I did the same for other types as well, even my own ParallelTaskScheduler, just in case.

Advertisements

About Jerome

I am a senior C# developer in Johannesburg, South Africa. I am also a recovering addict, who spent nearly eight years using methamphetamine. I write on my recovery blog about my lessons learned and sometimes give advice to others who have made similar mistakes, often from my viewpoint as an atheist, and I also write some C# programming articles on my programming blog.
This entry was posted in Programming and tagged , , . Bookmark the permalink.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s