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

Tuesday, March 27, 2007

 

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.


Comments
James Newton-King Tuesday, March 27, 2007
There is the possibility that the string could be interned and used in a lock somewhere else. Both strings would be the same object.
David Hogue Tuesday, March 27, 2007
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.
lucas Tuesday, March 27, 2007
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.
Eric W. Bachtal Tuesday, March 27, 2007
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.
singhhome Tuesday, March 27, 2007
string Interning can cause multiple objects to have a common reference, so all instances will have the same sync lock index.
Ken Tong Tuesday, March 27, 2007
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));
Moz Tuesday, March 27, 2007
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.
der Igel Tuesday, March 27, 2007
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.
marshall Tuesday, March 27, 2007
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.
Wayne Howarth Tuesday, March 27, 2007
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.
Kieron Tuesday, March 27, 2007
Hiya, will it be that you're locking a new instance of a string each time. So the lock is pointless?
James Tuesday, March 27, 2007
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.
scott Tuesday, March 27, 2007
For those of you who mentioned "string interning" - that's the answer.

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

There are other best pratices there, too.
Sheva Tuesday, March 27, 2007
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
gw Tuesday, March 27, 2007
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)?
scott Tuesday, March 27, 2007
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?
gw Wednesday, March 28, 2007
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.
James Curran Monday, April 2, 2007
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.
gw Wednesday, April 4, 2007
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?
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!