Deadlock between MethodImpl(MethodImplOptions.Synchronized) and lock(this) ?

[Joannes Vermorel]

I am currently dealing with a multithreaded application. Basically, I have
the following class:

class MyClass {

public void Foo1() {
// do something (no method call)
lock(this) { /* do something (no method call) */ }
}

[MethodImpl(MethodImplOptions.Synchronized)]
public void Foo2() {
// do something (no method call)
}
}

And with 2 threads, I manage to get a deadlock between calls to Foo1() and
Foo2(). My guess (but I may be wrong) is that somehow MethodImpl is also
taking a lock on the MyClass instance. A quick view at MSDN did not tell if
such guess was true or not.

Can anyone confirm or refute the guess ?

Thanks,
Joannes

 

[Jon Skeet [C# MVP]]

I am currently dealing with a multithreaded application. Basically, I have
the following class:

class MyClass {

public void Foo1() {
// do something (no method call)
lock(this) { /* do something (no method call) */ }
}

[MethodImpl(MethodImplOptions.Synchronized)]
public void Foo2() {
// do something (no method call)
}
}

And with 2 threads, I manage to get a deadlock between calls to Foo1() and
Foo2(). My guess (but I may be wrong) is that somehow MethodImpl is also
taking a lock on the MyClass instance. A quick view at MSDN did not tell if
such guess was true or not.

Can anyone confirm or refute the guess ?

While I don't know for absolutely sure, I'd say that's almost certainly
the case.

Personally, I'd avoid MethodImplOptions.Synchronized entirely, *and*
avoid locking on "this" - lock on a reference only available with your
class. For instance:

object dataLock = new object();

public void Foo1()
{
lock (dataLock)
{
....
}
}

 

[Joannes Vermorel]

Personally, I'd avoid MethodImplOptions.Synchronized entirely, *and*

avoid locking on "this" - lock on a reference only available with your
class. For instance:

object dataLock = new object();

public void Foo1()
{
lock (dataLock)
{
....
}
}

Thanks for the answer.

Also that's the second time you provide me an example with "object dataLock
= new object();". Until now, when I was implementing my classes, I was using
some random fields of the class for synchronization purposes. But I found
out that the use of <c>object</c>s that have no other purposes but to serve
for synchronization make IMO the code much more readable. For the cases
where memory is not a bottleneck, I found this quite an elegant practice. I
have no idea if it's commonly done this way though.

Joannes

 

[Jon Skeet [C# MVP]]

Thanks for the answer.

Also that's the second time you provide me an example with "object dataLock
= new object();". Until now, when I was implementing my classes, I was using
some random fields of the class for synchronization purposes. But I found
out that the use of <c>object</c>s that have no other purposes but to serve
for synchronization make IMO the code much more readable. For the cases
where memory is not a bottleneck, I found this quite an elegant practice. I
have no idea if it's commonly done this way though.

I think it is among those who care about multi-threading and
readability.

The way I see it, it has various benefits:

o The lock serving one purpose and one purpose only means it can be
clearly documented.

o Being an object which only the declaring class has access to means
that unexpected deadlocks due to other classes locking on it don't
occur.

I should write this up in an article some time...