View mode: basic / threaded / horizontal-split · Log in · Help
May 30, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On Wed, 30 May 2012 16:56:51 -0400, deadalnix <deadalnix@gmail.com> wrote:

> Le 30/05/2012 22:31, Steven Schveighoffer a écrit :
>> On Wed, 30 May 2012 15:48:47 -0400, Andrei Alexandrescu
>> <SeeWebsiteForEmail@erdani.org> wrote:
>>
>>> To clarify:
>>>
>>> On 5/30/12 12:25 PM, Alex Rønne Petersen wrote:
>>>> The mutex may not be
>>>> directly exposed in the sense that you can obtain a reference (though  
>>>> in
>>>> reality in all compiler implementations, you can), but it is exposed  
>>>> in
>>>> the sense that you can lock and unlock it, which is a mutation of  
>>>> state
>>>> and program flow.
>>>
>>> synchronized (object) {
>>> writeln("about to unlock the object");
>>> XXX
>>> writeln("unlocked the object");
>>> }
>>>
>>> Replace "XXX" with a construct that unlocks the object.
>>
>> This is not what we are talking about.
>>
>> I guess the easiest way to say it is, the API used to "lock and then
>> subsequently unlock" the mutex. That is, even this:
>>
>> Object o = new Object;
>>
>> synchronized(o)
>> {
>> }
>>
>> is exposing the mutex in such a way that you can interfere with the
>> semantic meaning of the mutex, and cause preventable deadlocks.
>>
>> It would be preferrable for:
>>
>> synchronized(o)
>> {
>> }
>>
>> to be an error, unless typeof(o) allows it explicitly.
>>
>> I gave you a case where you can have a deadlock, even with simple fully
>> synchronized classes. You might say, "yeah, but all mutexes can have
>> deadlocks!".
>>
>> My contention is that if the default is you do *not* expose the mutex to
>> external callers, there *cannot* be a deadlock.
>>
>> Here is the example again:
>>
>> 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();
>> }
>> }
>>
>> If synchronized(b.a) is valid, deadlock can occur. If it's invalid,
>> deadlock *cannot* occur.
>>
>> -Steve
>
> This cannot remove all deadlocks, but yes, it goes in the right  
> direction.

All deadlocks involving A and B mutexes.  Or if not, can you show how?  I  
can't see it.

-Steve
May 30, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 5/30/12 1:13 PM, deadalnix wrote:
> Le 30/05/2012 21:58, Andrei Alexandrescu a écrit :
>> It's getting where I was the whole time.
>>
>> Andrei
>
> So we have a communication problem.

Not me! :o)

Andrei
May 30, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On Wed, May 30, 2012 at 5:30 PM, Steven Schveighoffer
<schveiguy@yahoo.com> wrote:
> All deadlocks involving A and B mutexes.  Or if not, can you show how?  I
> can't see it.
>
> -Steve

This can cause a deadlock because locking order is not guaranteed, no?

synchronized class A {
 void delegate(B b) { b.call() }
 void call() {}
}

synchronized class B {
 void delegate(A a) { a.call() }
 void call() {}
}

Thanks,
-Jose
PS. This may not compile not sure if it should be 'void
delegate(shared B b)', etc.
May 30, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 5/30/12 1:31 PM, Steven Schveighoffer wrote:
> On Wed, 30 May 2012 15:48:47 -0400, Andrei Alexandrescu
> <SeeWebsiteForEmail@erdani.org> wrote:
>> synchronized (object) {
>> writeln("about to unlock the object");
>> XXX
>> writeln("unlocked the object");
>> }
>>
>> Replace "XXX" with a construct that unlocks the object.
>
> This is not what we are talking about.

Oh yes it is. "Expose the mutex" means "make the two mutex primitive 
operations lock() and unlock() freely usable against the mutex object".

> I guess the easiest way to say it is, the API used to "lock and then
> subsequently unlock" the mutex. That is, even this:
>
> Object o = new Object;
>
> synchronized(o)
> {
> }
>
> is exposing the mutex in such a way that you can interfere with the
> semantic meaning of the mutex, and cause preventable deadlocks.
>
> It would be preferrable for:
>
> synchronized(o)
> {
> }
>
> to be an error, unless typeof(o) allows it explicitly.

Yah, it should be only allowed for synchronized classes.

> I gave you a case where you can have a deadlock, even with simple fully
> synchronized classes. You might say, "yeah, but all mutexes can have
> deadlocks!".

Not only I might say that, I'm actually gonna say it. Yeah, but all 
mutexes can have deadlocks!

> My contention is that if the default is you do *not* expose the mutex to
> external callers, there *cannot* be a deadlock.

That's shuffling the fundamental issue from client code to library code. 
This can be done in either approach (synchronized vs. raw mutex), except 
it's clunkier with raw mutex.

> Here is the example again:
>
> 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();
> }
> }
>
> If synchronized(b.a) is valid, deadlock can occur. If it's invalid,
> deadlock *cannot* occur.

I don't get what this is supposed to prove.


Andrei
May 30, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
Le 30/05/2012 23:41, Andrei Alexandrescu a écrit :
> On 5/30/12 1:13 PM, deadalnix wrote:
>> Le 30/05/2012 21:58, Andrei Alexandrescu a écrit :
>>> It's getting where I was the whole time.
>>>
>>> Andrei
>>
>> So we have a communication problem.
>
> Not me! :o)
>
> Andrei

Happy to know that you understand yourself <:o) . Now let's simply work 
on a mind to mind broadcasting system !
May 31, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
> And what happens if I synchronize on TaskPool.classinfo in my code?
You don't own that class, you haven't written it and it's not meant to be
extended so why would you do that. Likewise the C# article [1] said
it'd be a bad idea to lock a literal string and the liquid-lock bug [2]
just locks nearby data to synchronize it's code. That this should be a
common source of bugs strikes me as rather odd, but then one could already
wonder why 'lock' is used intransitively. Saying 'lock on sth.' when you  
actually
want to lock sth. (data or resource) just pinpoints the underlying issue.

For example the analysis in [2] misses the actual point of the bug. He  
wanted to achieve
mutual exclusive usage of a channel but instead of locking the channel he  
locked a message.

[1] http://msdn.microsoft.com/en-us/library/ms173179.aspx
[2] http://schneide.wordpress.com/tag/liquid-lock/

For me this whole discussion boils down to the following.
Locks are quite safe if you use them to actually protect a resource.
Locks are very unsafe if you use them as part of an implicit protocol.
May 31, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
>> FWIW, I recently came across the term here:
>> http://schneide.wordpress.com/tag/liquid-lock/
>
> I explained that in another post a few minutes ago, and yes, this is it.
>
> Maybe i should write an article on that, I though it was more well known.

> The main problem here is the object the lock is acquired upon: the  
> reference of lastMessage is mutable! We call this a liquid lock, because  
> the lock isn’t as solid as it should be. It’s one of the more hideous  
> multithreading pitfalls as it looks like everything’s fine at first  
> glance.

You should try to name the real root of the bug.
It's trying to serialize access to a channel by locking the messages.

There is nothing fancy about this, it has nothing to do with mutability
it's just about locking the wrong resource.
May 31, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On Wed, 30 May 2012 20:45:51 +0200, Alex Rønne Petersen <alex@lycus.org>  
wrote:

> Also, I don't think calling this a rant is fair. I gave specific reasons  
> for why the patterns I mentioned are bad in _libraries_ of all things  
> (notice that I was talking about druntime and phobos!).

Sorry, I didn't meant to be harsh.
But please reread your initial post.
It's important to emphasize the specific reasons in
an objective way to start off a solid discussion.
Words like extreme, horrible, mess, ban, blame or fault
are counterproductive.
May 31, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 2012-05-30 21:10, Andrei Alexandrescu wrote:

> I see how these can be annoying, but they're not the result of us not
> designing things. We designed things best we could.

I would say it's not good enough. The whole approach of designing the 
language is wrong. This is how it works today, which is bad:

1. Designing feature X
2. Implementing in the compiler
3. Shipped with the next release

Andrei, Walter: "Hey look at this new feature X in the next release, 
it's great".

Community: "Say what now. What did that come from?".

Some time later

Community: "X is broken in these different ways. X can't integrate with 
Y, Z, W. Since Phobos doesn't use the feature we can't use it, making it 
useless"
Andrei, Walter: "No, it's perfectly designed"
Community: "But it's not working in practice"
Andrei, Walter: "Sure it is, end of discussion"

This is the bad approach, but there's also a worse approach:

1. Designing feature X
2. Document the feature in TDPL
3. Wait wait wait
4. Community: "Where is feature X, it's in TDPL"
5. Andrei, Walter: "It's not implemented yet/correctly"
6. Repeat step 3-5 a couple of times
7. Implement feature X in the compiler
8. Ship with the next release

Community: "X is broken in these different ways. X can't integrate with 
Y, Z, W. Since Phobos doesn't use the feature we can't use it, making it 
useless"
Andrei, Walter: "No, it's perfectly designed"
Community: "But it's not working in practice"
Andrei, Walter: "It can't be changed, it's already in TDPL, it's written 
in stone"

What should have been done is something like this:

1. Designing feature X
2. Show the new feature for the community
3. Consider the feedback and possible tweak/redesign
4. Implementing in an experimental branch of the compiler
5. Release an experimental version with just this feature
6. Repeat step 3-5 until satisfied or put on hold/drop the idea
7. Prepare Phobos and druntime for the new feature
8. Move the implementation to the main branch
9. Ship feature X with the next release
10. wait
11. Fix bugs for feature X
12. Repeat step 10-11 a couple of times
13. Write about it in TDPL

-- 
/Jacob Carlborg
May 31, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 2012-05-30 21:58, Andrei Alexandrescu wrote:
> On 5/30/12 12:52 PM, deadalnix wrote:
>> Ha, this is getting somewhere :D
>
> It's getting where I was the whole time.
>
> Andrei

No, the original problem was that you can synchronize on ALL objects, 
regardless if they're marked with "synchronized" or not. This it how it 
works today.

-- 
/Jacob Carlborg
14 15 16 17 18 19 20 21 22
Top | Discussion index | About this forum | D home