What's Wrong With This Code? (#13)

 

For lucky #13, I want to know what can go terribly wrong with the following code, and why.

class HardWorker
{        
    public void DoMultiThreadedWork(object someParameter)
    {
        lock (lockObject)
        {
            // ... lots of work ...
        }
    }

    private string lockObject = "lockit";
}

 

Hint: Think about memory optimizations in the CLR.

posted on Monday, March 26, 2007 10:34 PM by scott

Comments

Monday, March 26, 2007 8:20 PM by James Newton-King

# re: What's Wrong With This Code? (#13)

There is the possibility that the string could be interned and used in a lock somewhere else. Both strings would be the same object.
Monday, March 26, 2007 9:04 PM by David Hogue

# re: What's Wrong With This Code? (#13)

Due to the hint (and a little help from Google), I remembered there was something about duplicate strings pointing to the same reference. So if somewhere else in the program a lock also uses the string "lockit" you could have some problems.
Monday, March 26, 2007 10:28 PM by lucas

# re: What's Wrong With This Code? (#13)

Is it because a string is immutable, it is pointless to lock it, so it would optimize out...? interesting. I remember going through old code, making it thread safe, had to lock access to an old hash-table, interesting...

Thanks for the puzzle.
Monday, March 26, 2007 11:02 PM by Eric W. Bachtal

# re: What's Wrong With This Code? (#13)

It's late, but I'll bite. The string literal on which the lock is taken is interned by the CLR to a string pool, so there's really only one shared "lockit" in the appdomain. This means this lock is subject to stepping on (and being stepped on) by other "lockit" locks (in other words, it's not really a private lock). But I think the bigger issue is that the interned string pool is rearranged under memory pressure by the CLR. The lock taken here will prevent this rearrangement, causing, I assume, some sort of grief for the CLR. Or maybe not, like I said, it's late. Looking forward to your explanation.
Monday, March 26, 2007 11:33 PM by singhhome

# re: What's Wrong With This Code? (#13)

string Interning can cause multiple objects to have a common reference, so all instances will have the same sync lock index.
Monday, March 26, 2007 11:39 PM by Ken Tong

# re: What's Wrong With This Code? (#13)

I have been subscribing your blog for months and learn a lot.

All instances of HardWorker will be locking the same instance of lockObject.

Try this:
string a = "lockit"; // control object
string b = "lockit"; // literal
string c = "LOCKIT".ToLower(); // return from a method
string d = "lock" + "it"; // compile time concatenation
string e1 = "lock";
string e2 = "it";
string e = e1 + e2; // runtime concatenation
Console.WriteLine(object.ReferenceEquals(a, b));
Console.WriteLine(object.ReferenceEquals(a, c));
Console.WriteLine(object.ReferenceEquals(a, d));
Console.WriteLine(object.ReferenceEquals(a, e));
Tuesday, March 27, 2007 12:02 AM by Moz

# re: What's Wrong With This Code? (#13)

Wild guess says that the optimiser will produce a constant string literal rather than an object, so it won't have all the methods an object should. The compiler will notice this and link in a method on the literal that doesn't exist.
Tuesday, March 27, 2007 12:34 AM by der Igel

# re: What's Wrong With This Code? (#13)

Strings are interned. May be another part of code (in different class) which also locked on string "lockit". And that code will block this function.
Tuesday, March 27, 2007 12:45 AM by marshall

# re: What's Wrong With This Code? (#13)

lockObject is a instance variable rather than a static object and can be locked by each instance of the class HardWorker independent of any other instance.
Tuesday, March 27, 2007 1:57 AM by Wayne Howarth

# re: What's Wrong With This Code? (#13)

Could it be something to do with the fact that the compiler may optimise the code so that multiple instances of the HardWorker class reference the same instance of the string object holding "lockit" on the heap?

So that assuming instance #1 has the lock then instance #2 wouldn't be able to obtain the lock until instance #1 released it despite the fact that it isn't declared as static.
Tuesday, March 27, 2007 2:33 AM by Kieron

# re: What's Wrong With This Code? (#13)

Hiya, will it be that you're locking a new instance of a string each time. So the lock is pointless?
Tuesday, March 27, 2007 3:50 AM by James

# re: What's Wrong With This Code? (#13)

The string could be interned, so you could be locking in places you never intended, and very probably introducing deadlocks.

Using a plain old System.Object instead of a string should sort this out.
Tuesday, March 27, 2007 5:27 AM by scott

# re: What's Wrong With This Code? (#13)

For those of you who mentioned "string interning" - that's the answer.

See "The lock keyword" in the following documentation: http://msdn2.microsoft.com/en-us/library/ms173179.aspx

There are other best pratices there, too.
Tuesday, March 27, 2007 6:10 AM by Sheva

# re: What's Wrong With This Code? (#13)

the code is a bit tricky just as this one:
class HardWorker
{
public void DoMultiThreadedWork(object someParameter)
{
lock (lockObject)
{
// ... lots of work ...
}
}

private Int32 lockObject = 123;
}

Sheva
Tuesday, March 27, 2007 6:24 AM by gw

# re: What's Wrong With This Code? (#13)

I may be mistaken but isn't string interning disabled by default unless specified otherwise due to possibly slow startup times (although I am not defending the above practice)?
Tuesday, March 27, 2007 8:27 AM by scott

# re: What's Wrong With This Code? (#13)

Sheva: that's a subtle one, too.

gw: as far as I know, literal string interning is still on by default. Did something change in 2.0? Do you have a link?
Wednesday, March 28, 2007 4:41 PM by gw

# re: What's Wrong With This Code? (#13)

No link I thought I read it in "CLR via C#" by Richter and looked it up really quick.

"By default when an assembly is loaded, the CLR interns all of the literal strings described in the assembly's metadata. Microsoft learned that this hurts performance significantly due to the additional hash table lookups, so it is now possible to turn this "feature" off...Note that, in an attempt to improve your applications performance, the C# compiler always specifies this attribute/flag whenever you compile an assembly."

Again, I am not saying what above is by any stretch correct, just jogged my memory of what i had read.
Monday, April 02, 2007 8:16 AM by James Curran

# re: What's Wrong With This Code? (#13)

I believe we are confusing two related, but distinct, features: String interning, and string pooling.

String Pooling has all string initialize to the same literal text, refer to a common string. This is done at compile time for a single source file, and as Ken Tong code demostrates, is on by default. (and is also common in non-managed C++ code, and probably in other languages as well). It has the advanced of space, both in run-time ram, and exe size.

String Interning expands on that concept, do assign, at run-time and across multiple assemblies, similar strings to a common string representation. This includes strings which become similar to others via manipulation. It saves run-time memory space,allowing the duplicate representations to be freed & garbage collected, and is therefore largely pointless in a non-managed environment.
Wednesday, April 04, 2007 10:01 AM by gw

# re: What's Wrong With This Code? (#13)

Ah, good point, because it is a literal it will be moved to the "constant values" section of the assembly and be shared among all other string literals of that value.

However, isn't this optimization only meant to optimize any other literals that share this value?

Since the above example is not a constant wouldn't the literal value be pooled at compile time but a new instance of a string with that literal value be created at runtime? In other words wouldn't the literal value be pooled but each instance of HardWorker would have a unique reference/pointer to a new string of that literal value?