Improving NotifyWorker (and why blogging makes me smarter)
My current day job consists of writing middleware and web-based LOB software for an actuarial services company.
David is my primary collaborator and a fellow blogger. We get along amazingly well, but every once in a while, we disagree. The amazing thing, though, is how we handle disagreements.
I'm at a point where I look forward to disagreement! For a reason that I hope more people can come to realize.
If we disagree about how something should work, it's almost always an opportunity for improvement. Specifically, at least one of the following is likely to happen:
- I get smarter
- He gets smarter
- The code gets better
Quite often, it's all three. I know this sounds like motivational speaker talk or corporate training fodder, but it's true. The fact that we often discuss our disagreements with explicit reference to masters is also helpful.
I blog for similar reasons. I'm far from a CLR, C# or WPF expert, at least relative to the experts I've had the opportunity to work with.
I'm just a geek trying to figure things out and sharing as I go. Hopefully what I share is helpful. Often, though, blogging has self-serving benefits. Just taking the time to write down explanations for what I'm doing forces me to rationalize edge cases.
If I had never blogged the recursive flattening craziness, I would have never have seen all of the edge cases I had missed the first time out.
Anyway, to my point.
Simon was kind enough to point out a deadlock in my NotifyWorker code.
FYI: At this point, looking at the code will be quite helpful.
I was doing a Thread.Join() in the middle of a lock. Of course, if the thread in question is waiting on the lock which is held, the thread will never exit. The result: the Join() call will never return.
Deadlock, indeed.
Props to Simon for pointing out this problem.
I moved the Join() outside of the lock and thought I'd fixed the problem.
Maybe, but I also realized another problem.
- Dispose is likely called from the UI thread.
- Dispose calls Join on my worker thread as discussed above.
- The worker thread does Dispatcher.Invoke twice.
What happens if Dispose is called between my check for m_disposed and when the Invoke actually gets to the UI thread?
Dispose will be tying up the UI thread waiting for Join() to return. The worker thread will never terminate because it's waiting for Invoke to get to the UI thread.
Deadlock again.
Darn.
The solution: Move from Dispatcher.Invoke to Dispatcher.BeginInvoke.
BeginInvoke returns an instance of DispatcherOperation. This class has an Abort method which can be used to abort a pending BeginInvoke.
I now have an instance field containing the pending operation, if one exists, which can be aborted via Dispose before the call to Join. I also now enforce that Dispose must be called on the UI thread (by adding a call to VerifyAccess). This ensures that Abort is not called while the worker is synchronized to the UI thread.
After some careful uses of DisaptcherOperation.Wait and some paranoid null checks, I think I'm okay.
As an aside, I also noticed that Dispose() will throw in the case where NotifyNewWork() has never been called. I fixed that, too.
*Sigh*
Threading code is tough. I had to stand up and pace around a few times as I mentally worked through possible edge cases.
As I've said before, I think this implementation is better, but it would be a stretch for me to claim it's correct. Hopefully, it's more correct.
Please keep the questions, comments, and feedback coming (after you take a look at the post about contacting me).
The code will get better and we'll all get smarter.
Happy hacking.