May 30, 2012
On Wed, 30 May 2012 18:16:38 +0100, Andrei Alexandrescu <SeeWebsiteForEmail@erdani.org> wrote:
> On 5/30/12 9:43 AM, Regan Heath wrote:
>> On Wed, 30 May 2012 17:00:43 +0100, Andrei Alexandrescu
>> <SeeWebsiteForEmail@erdani.org> wrote:
>>
>>> On 5/30/12 5:32 AM, Regan Heath wrote:
>>>> On Wed, 30 May 2012 10:21:00 +0100, deadalnix <deadalnix@gmail.com>
>>>> wrote:
>>>>> You don't want to synchronize on ANY object. You want to synchronize
>>>>> on explicit mutexes.
>>>>
>>>> +1 .. this is the key point/issue.
>>>
>>> TDPL's design only allows for entire synchronized classes (not
>>> separate synchronized and unsynchronized methods), which pair mutexes
>>> with the data they protect. This is more restrictive than exposing
>>> mutexes, but in a good way. We use such a library artifact in C++ at
>>> Facebook all the time, to great success.
>>
>> Can you call pass them to a synchronized statement? i.e.
>>
>> TDPLStyleSynchClass a = new TDPLStyleSynchClass();
>> synchronized(a) {
>> }
>
> Yes. Well I recommend acquiring the text! :o)
>
>> ... because, if you can, then you're exposing the mutex.
>
> No.

For the purposes of this thread, and the anti-pattern/problem we're discussing, you are.  It is the combination of synchronized classes/methods (implicit locking) and external synchronized statements (explicit locking) which result in the unexpected, accidental, and hard to see deadlocks we're talking about here.

>>> People shouldn't create designs that have synchronized classes
>>> referring to one another naively. Designing with mutexes (explicit or
>>> implicit) will always create the possibility of deadlock, so examples
>>> how that could happen are easy to come across.
>>
>> True. But in my Example 1
>
> Your example 1 should not compile.

o.. k.. I expected you would get my meaning with the simplified example.  Here you are:

import core.thread;
import std.random;
import std.stdio;

class C
{
  synchronized void ccc() { writefln("c.ccc()"); }
}

class D
{
  synchronized void ddd() { writefln("d.ddd()"); }
}

shared C c;
shared D d;

void main()
{
  c = new shared(C)();
  d = new shared(D)();
  Thread thread1 = new Thread( &threadFunc1 );
  Thread thread2 = new Thread( &threadFunc2 );
  thread1.start();
  thread2.start();
  Thread.sleep(dur!("seconds")(60));
}

void threadFunc1()
{
  Random gen = rndGen();

  while(1)
  {
    synchronized(c)
    {
      printf("threadFunc1 sync(c) %p, obtain d.. %p\n", c, d);
      d.ddd();
    }
	
    Thread.sleep(dur!("usecs")(gen.front()));
    gen.popFront();
  }
}

void threadFunc2()
{
  Random gen = rndGen();

  while(1)
  {
    synchronized(d)
    {
      printf("threadFunc1 sync(d) %p, obtain c.. %p\n", d, c);
      c.ccc();
    }
	
    Thread.sleep(dur!("usecs")(gen.front()));
    gen.popFront();
  }
}

Runs and deadlocks immediately.

>>> TDPL improves on deadlocks by introducing synchronized statements with
>>> more than one argument, see 13.15.
>>
>> Is there anywhere I can see this online? (for free :p)
>
> http://goo.gl/ZhPM2

Thanks.

R

-- 
Using Opera's revolutionary email client: http://www.opera.com/mail/
May 30, 2012
[snip]
>>> Such an object is known to be lockable, and most object will not be. It
>>> will avoid liquid lock, because most thing will not be lockable.
>>
>> I'm celebrating day 2 of having no idea what a liquid lock is.
>
> I have to confess to this as well. I'm starting to suspect that he
> really means deadlock. Searching on Google for "liquid lock mutex"
> literally brings up this very NG thread. :)
>

Maybe it's livelock?

P.S. Just trying to snatched the prize in this little guess-game :)


-- 
Dmitry Olshansky
May 30, 2012
On Wed, 30 May 2012 13:12:47 -0400, Andrei Alexandrescu <SeeWebsiteForEmail@erdani.org> wrote:

> On 5/30/12 9:23 AM, Alex Rønne Petersen wrote:
>> On 30-05-2012 18:14, Andrei Alexandrescu wrote:
>>> This is news to me. How do you publicly access the mutex of a
>>> synchronized class object?
>>
>> Generally in two ways:
>>
>> 1) synchronized (obj)
>
> This is not accessing the mutex for arbitrary operations.

If I might interject, I think the issue is that as the author of a class, you cannot enforce that *only* the data in your class is protected by the mutex.

Yes, you can enforce that the data is protected, but there is nothing stopping a 3rd party caller from hijacking your mutex to protect other things it shouldn't be involved with.

For instance, let's say you have:

synchronized class A
{
  private int _foo;
  @property int foo() { return _foo;}
}

synchronized class B
{
   private A _a;
   @property A a() { return _a;}
   void fun() {}
}

void thread(B b)
{
   synchronized(b.a)
   {
      b.fun();
   }
}

Run multiple instances of this thread, and it will deadlock.  As the author of A, you have no control for preventing external code from hijacking your mutex for its own purposes in an incorrect way.

But take out this public access to your mutex, and you *can* enforce A's locking semantics correctly, and therefore B's semantics.  Absolutely no way can A or B's lock participate in a deadlock.

I think it is worth making this enforceable.  In other words, the synchronized(b.a) statement shouldn't compile.

Yes, you can just use a private mutex.  But doesn't that just lead to recommending not using a feature of the language?  Besides, I really like the synchronized block keyword thing.  It's a great mechanism for locking.

-Steve
May 30, 2012
On Wednesday, 30 May 2012 at 17:46:11 UTC, Dmitry Olshansky wrote:
> [snip]
>>>> Such an object is known to be lockable, and most object will not be. It will avoid liquid lock, because most thing will not be lockable.
>>>
>>> I'm celebrating day 2 of having no idea what a liquid lock is.
>>
>> I have to confess to this as well. I'm starting to suspect that he really means deadlock. Searching on Google for "liquid lock mutex" literally brings up this very NG thread. :)
>>
> Maybe it's livelock?
>
> P.S. Just trying to snatched the prize in this little guess-game :)

FWIW, I recently came across the term here:
http://schneide.wordpress.com/tag/liquid-lock/
May 30, 2012
On 5/30/12 10:28 AM, Alex Rønne Petersen wrote:
> On 30-05-2012 19:12, Andrei Alexandrescu wrote:
>> On 5/30/12 9:23 AM, Alex Rønne Petersen wrote:
>>> On 30-05-2012 18:14, Andrei Alexandrescu wrote:
>>>> This is news to me. How do you publicly access the mutex of a
>>>> synchronized class object?
>>>
>>> Generally in two ways:
>>>
>>> 1) synchronized (obj)
>>
>> This is not accessing the mutex for arbitrary operations.
>
> No, it is indeed not. You didn't explicitly say you wanted to do
> "arbitrary operations", so I assumed that by accessing the mutex you
> meant doing the only two things that are sensible for a mutex: Locking
> and unlocking it. What else would you want to do?

For example, locking it in one function and unlocking it in another. This is impossible with synchronized(obj).

To summarize, the synchronized(obj) operation is not exposing the mutex, only manipulate it in a specific way.


Andrei
May 30, 2012
On 5/30/12 10:34 AM, Alex Rønne Petersen wrote:
> On 30-05-2012 19:22, Andrei Alexandrescu wrote:
>> On 5/30/12 10:20 AM, Martin Nowak wrote:
>>> On Wed, 30 May 2012 01:10:41 +0200, Andrei Alexandrescu
>>> <SeeWebsiteForEmail@erdani.org> wrote:
>>>
>>>> On 5/29/12 4:06 PM, Alex Rønne Petersen wrote:
>>>>> Synchronized blocks are good because they
>>>>> operate on an implicit, hidden, global mutex. You can't screw up with
>>>>> that.
>>>>
>>>> I think there's quite some disconnect here. If there's any
>>>> anti-pattern in this discussion, it's operating on an implicit,
>>>> hidden, global mutex. Walter agreed to eliminate that from D, but
>>>> never got around to it.
>>>>
>>>
>>> They're not really global, it's one per synchronized block.
>>
>> That means global. They're terrible and should be eliminated from the
>> language.
>>
>>
>> Andrei
>
> So you do agree that explicit synchronization is better?

No. It's worse most of the time, and usefully more flexible sometimes.

Andrei
May 30, 2012
On Tue, 29 May 2012 00:36:13 +0200, Alex Rønne Petersen <alex@lycus.org> wrote:

> Hi,
>
> I've seen several occurrences of synchronized (this) and synchronized (this.classinfo) in both druntime and phobos by now. I propose that we officially ban these patterns with extreme prejudice.
>
> 1) Locking on the object instance is a HORRIBLE IDEA. Anyone who happens to use the object for locking will most likely end up with a deadlock on their hands.

> 2) Locking on something as fundamental as type info means that any arbitrary part of the application could cause a deadlock by doing the same.
All occurences of this pattern use it to obtain a global unique nameable mutex.
It's a little ugly but I fail to see fundamental issues.

synchronized(Stacktrace.classinfo)
synchronized(typeid(ArrayAllocLengthLock)) // uses an exclusive type as named mutex
synchronized(TaskPool.classinfo)
synchronized(this.classinfo) // this a.k.a. Socket

Then there is std.concurrency.registryLock which is used in the exact same ways as the occurences above. To do that it requires 6 lines of code, has to successfully avoid 2 pitfalls and allocates the mutex eagerly.

Then there is core.thread which has a lot of issues with locking.

There is some suspicious code in object.rt_attachDisposeEvent
which synchronizes on it's argument.

I'm inclined to say that this is a very thin backing for such a rant.


There was one interesting argument by Regan Heath:

> The problem is /not/ that you can lock any object.
> The problem is /not/ that we have synchronized(object) {}
> The problem is /not/ that we have synchronized classes/methods.

> The problem /is/ that synchronized classes/methods use a mutex which is exposed publicly, and it the same mutex as used by synchronized(object) {}.  This exposure/re-use makes deadlocks more likely to happen, and harder to spot.

Now I do not see anyone synchronizing it's code on arbitrary objects or any
other kind of abuse that would increase the inherent possibility for deadlocks.
May 30, 2012
On 5/30/12 10:40 AM, Regan Heath wrote:
> On Wed, 30 May 2012 18:16:38 +0100, Andrei Alexandrescu
> <SeeWebsiteForEmail@erdani.org> wrote:
>> On 5/30/12 9:43 AM, Regan Heath wrote:
>>> On Wed, 30 May 2012 17:00:43 +0100, Andrei Alexandrescu
>>> <SeeWebsiteForEmail@erdani.org> wrote:
>>>
>>>> On 5/30/12 5:32 AM, Regan Heath wrote:
>>>>> On Wed, 30 May 2012 10:21:00 +0100, deadalnix <deadalnix@gmail.com>
>>>>> wrote:
>>>>>> You don't want to synchronize on ANY object. You want to synchronize
>>>>>> on explicit mutexes.
>>>>>
>>>>> +1 .. this is the key point/issue.
>>>>
>>>> TDPL's design only allows for entire synchronized classes (not
>>>> separate synchronized and unsynchronized methods), which pair mutexes
>>>> with the data they protect. This is more restrictive than exposing
>>>> mutexes, but in a good way. We use such a library artifact in C++ at
>>>> Facebook all the time, to great success.
>>>
>>> Can you call pass them to a synchronized statement? i.e.
>>>
>>> TDPLStyleSynchClass a = new TDPLStyleSynchClass();
>>> synchronized(a) {
>>> }
>>
>> Yes. Well I recommend acquiring the text! :o)
>>
>>> ... because, if you can, then you're exposing the mutex.
>>
>> No.
>
> For the purposes of this thread, and the anti-pattern/problem we're
> discussing, you are.

No. I explained in my previous post that the synchronized statement does not expose locks. This is not a matter of opinion.

> It is the combination of synchronized
> classes/methods (implicit locking) and external synchronized statements
> (explicit locking) which result in the unexpected, accidental, and hard
> to see deadlocks we're talking about here.

You can have deadlocks but with synchronized you can't leak locks or doubly-unlock them. With free mutexes you have all of the above.

>>>> People shouldn't create designs that have synchronized classes
>>>> referring to one another naively. Designing with mutexes (explicit or
>>>> implicit) will always create the possibility of deadlock, so examples
>>>> how that could happen are easy to come across.
>>>
>>> True. But in my Example 1
>>
>> Your example 1 should not compile.
>
> o.. k.. I expected you would get my meaning with the simplified example.
> Here you are:

[snip]

> Runs and deadlocks immediately.

Sure. As I said, synchronized helps with scoping the locks and unlocks, but not with deadlocks. You can rewrite the example with two bare mutexes just as well.



Andrei
May 30, 2012
On 5/30/12 10:47 AM, Steven Schveighoffer wrote:
> Yes, you can just use a private mutex. But doesn't that just lead to
> recommending not using a feature of the language?

I don't think so. Synchronized classes are the unit of scoped locking in D. If you want to do all scoped locking internally, make the class private.

> Besides, I really like
> the synchronized block keyword thing. It's a great mechanism for locking.

Me too!


Andrei
May 30, 2012
On 30-05-2012 20:26, Martin Nowak wrote:
> On Tue, 29 May 2012 00:36:13 +0200, Alex Rønne Petersen
> <alex@lycus..org> wrote:
>
>> Hi,
>>
>> I've seen several occurrences of synchronized (this) and synchronized
>> (this.classinfo) in both druntime and phobos by now. I propose that we
>> officially ban these patterns with extreme prejudice.
>>
>> 1) Locking on the object instance is a HORRIBLE IDEA. Anyone who
>> happens to use the object for locking will most likely end up with a
>> deadlock on their hands.
>
>> 2) Locking on something as fundamental as type info means that any
>> arbitrary part of the application could cause a deadlock by doing the
>> same.
> All occurences of this pattern use it to obtain a global unique nameable
> mutex.
> It's a little ugly but I fail to see fundamental issues.
>
> synchronized(Stacktrace.classinfo)
> synchronized(typeid(ArrayAllocLengthLock)) // uses an exclusive type as
> named mutex
> synchronized(TaskPool.classinfo)

And what happens if I synchronize on TaskPool.classinfo in my code? The point here is that deadlocks *can* happen. If we went by the logic that "it's not likely to go wrong, so let's not care", why do we have slices? scope statements? exception handling?

> synchronized(this.classinfo) // this a.k.a. Socket
>
> Then there is std.concurrency.registryLock which is used in the exact
> same ways as the occurences above. To do that it requires 6 lines of
> code, has to successfully avoid 2 pitfalls and allocates the mutex eagerly.
>
> Then there is core.thread which has a lot of issues with locking.

What issues specifically...?

>
> There is some suspicious code in object.rt_attachDisposeEvent
> which synchronizes on it's argument.
>
> I'm inclined to say that this is a very thin backing for such a rant.
>
>
> There was one interesting argument by Regan Heath:
>
>> The problem is /not/ that you can lock any object.
>> The problem is /not/ that we have synchronized(object) {}
>> The problem is /not/ that we have synchronized classes/methods.
>
>> The problem /is/ that synchronized classes/methods use a mutex which
>> is exposed publicly, and it the same mutex as used by
>> synchronized(object) {}. This exposure/re-use makes deadlocks more
>> likely to happen, and harder to spot.
>
> Now I do not see anyone synchronizing it's code on arbitrary objects or any
> other kind of abuse that would increase the inherent possibility for
> deadlocks.

As I pointed out in another post in this thread, I've seen the pattern used several times by people asking questions on media such as IRC. IRC has much higher traffic than e.g. D.learn.

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org