View mode: basic / threaded / horizontal-split · Log in · Help
January 07, 2012
Re: Some Issues With Synchronized
On Wed, 04 Jan 2012 19:12:04 -0500, 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.

Can Mutex do this?

I'm not sure a synchronized class shouldn't be locked in the ctor, it's  
entirely feasible for a synchronized ctor to place its this reference in  
some global location for other threads to try and lock.

But if Mutex can check if there's an existing lock, make sure it's  
unlocked, then replace it with a new one, I think it should be sound, as  
long as you do it in the ctor before exposing the 'this' reference  
somewhere.

-Steve
January 07, 2012
Re: Some Issues With Synchronized
There's still a race condition between unlocking the monitor and freeing the memory. 

Sent from my iPhone

On Jan 7, 2012, at 7:25 AM, "Steven Schveighoffer" <schveiguy@yahoo.com> wrote:

> On Wed, 04 Jan 2012 19:12:04 -0500, 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.
> 
> Can Mutex do this?
> 
> I'm not sure a synchronized class shouldn't be locked in the ctor, it's entirely feasible for a synchronized ctor to place its this reference in some global location for other threads to try and lock.
> 
> But if Mutex can check if there's an existing lock, make sure it's unlocked, then replace it with a new one, I think it should be sound, as long as you do it in the ctor before exposing the 'this' reference somewhere.
> 
> -Steve
January 07, 2012
Re: Some Issues With Synchronized
On Sat, 07 Jan 2012 11:08:51 -0500, Sean Kelly <sean@invisibleduck.org>  
wrote:

> There's still a race condition between unlocking the monitor and freeing  
> the memory.

Wouldn't that result in a segfault or other error?  How is that worse than  
the current state of affairs?

-Steve
January 07, 2012
Re: Some Issues With Synchronized
Right now, Mutex will assert if you assign it to an object with a monitor. Or it would with a non-release build. That's the only entirely safe thing to do. 

Sent from my iPhone

On Jan 7, 2012, at 9:27 AM, "Steven Schveighoffer" <schveiguy@yahoo.com> wrote:

> On Sat, 07 Jan 2012 11:08:51 -0500, Sean Kelly <sean@invisibleduck.org> wrote:
> 
>> There's still a race condition between unlocking the monitor and freeing the memory.
> 
> Wouldn't that result in a segfault or other error?  How is that worse than the current state of affairs?
> 
> -Steve
January 09, 2012
Re: Some Issues With Synchronized
On Sat, 07 Jan 2012 18:07:33 -0500, Sean Kelly <sean@invisibleduck.org>  
wrote:

> Right now, Mutex will assert if you assign it to an object with a  
> monitor. Or it would with a non-release build.

In other words, it doesn't :)

-Steve
January 09, 2012
Re: Some Issues With Synchronized
Which goes back to my argument that we should ship both checked and unchecked (release and not) builds and the compiler should link the proper one. 

Sent from my iPhone

On Jan 9, 2012, at 6:01 AM, "Steven Schveighoffer" <schveiguy@yahoo.com> wrote:

> On Sat, 07 Jan 2012 18:07:33 -0500, Sean Kelly <sean@invisibleduck.org> wrote:
> 
>> Right now, Mutex will assert if you assign it to an object with a monitor. Or it would with a non-release build.
> 
> In other words, it doesn't :)
> 
> -Steve
January 09, 2012
Re: Some Issues With Synchronized
On Mon, 09 Jan 2012 09:41:40 -0500, Sean Kelly <sean@invisibleduck.org>  
wrote:

> Which goes back to my argument that we should ship both checked and  
> unchecked (release and not) builds and the compiler should link the  
> proper one.

Or you make it a release-mode check.  I can't imagine this being an  
inner-loop operation.

But my point was, it's already unsafe.  We've not had horrible issues with  
it in its current state.  Is it worse to allow replacing the monitor?  If  
you take the right steps (i.e. do it first line of the ctor), it's not  
going to result in a race condition.

-Steve
January 09, 2012
Re: Some Issues With Synchronized
I know. But once I add the ability to replace a mutex there's no way to detect how it's being used. Though as its in core I guess the user should simply be trusted to do the right thing. 

Sent from my iPhone

On Jan 9, 2012, at 6:57 AM, "Steven Schveighoffer" <schveiguy@yahoo.com> wrote:

> On Mon, 09 Jan 2012 09:41:40 -0500, Sean Kelly <sean@invisibleduck.org> wrote:
> 
>> Which goes back to my argument that we should ship both checked and unchecked (release and not) builds and the compiler should link the proper one.
> 
> Or you make it a release-mode check.  I can't imagine this being an inner-loop operation.
> 
> But my point was, it's already unsafe.  We've not had horrible issues with it in its current state.  Is it worse to allow replacing the monitor?  If you take the right steps (i.e. do it first line of the ctor), it's not going to result in a race condition.
> 
> -Steve
January 10, 2012
Re: Some Issues With Synchronized
On Mon, Jan 9, 2012 at 8:41 AM, Sean Kelly <sean@invisibleduck.org> wrote:
> Which goes back to my argument that we should ship both checked and unchecked (release and not) builds and the compiler should link the proper one.
>
> Sent from my iPhone
>

I agree. This is common in the C/C++ world, and it seems even more
important in D since we have so many DbC features that disappear
completely in release-mode binaries.
If this isn't supported, DbC is worthless in library code.
Next ›   Last »
1 2
Top | Discussion index | About this forum | D home