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.

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.

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.

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.

string Interning can cause multiple objects to have a common reference, so all instances will have the same sync lock index.

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));

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.

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.

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.

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.

Hiya, will it be that you're locking a new instance of a string each time. So the lock is pointless?

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.

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.

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

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)?

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?

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.

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.

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?