Using String.Empty as a SyncLock object

[ilitirit]

Are there any reasons against writing code like this?

class Foo
{
static string _SyncLock = String.Empty;

public void ThreadSafeMethod()
{
lock (_SyncLock)
{
// Code goes here
}
}
}

[Andy]

On Mar 7, 7:51 am, [e-mail] wrote:

Are there any reasons against writing code like this?

class Foo
{
static string _SyncLock = String.Empty;

public void ThreadSafeMethod()
{
lock (_SyncLock)
{
// Code goes here
}
}

}

I don't see anything wrong with it; you're locking on an instance,
what it contains is irrelevant. An object may have less overhead
though, because it doesn't need a character array..

[Andy]

On Mar 7, 7:51 am, [e-mail] wrote:

Are there any reasons against writing code like this?

class Foo
{
static string _SyncLock = String.Empty;

public void ThreadSafeMethod()
{
lock (_SyncLock)
{
// Code goes here
}
}

}

My only concern would be that string.Empty may be a singleton, and if
other code decides to lock on it, you could head a deadlock
condition. Might be better just to lock on an object.

[Brian Gideon]

On Mar 7, 6:51 am, [e-mail] wrote:

Are there any reasons against writing code like this?

class Foo
{
static string _SyncLock = String.Empty;

public void ThreadSafeMethod()
{
lock (_SyncLock)
{
// Code goes here
}
}

}

Hi,

I'm fairly confident that String.Empty returns the same instance
everytime. That means if you're using this strategy consistently
throughout your application you'll have completely independent
sections of code competing for the same lock. Why not instantiate an
Object for this purpose?

Brian

[ilitirit]

Thanks guys. I had the feeling this was the case but I just wanted to
make sure. This code is part of a .NET 1.1 application that I'm
supposed to migrate so I guess I might as well start fixing things
like this as well.

[=?ISO-8859-1?Q?G=F6ran_Andersson?=]

il******@gmail.com wrote:

Are there any reasons against writing code like this?

class Foo
{
static string _SyncLock = String.Empty;

public void ThreadSafeMethod()
{
lock (_SyncLock)
{
// Code goes here
}
}
}

As already have been said, String.Empty always is the same instance, so
locking on that means that all locks block each other even if they are
completely independend. Use new Object() to create a unique object for
the lock.

When locking in a static method, use a static variable for locking, but
when locking in an instance method, use an instance variable for locking.

--
Göran Andersson
_____
http://www.guffa.com

[Fabio]

"Göran Andersson" <gu***@guffa.comha scritto nel messaggio news:%

As already have been said, String.Empty always is the same instance, so
locking on that means that all locks block each other even if they are
completely independend.

When you do

static string _SyncLock = String.Empty;

You create a new istance, so the problem isn't the string.Empty, but the
"static" of _SyncLock.
Of course, a

object _SyncLock = new object();

would be more clear.

[Mike Schilling]

Fabio wrote:

"Göran Andersson" <gu***@guffa.comha scritto nel messaggio news:%

>As already have been said, String.Empty always is the same instance,
so locking on that means that all locks block each other even if
they are completely independend.

When you do

static string _SyncLock = String.Empty;

You create a new istance, so the problem isn't the string.Empty, but
the "static" of _SyncLock.

Why do you say that? "=" doesn't create new instances, it assigns
references.

[Fabio]

"Mike Schilling" <[e-mail] scritto nel messaggio news:%

Why do you say that? "=" doesn't create new instances, it assigns
references.

It's a string: it's immutable.

[Willy Denoyette [MVP]]

"Fabio" <zn*******@virgilio.itwrote in message
news:O3**************@TK2MSFTNGP06.phx.gbl...

"Mike Schilling" <[e-mail] scritto nel messaggio news:%

>Why do you say that? "=" doesn't create new instances, it assigns references.

It's a string: it's immutable.

Sure, but that's not the point, you are setting a reference to an existing instance. More,
as others have said, there is only one instance of String.Empty per process, that means that
each reference to String.Empty will point to the same instance.

Willy.

[Zytan]

It's a string: it's immutable.

>
Sure, but that's not the point, you are setting a reference to an existing instance. More,
as others have said, there is only one instance of String.Empty per process, that means that
each reference to String.Empty will point to the same instance.

Exactly. MSDN is actually very clear on this point:

http://msdn2.microsoft.com/en-us/library/c5kehkcz.aspx
"lock("myLock") is a problem since any other code in the process using
the same string, will share the same lock."

Zytan

[Jon Skeet [C# MVP]]

<il******@gmail.comwrote:

Are there any reasons against writing code like this?

Absolutely - *anything* else that decides to do the same thing will be
using the same lock, even though you really don't want to! It's just
*waiting* for a deadlock...

Why use that rather than just a plain new object?

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[Jon Skeet [C# MVP]]

Fabio <zn*******@virgilio.itwrote:

"Mike Schilling" <[e-mail] scritto nel messaggio news:%

Why do you say that? "=" doesn't create new instances, it assigns
references.

It's a string: it's immutable.

That's irrelevant - or in fact, it's the only reason that string.Empty
*can* legitimately return a reference to the same object every time).
Try this:

using System;

class Test
{
static void Main()
{
string x = string.Empty;
string y = string.Empty;

Console.WriteLine (object.ReferenceEquals(x,y));
}
}

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too