Thread overview | |||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
December 22, 2011 Some Issues With Synchronized | ||||
---|---|---|---|---|
| ||||
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. |
December 23, 2011 Re: Some Issues With Synchronized | ||||
---|---|---|---|---|
| ||||
Posted in reply to Andrew Wiley | 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
|
December 24, 2011 Re: Some Issues With Synchronized | ||||
---|---|---|---|---|
| ||||
Posted in reply to Somedude | 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.
|
December 24, 2011 Re: Some Issues With Synchronized | ||||
---|---|---|---|---|
| ||||
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");
}
}
---
|
January 04, 2012 Re: Some Issues With Synchronized | ||||
---|---|---|---|---|
| ||||
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.
|
January 04, 2012 Re: Some Issues With Synchronized | ||||
---|---|---|---|---|
| ||||
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.
|
January 04, 2012 Re: Some Issues With Synchronized | ||||
---|---|---|---|---|
| ||||
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.
|
January 05, 2012 Re: Some Issues With Synchronized | ||||
---|---|---|---|---|
| ||||
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.
|
January 05, 2012 Re: Some Issues With Synchronized | ||||
---|---|---|---|---|
| ||||
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.
|
January 05, 2012 Re: Some Issues With Synchronized | ||||
---|---|---|---|---|
| ||||
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.
|
Copyright © 1999-2021 by the D Language Foundation