Refactoring threading code
time to read 4 min | 614 words

Let us start by saying that threading is complex, hard and complicated.

A while ago I was asked to do a code review of a JoinAll() method, it looked somewhat like this:

public void JoinAll(IEnumerable<Thread> threads)
{
	foreach (Thread thread in threads)
	{
		if(thread.Join(500)==false)
			thread.Abort();
	}
}

I was properly horrified by the use of thread.Abort(), but as you know, all rules have exceptions, so instead of commenting on the implementation details of the method, I asked to understand the entire scenario. This piece of code will probably easier than me trying to explain:

List<Thread> threads = new List<Thread>();
List<string> results = new List<string>();

foreach (string urlParam in GetUrlsToQuery())
{
	string url = urlParam;
	Thread thread = new Thread(delegate()
	{
		WebRequest wr = WebRequest.Create(url);
		WebResponse response = wr.GetResponse();
		using (Stream stream = response.GetResponseStream())
		using (StreamReader sr = new StreamReader(stream))
		{
			string result = sr.ReadToEnd();
			lock (results)
			{
				results.Add(result);
			}
		}
	});
	threads.Add(thread);
	thread.Start();
}

foreach (Thread thread in threads)
{
	if(thread.Join(500)==false)
		thread.Abort();
}

ProcessResults(results);

That piece of code is in the Page_Load method of a page. The scenario was querying remote servers for information (15 - 20 of them, most often), and there was a time limit to how long we needed to wait before we need to display the results to the user.

The code above actually works, but it is tremendously wasteful of resources (creating a thread is expensive) and not really nice way to behave to a thread, it is entirely possible to get to a point where you break up the lock mechanism and remains with a broken app because of aborted threads that were in the middle of supposedly atomic operation. Not to mention that we are likely not going to cleanup the resources very well if we abort the thread.

The hard constraint that the page must be rendered in a second or so. But here, if even two of those servers are not playing well, it is not going to render fast enough.

In short, do write code like the above.

I took the code and change it a bit, instead of explicit threading, let us use the async API instead, so we get this piece of code: 

List<string> results = new List<string>();
List<WaitHandle> waitHandles = new List<WaitHandle>();
foreach (string url in GetUrlToQuery())
{
	WebRequest wr = WebRequest.Create(url);
	IAsyncResult asyncResult = wr.BeginGetResponse(delegate(IAsyncResult ar)
	{
		WebResponse response = wr.EndGetResponse(ar);
		using (Stream stream = response.GetResponseStream())
		using (StreamReader sr = new StreamReader(stream))
		{
			string result = sr.ReadToEnd();
			lock (results)
			{
				results.Add(result);
			}
		}
	},null);
	waitHandles.Add(asyncResult.AsyncWaitHandle);
}

WaitHandle.WaitAll(waitHandles.ToArray(), TimeSpan.FromMilliseconds(700),false);
string[] resultsCopy;
lock(results)
{
	resultsCopy  = results.ToArray();
}
ProcessResults(resultsCopy);

Now, instead of having a lot of blocked threads, we have moved it to the framework responsibility, we then gather all the wait handles and wait on all of them (remember, there aren't more than 20, but if more than 64 are required, we can manage as well).

The important part comes when we decide that we want to give up, we make a copy of the results so far, and then start to process them. In the meantime, any request that was completed can go along its merry way and add the newly got result to the results list, but that is no longer relevant, because it got there too late.

If I wanted to get really deep into it, ASP.Net has the notions of the async request handling, which is very cool, but it doesn't have an expiration time (that I know of, at least), and we didn't want to get into it in the first place.

Anxiously awaiting your comments, telling me how wrong I am.

Tweet Share Share 14 comments
Tags: