I'm having some issues getting the synchronized modifier to work like a synchronized block. Sorry this code example is long, but here's three dummy implementations of a Condition-based work queue:

---import std.stdio;
import core.sync.mutex;
import core.sync.condition;
import core.thread;

synchronized class Queue1 {
private:
    bool _work;
    Condition _cond;
public:
    this() {
        auto lock = new Mutex(this); // initialize the monitor for
this object so we can use the same lock in the Condition
        _cond = new Condition(lock);
        _work = false;
    }
    void doWork() {
        while(!_work) (cast()_cond).wait();
        _work = false;
        writeln("did work");
        return;
    }
    void addWork() {
        _work = true;
        (cast()_cond).notify();
        writeln("added work");
    }
}

class Queue2 {
private:
    bool _work;
    Condition _cond;
public:
    this() {
        auto lock = new Mutex(this); // again, initialize the monitor
explicitly so we can reuse the lock
        _cond = new Condition(lock);
        _work = false;
    }
    synchronized void doWork() {
        while(!_work) (cast()_cond).wait();
        _work = false;
        writeln("did work");
        return;
    }
    synchronized void addWork() {
        _work = true;
        (cast()_cond).notify();
        writeln("added work");
    }
}

class Queue3 {
private:
    bool _work;
    Condition _cond;
    Mutex _lock;
public:
    this() {
        _lock = new Mutex(this); // make our own lock and ignore the monitor
        _cond = new Condition(_lock);
        _work = false;
    }
    void doWork() shared {
        synchronized(_lock) {
            while(!_work) (cast()_cond).wait();
            _work = false;
            writeln("did work");
            return;
        }
    }
    void addWork() shared {
        synchronized(_lock) {
            _work = true;
            (cast()_cond).notify();
            writeln("added work");
       }
    }
}

void main() {
    version(Q1) auto queue = new shared(Queue1)();
    version(Q2) auto queue = new shared(Queue2)();
    version(Q3) auto queue = new shared(Queue3)();
    auto thread = new Thread({
        while(true) queue.doWork();
        });
    thread.start();
    while(true) queue.addWork();
}
---

As you can see, this program starts a producer thread and a consumer thread, and it should endlessly print "did work" and "added work". Because the consumer has to wake up from the condition variable periodically, the producer tends to print far more often.

So first, according to the D spec, I believe these three queues should behave identically. Is that true?

Here's what happens:
Queue1:
  DMD: crash
  GDC: crash
    Both print errors that look like this:
core.sync.exception.SyncException@src/core/sync/mutex.d(159): Unable
to unlock mutex
    From GDC's stack trace, it looks like the constructor call is
being synchronized, so it crashes when I swap out the lock halfway
through the constructor. Do we really need to synchronize constructor
calls? I've never heard of another language that does this.

Queue2:
  DMD: works
  GDC: deadlock
    Unless someone can see an issue here, I guess this is a GDC bug
and should be handled separately.

Queue3:
  DMD: works
  GDC: works
    This is about the most problematic way to do locking, but it does
seem to work reliably.

Le 22/12/2011 10:47, Andrew Wiley a écrit :
> I'm having some issues getting the synchronized modifier to work like a synchronized block. Sorry this code example is long, but here's three dummy implementations of a Condition-based work queue:
> 
> ---import std.stdio;
> import core.sync.mutex;
> import core.sync.condition;
> import core.thread;
> 
> synchronized class Queue1 {
> private:
>     bool _work;
>     Condition _cond;
> public:
>     this() {
>         auto lock = new Mutex(this); // initialize the monitor for
> this object so we can use the same lock in the Condition
>         _cond = new Condition(lock);
>         _work = false;
>     }
>     void doWork() {
>         while(!_work) (cast()_cond).wait();
>         _work = false;
>         writeln("did work");
>         return;
>     }
>     void addWork() {
>         _work = true;
>         (cast()_cond).notify();
>         writeln("added work");
>     }
> }
> 
> class Queue2 {
> private:
>     bool _work;
>     Condition _cond;
> public:
>     this() {
>         auto lock = new Mutex(this); // again, initialize the monitor
> explicitly so we can reuse the lock
>         _cond = new Condition(lock);
>         _work = false;
>     }
>     synchronized void doWork() {
>         while(!_work) (cast()_cond).wait();
>         _work = false;
>         writeln("did work");
>         return;
>     }
>     synchronized void addWork() {
>         _work = true;
>         (cast()_cond).notify();
>         writeln("added work");
>     }
> }
> 
> class Queue3 {
> private:
>     bool _work;
>     Condition _cond;
>     Mutex _lock;
> public:
>     this() {
>         _lock = new Mutex(this); // make our own lock and ignore the monitor
>         _cond = new Condition(_lock);
>         _work = false;
>     }
>     void doWork() shared {
>         synchronized(_lock) {
>             while(!_work) (cast()_cond).wait();
>             _work = false;
>             writeln("did work");
>             return;
>         }
>     }
>     void addWork() shared {
>         synchronized(_lock) {
>             _work = true;
>             (cast()_cond).notify();
>             writeln("added work");
>        }
>     }
> }
> 
> void main() {
>     version(Q1) auto queue = new shared(Queue1)();
>     version(Q2) auto queue = new shared(Queue2)();
>     version(Q3) auto queue = new shared(Queue3)();
>     auto thread = new Thread({
>         while(true) queue.doWork();
>         });
>     thread.start();
>     while(true) queue.addWork();
> }
> ---
> 
> As you can see, this program starts a producer thread and a consumer thread, and it should endlessly print "did work" and "added work". Because the consumer has to wake up from the condition variable periodically, the producer tends to print far more often.
> 
> So first, according to the D spec, I believe these three queues should behave identically. Is that true?
> 
> Here's what happens:
> Queue1:
>   DMD: crash
>   GDC: crash
>     Both print errors that look like this:
> core.sync.exception.SyncException@src/core/sync/mutex.d(159): Unable
> to unlock mutex
>     From GDC's stack trace, it looks like the constructor call is
> being synchronized, so it crashes when I swap out the lock halfway
> through the constructor. Do we really need to synchronize constructor
> calls? I've never heard of another language that does this.
> 
> Queue2:
>   DMD: works
>   GDC: deadlock
>     Unless someone can see an issue here, I guess this is a GDC bug
> and should be handled separately.
> 
> Queue3:
>   DMD: works
>   GDC: works
>     This is about the most problematic way to do locking, but it does
> seem to work reliably.

On windows XP with DMD 2.057, I get
Queue1: deadlock
Queue2: works
Queue3: works

On Fri, Dec 23, 2011 at 1:25 AM, Somedude <lovelydear@mailmetrash.com> wrote:
> On windows XP with DMD 2.057, I get
> Queue1: deadlock
> Queue2: works
> Queue3: works

Yes, I posted another (much shorter) post describing the issue with Queue1. In short, since Queue1 is a synchronized class, the constructor is synchronized (which is mostly worthless). As a consequence, when I replace the lock in the middle of the function, bad things happen when the runtime tries to unlock the lock at the end and sees the new lock.

This version of Queue1 shows a very hacky way to get around this:
---
synchronized class Queue1 {private:   bool _work;   Condition
_cond;public:   this() {       auto lock = new Mutex(this); //
initialize the monitor for this object so we can use the same lock in
the Condition       lock.lock(); // HACK: acquire the lock so we can
unlock it at the end of the function       _cond = new
Condition(lock);       _work = false;   }   void doWork() {
while(!_work) (cast()_cond).wait();       _work = false;
writeln("did work");       return;   }   void addWork() {       _work
= true;       (cast()_cond).notify();       writeln("added work");
}}---
Queue2 looks like a bug where GDC is acquiring all locks twice in
synchronized functions, and since the condition variable only unlocks
the lock once, a deadlock results. I'll get a bug report up about it
shortly.

On Fri, Dec 23, 2011 at 8:33 PM, Andrew Wiley <wiley.andrew.j@gmail.com> wrote:
> On Fri, Dec 23, 2011 at 1:25 AM, Somedude <lovelydear@mailmetrash.com> wrote:
>> On windows XP with DMD 2.057, I get
>> Queue1: deadlock
>> Queue2: works
>> Queue3: works
>
> Yes, I posted another (much shorter) post describing the issue with Queue1. In short, since Queue1 is a synchronized class, the constructor is synchronized (which is mostly worthless). As a consequence, when I replace the lock in the middle of the function, bad things happen when the runtime tries to unlock the lock at the end and sees the new lock.
>
> This version of Queue1 shows a very hacky way to get around this:
> ---
> synchronized class Queue1 {private:   bool _work;   Condition
> _cond;public:   this() {       auto lock = new Mutex(this); //
> initialize the monitor for this object so we can use the same lock in
> the Condition       lock.lock(); // HACK: acquire the lock so we can
> unlock it at the end of the function       _cond = new
> Condition(lock);       _work = false;   }   void doWork() {
> while(!_work) (cast()_cond).wait();       _work = false;
> writeln("did work");       return;   }   void addWork() {       _work
> = true;       (cast()_cond).notify();       writeln("added work");
> }}---
> Queue2 looks like a bug where GDC is acquiring all locks twice in
> synchronized functions, and since the condition variable only unlocks
> the lock once, a deadlock results. I'll get a bug report up about it
> shortly.

Gah, my hacked/fixed Queue1 got garbled:
---
synchronized class Queue1 {
private:
    bool _work;
    Condition _cond;
public:
    this() {
        auto lock = new Mutex(this); // initialize the monitor for this object
        // so we can use the same lock in the Condition
        lock.lock(); // HACK: acquire the lock so we can unlock it
        // at the end of the function
        _cond = new Condition(lock);
        _work = false;
    }
    void doWork() {
        while(!_work) (cast()_cond).wait();
        _work = false;
        writeln("did work");
        return;
    }
    void addWork() {
        _work = true;
        (cast()_cond).notify();
        writeln("added work");
    }
}
---

On Dec 23, 2011, at 8:33 PM, Andrew Wiley wrote:

> On Fri, Dec 23, 2011 at 1:25 AM, Somedude <lovelydear@mailmetrash.com> wrote:
>> On windows XP with DMD 2.057, I get
>> Queue1: deadlock
>> Queue2: works
>> Queue3: works
> 
> Yes, I posted another (much shorter) post describing the issue with
> Queue1. In short, since Queue1 is a synchronized class, the
> constructor is synchronized (which is mostly worthless).

The ctor should really be shared.

On Dec 23, 2011, at 8:49 PM, Andrew Wiley wrote:

> On Fri, Dec 23, 2011 at 8:33 PM, Andrew Wiley <wiley.andrew.j@gmail.com> wrote:
>> On Fri, Dec 23, 2011 at 1:25 AM, Somedude <lovelydear@mailmetrash.com> wrote:
>>> On windows XP with DMD 2.057, I get
>>> Queue1: deadlock
>>> Queue2: works
>>> Queue3: works
>> 
>> Yes, I posted another (much shorter) post describing the issue with Queue1. In short, since Queue1 is a synchronized class, the constructor is synchronized (which is mostly worthless). As a consequence, when I replace the lock in the middle of the function, bad things happen when the runtime tries to unlock the lock at the end and sees the new lock.
>> 
>> This version of Queue1 shows a very hacky way to get around this:
>> ---
>> synchronized class Queue1 {private:   bool _work;   Condition
>> _cond;public:   this() {       auto lock = new Mutex(this); //
>> initialize the monitor for this object so we can use the same lock in
>> the Condition       lock.lock(); // HACK: acquire the lock so we can
>> unlock it at the end of the function       _cond = new
>> Condition(lock);       _work = false;   }   void doWork() {
>> while(!_work) (cast()_cond).wait();       _work = false;
>> writeln("did work");       return;   }   void addWork() {       _work
>> = true;       (cast()_cond).notify();       writeln("added work");
>> }}---
>> Queue2 looks like a bug where GDC is acquiring all locks twice in
>> synchronized functions, and since the condition variable only unlocks
>> the lock once, a deadlock results. I'll get a bug report up about it
>> shortly.
> 
> Gah, my hacked/fixed Queue1 got garbled:
> ---
> synchronized class Queue1 {
> private:
>    bool _work;
>    Condition _cond;
> public:
>    this() {
>         auto lock = new Mutex(this); // initialize the monitor for this object

This assumes that there is no existing monitor for the object.  At best you'll get a memory leak here.

On Wed, Jan 4, 2012 at 2:12 PM, Sean Kelly <sean@invisibleduck.org> wrote:
> On Dec 23, 2011, at 8:49 PM, Andrew Wiley wrote:
>
>> On Fri, Dec 23, 2011 at 8:33 PM, Andrew Wiley <wiley.andrew.j@gmail.com> wrote:
>>> On Fri, Dec 23, 2011 at 1:25 AM, Somedude <lovelydear@mailmetrash.com> wrote:
>>>> On windows XP with DMD 2.057, I get
>>>> Queue1: deadlock
>>>> Queue2: works
>>>> Queue3: works
>>>
>>> Yes, I posted another (much shorter) post describing the issue with Queue1. In short, since Queue1 is a synchronized class, the constructor is synchronized (which is mostly worthless). As a consequence, when I replace the lock in the middle of the function, bad things happen when the runtime tries to unlock the lock at the end and sees the new lock.
>>>
>>> This version of Queue1 shows a very hacky way to get around this:
>>> ---
>>> synchronized class Queue1 {private:   bool _work;   Condition
>>> _cond;public:   this() {       auto lock = new Mutex(this); //
>>> initialize the monitor for this object so we can use the same lock in
>>> the Condition       lock.lock(); // HACK: acquire the lock so we can
>>> unlock it at the end of the function       _cond = new
>>> Condition(lock);       _work = false;   }   void doWork() {
>>> while(!_work) (cast()_cond).wait();       _work = false;
>>> writeln("did work");       return;   }   void addWork() {       _work
>>> = true;       (cast()_cond).notify();       writeln("added work");
>>> }}---
>>> Queue2 looks like a bug where GDC is acquiring all locks twice in
>>> synchronized functions, and since the condition variable only unlocks
>>> the lock once, a deadlock results. I'll get a bug report up about it
>>> shortly.
>>
>> Gah, my hacked/fixed Queue1 got garbled:
>> ---
>> synchronized class Queue1 {
>> private:
>>    bool _work;
>>    Condition _cond;
>> public:
>>    this() {
>>         auto lock = new Mutex(this); // initialize the monitor for this object
>
> This assumes that there is no existing monitor for the object.  At best you'll get a memory leak here.

Then is there any way to safely use this sort of idiom? Putting it on the first line of the constructor was the earliest way I could see to try to swap the lock.

On Jan 4, 2012, at 2:55 PM, Andrew Wiley wrote:

> On Wed, Jan 4, 2012 at 2:12 PM, Sean Kelly <sean@invisibleduck.org> wrote:
>> 
>> 
>> This assumes that there is no existing monitor for the object.  At best you'll get a memory leak here.
> 
> Then is there any way to safely use this sort of idiom? Putting it on the first line of the constructor was the earliest way I could see to try to swap the lock.

Not currently.  The relevant ctor for Mutex actually has an "assert(o.__monitor is null)" clause in it that simply never triggers because we ship a "release" build (I hate that label).  The problem is that replacing an existing monitor means a race condition, since other threads might be trying to lock it or are waiting on it at the time.  The real issue is that the ctor for a synchronized module shouldn't be synchronized, but I'll grant that it's easier to fix this in user/library code than sort out a compiler fix.  What you can do is:
---
extern (C) void _d_monitordelete(Object h, bool det);
synchronized class Queue1 {
private:
   bool _work;
   Condition _cond;
public:
   this() {
        _d_monitordelete(this, true);
        auto lock = new Mutex(this); // initialize the monitor for this object
       // so we can use the same lock in the Condition
       lock.lock(); // HACK: acquire the lock so we can unlock it
       // at the end of the function
       _cond = new Condition(lock);
       _work = false;
   }
   ...
}
---
Obviously not ideal since it requires a lot of hackery, but it should work as desired.

On Wed, Jan 4, 2012 at 6:12 PM, Sean Kelly <sean@invisibleduck.org> wrote:
> On Jan 4, 2012, at 2:55 PM, Andrew Wiley wrote:
>
>> On Wed, Jan 4, 2012 at 2:12 PM, Sean Kelly <sean@invisibleduck.org> wrote:
>>>
>>>
>>> This assumes that there is no existing monitor for the object.  At best you'll get a memory leak here.
>>
>> Then is there any way to safely use this sort of idiom? Putting it on the first line of the constructor was the earliest way I could see to try to swap the lock.
>
> Not currently.  The relevant ctor for Mutex actually has an "assert(o.__monitor is null)" clause in it that simply never triggers because we ship a "release" build (I hate that label).  The problem is that replacing an existing monitor means a race condition, since other threads might be trying to lock it or are waiting on it at the time.  The real issue is that the ctor for a synchronized module shouldn't be synchronized, but I'll grant that it's easier to fix this in user/library code than sort out a compiler fix.  What you can do is:
> ---
> extern (C) void _d_monitordelete(Object h, bool det);
> synchronized class Queue1 {
> private:
>   bool _work;
>   Condition _cond;
> public:
>   this() {
>        _d_monitordelete(this, true);
>        auto lock = new Mutex(this); // initialize the monitor for this object
>       // so we can use the same lock in the Condition
>       lock.lock(); // HACK: acquire the lock so we can unlock it
>       // at the end of the function
>       _cond = new Condition(lock);
>       _work = false;
>   }
>   ...
> }
> ---
> Obviously not ideal since it requires a lot of hackery, but it should work as desired.

I haven't filed a bug yet about constructors for synchronized classes
being synchronized. Should I?
I believe it's wrong and unhelpful, but I haven't really gotten any
feedback and I'm afraid I may have missed something.

It's just like ctors of invariant classes being invariant. Initialization should be exempt from the rules surrounding other accesses.

Sent from my iPhone

On Jan 4, 2012, at 5:06 PM, Andrew Wiley <wiley.andrew.j@gmail.com> wrote:

> On Wed, Jan 4, 2012 at 6:12 PM, Sean Kelly <sean@invisibleduck.org> wrote:
>> On Jan 4, 2012, at 2:55 PM, Andrew Wiley wrote:
>> 
>>> On Wed, Jan 4, 2012 at 2:12 PM, Sean Kelly <sean@invisibleduck.org> wrote:
>>>> 
>>>> 
>>>> This assumes that there is no existing monitor for the object.  At best you'll get a memory leak here.
>>> 
>>> Then is there any way to safely use this sort of idiom? Putting it on the first line of the constructor was the earliest way I could see to try to swap the lock.
>> 
>> Not currently.  The relevant ctor for Mutex actually has an "assert(o.__monitor is null)" clause in it that simply never triggers because we ship a "release" build (I hate that label).  The problem is that replacing an existing monitor means a race condition, since other threads might be trying to lock it or are waiting on it at the time.  The real issue is that the ctor for a synchronized module shouldn't be synchronized, but I'll grant that it's easier to fix this in user/library code than sort out a compiler fix.  What you can do is:
>> ---
>> extern (C) void _d_monitordelete(Object h, bool det);
>> synchronized class Queue1 {
>> private:
>>   bool _work;
>>   Condition _cond;
>> public:
>>   this() {
>>        _d_monitordelete(this, true);
>>        auto lock = new Mutex(this); // initialize the monitor for this object
>>       // so we can use the same lock in the Condition
>>       lock.lock(); // HACK: acquire the lock so we can unlock it
>>       // at the end of the function
>>       _cond = new Condition(lock);
>>       _work = false;
>>   }
>>   ...
>> }
>> ---
>> Obviously not ideal since it requires a lot of hackery, but it should work as desired.
> 
> I haven't filed a bug yet about constructors for synchronized classes
> being synchronized. Should I?
> I believe it's wrong and unhelpful, but I haven't really gotten any
> feedback and I'm afraid I may have missed something.