What’s Wrong With This Code? (#21)

Tuesday, January 27, 2009

This version of WWWTC is dedicated to closures. We can use closures and functional programming in general to create elegant, succinct solutions, but sometimes we can also create subtle bugs. 

Exhibit A. The C# developer who wrote this code expected DoSomething to be invoked with the values 0 through 9, but something is terribly wrong.

for (int counter = 0; counter < 10; counter++)
{
    ThreadPool.QueueUserWorkItem(
        state => DoSomething(counter)
    );
}

What’s the problem?

Bonus Round

A JavaScript developer is working with the following markup.

<div class="header">
    <div class="detail">Detail 1 …</div>
</div>
<div class="header">
    <div class="detail">Detail 2 …</div>
</div>
<!-- and so on-->

When the user clicks inside a detail section, the developer wants to change the background color of all the other header sections. Thinking of one way to achieve this behavior, the developer first tests the jQuery not method.

$(function() {
    $(".header").each(function() {
        alert($(".header").not(this).length);
        return false;
    });
});

This code correctly alerts the user once and displays the total number of headers – 1.Emboldened with this proof of concept, the developer puts in the required CSS manipulation and embeds the code into a click event handler.

$(function() {
    $(".header").each(function() {
        $(".detail", this).click(function() {                    
            $(".header").not(this).css("background-color", "red");
        });
    });
});    

Suddenly, things aren’t working and every header section turns red. What went wrong?


Comments
Morgan Cheng Tuesday, January 27, 2009
For Exhibit A, "captured variable" in C# would be instantiated every time its scope is entered. However, in the sample code, the variable counter is outside of the scope of the inner closure. So, all closures are actually referring to one single counter variable.

Wookie Tuesday, January 27, 2009
For the js problem, the variable "this" in the line that sets the css reference is pointing to the current detail, not the header that contains it. Since all header objects returned by the inner query match the not function (none of them are the same object as the detail), everything will be red.

To solve it, add a var that=this; in the third line, and make it
$(".header").not(that).css("background-color", "red");
Victor Kornov Tuesday, January 27, 2009
1) Since the lamda is not executed right away... every one of them will run with the same counter value = 9 (last). That's because lambda => anonymous method => Compiler generated class with a field pointing (as in pointer) to the last counter value. i.e. it was captured by reference. To avaoid this, you need to assign counter value to a variable in loop & use that in lambda.

2)Jquery in most of it's chaining methods makes "this" to point to current element. So, the code not works as expected, because outer this points to a .header, while inner this points to a .detail
Michael Wallett Tuesday, January 27, 2009
ThreadPool.QueueUserWorkItem will execute the callback when a thread becomes available. In the meantime the for loop is still getting executed on the original thread. The callback method is referencing the local counter variable.

The JavaScript problem is caused by a change in context. .not(this) now refers to elements with a .detail class.
Dave B Tuesday, January 27, 2009
In the first problem, I believe the issue is that the reference to counter and not the value is passed to the lambda expression. Therefore when the lambda is executed, the value of counter at that time is passed in, so theoretically all calls to DoSomething could be made with a value of 9.

For the bonus round, the issue is with the function passed into the detail's click. In that function "this" references the item clicked on and not the parent header element. So, all headers will turn red, as the not operator will not match the correct header element.
Eric Smith Tuesday, January 27, 2009
That's the wonderful thing about closures in C#, all kinds of magic happens under the hood - in particular, a closure class is generated to hold the "local" (actually, not so local) counter. Hence, once those threads are invoked, they'll all use a value of 9.

The details in IL over here: skepticabin.wordpress.com/.../delegate-magic/
Ilia Jerebtsov Tuesday, January 27, 2009
I might be completely wrong, because I only picked up C# 3.0 a couple of days ago, but I think that because of laziness and the funky scope of a variable in a closure, the DoSomething function will only be called when the thread actually gets around to calling it, and more likely than less the counter variable will likely already have a different value than expected (probably always 9).

I haven't worked with jquery enough to understand your bonus round.
Wookie Tuesday, January 27, 2009
P.S.: ReSharper will give the solution for the first problem automatically, so it's not so fun ;)
The Chief Tuesday, January 27, 2009
Exhibit A
Value passed to DoSomething will be whatever counter is at the time the queued work item is executed (e.g. 9 if delegate invoked after loop has finished). An additional variable would need to be added within the for loop, which would be captured by the delegate.

Bill Christie Tuesday, January 27, 2009
Every time that DoSomething(counter) fires, the counter variable will equal 10, because closures continue to reference the actual variable. They don't just use the value that the variable contained when the assignment was made.

Bonus:
Without running it I can't figure out what each of the 'this' variable will contain but I believe the code should be written like this:

$(function() {
$(".header").each(function() {
var thatHeader = this;
$(".detail", thatHeader).click(function() {
var thatDetail = this;
$(".header").not(thatDetail).css("background-color", "red");
});
});
});
Neil Tuesday, January 27, 2009
jQuery problem ...

Is it that the line:

$(".header").not(this).css("background-color", "red");

... the "this" is referring to the "detail" div, so essentially the line is grabbing and "header" that is not the same as "detail", which is all of them, hence all are changed.

Maybe if the "header" was stored in a var before the "click", then used, something like this ? .....

$(function() {
$(".header").each(function() {
var h = this;
$(".detail", this).click(function() {
$(".header").not(h).css("background-color", "red");
});
});
});
Rob Tuesday, January 27, 2009
In the first example, will state be set to the result of DoSomething(counter ) for the last thread that is executed. counter could be any of the values as we don't know what order they are running in.
Rob Tuesday, January 27, 2009
For the bonus round, should the selector that searches for the header be:
$(".header").not(this.parent()).css("background-color", "red");

Also, on the click you should really remove the css property background-color otherwise as soon as you click on another detail div all headers will end up red.
Kim Tuesday, January 27, 2009
I believe the following to be correct:
The Lambda expression captures the *variable* counter and not the value. By the time DoSomething(counter) is called by the threadpool, the value of counter is the last value that was assigned in the loop. Not neccessarily 9 since you could have a context switch in the middle of the loop. If counter is 4 and you have a context switch, all background calls to DoSomething() will have the value 4 for counter.
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!