Pop quiz, what's wrong with the following code?
public void Unsubscribe()
{
if (_request != null)
{
ThreadPool.Enqueue(() => _service.Unsubscribe(_request));
}
}
public void Subscribe(string key)
{
Unsubscribe();
if (!String.IsNullOrEmpty(key))
{
_request = new Request(key, handler);
ThreadPool.Enqueue(() => _service.Subscribe(_request));
}
}
Does everyone see the issue? There is a critical bug in the above code which isn't always readily apparent.
Try to find it...
I actually wrote code like this today (same concept, different implementation) and immediately saw some serious defects. Honestly, I'm lucky the issues popped up right away, these sorts of things tend to not appear right away, but jump up to bite you at a later point.
In this case the issue is the use of closures. When using a closure it copies the fields from outside the lambda expression into the expression meaning that my use of _request from within the lambda is actually the same reference that exists outside the lambda expression. So in the above case the Unsubscribe lambda gets executed on a new thread (from the pool) but by the time it actually executes the _request has already been changed.
In this case, you're actually unsubscribing from a request that most likely hasn't even been subscribed yet. And to top it off you haven't unsubscribed from the old request yet either. Obviously the above is a race condition where the exact output isn't guaranteed. There is a chance it works perfectly (though doubtful with a true thread pool). There is a chance the new request is subscribed first and then immediately unsubscribed as well.
The simplest way to resolve this issue is by changing the variable which is captured from one which is shared between both closures to one that is unique to each closure. As shown here:
public void Unsubscribe()
{
if (_request != null)
{
var localRequest = _request;
ThreadPool.Enqueue(() => _service.Unsubscribe(localRequest));
}
}
public void Subscribe(string key)
{
Unsubscribe();
if (!String.IsNullOrEmpty(key))
{
_request = new Request(key, handler);
var localRequest =_request;
ThreadPool.Enqueue(() => _service.Subscribe(localRequest));
}
}
However, what I really want to solve this type of problem going forward is to develop something which is process aware, much like the Saga in NServiceBus. Of course my goal is not to be a long running, persistable process like the Saga in NServiceBus, but the process portion is what I'm looking at.
3 comments:
I'm missing something here, since
var someThing = _someOtherThing;
Doesn't someThing still reference the same object? Wouldn't that mean that when you send that into a method, you are still changing _someOtherThing by reference?
public static void ChangeUserNameInLambdaWithNewVariant(User userToChange, String newName)
{
var newUser = userToChange;
Enumerable.Range(0, 1).ToList().RunNoParameterActionWithForEach(() => { newUser.UserName = newName; });
}
public static void ChangeUserNameInLambdaWithNewUser(User userToChange, String newName)
{
User newUser;
newUser = userToChange;
Enumerable.Range(0, 1).ToList().RunNoParameterActionWithForEach(() => { newUser.UserName = newName; });
}
public static void ChangeUserNameInLambdaWithoutNewUser(User userToChange, String newName)
{
Enumerable.Range(0, 1).ToList().RunNoParameterActionWithForEach(() => { userToChange.UserName = newName; });
}
All three change the orginal object.
By the way, RunNoParameterActionWithForEach is just a cheesy extension method that basically runs a foreach loop and calls the action.
Sean,
The key here is the closure and understanding what is captured within the closure. When using a closure the variable itself is captured, not its values (meaning it gets a reference to the variable (reference to value or reference to reference).
That's why you can change value types within a closure and see their results outside of the closure.
Therefore you're correct any change to the properties of the reference type would be reflected within the closure (or outside) but not a change to the reference itself. I probably should have been more clear, but in my case my request objects are immutable and therefore it would be impossible for any of its data to change.
Thanks for sharing very nice code.
Post a Comment