On 29-05-2012 01:46, Jonathan M Davis wrote:
> On Tuesday, May 29, 2012 01:38:25 Alex Rønne Petersen wrote:
>> I should probably add that Java learned it long ago, and yet we adopted
>> it anyway... blergh.
>
> The "lesson learned" from Java that TDPL enumerates is the mistake of having
> synchronized on functions rather than entire classes, but clearly even that is
> currently TDPL-only and not actually properly implemented yet.
>
> - Jonathan M Davis

But synchronized on entire classes is exactly equally flawed... the fundamental problem is still that they synchronize on a public resource.

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

On Tuesday, May 29, 2012 01:54:59 Alex Rønne Petersen wrote:
> On 29-05-2012 01:41, Jonathan M Davis wrote:
> > On Tuesday, May 29, 2012 01:35:23 Alex Rønne Petersen wrote:
> >> I don't think arguing about them makes sense at this point. Way too much code would break if we changed the semantics. I'd consider it a mistake and a lesson learned, rather.
> >> 
> >> But I take it you agree that synchronized (this) and similar "uncontrollable" synchronization blocks should be avoided?
> > 
> > I'm not an expert on threading stuf, but it would be my opinion that if you're not intending to protect the entire class with locks that it makes no sense to lock on the class itself. You're locking for something specific, in which case, your sychronized block should be locking on something else specific to what you're trying to protect. Certainly, that's how I'd approach it with mutexes. You don't have a mutex for an entire class unless it's actually used for all of the class' functions. Rather, you use mutexes specific to what you're trying to protect.
> > 
> > - Jonathan M Davis
> 
> Right, but even if you really *are* protecting the entire class, you can still create mysterious deadlocks if users of your code lock on your class. So I'm arguing that no matter the use case, never lock on a 'this' reference exposed outside of some API layer.

Well, it seems pretty abysmal to me to be locking on something that you don't control. Making a mutex that your class used public would just be stupid. With synchronized classes/functions, you're basically creating and using an implicit mutex for the whole class, which would then be the same as if you locked it at the beginning of every member function call and unlocked it at its end, which doesn't expose the mutex at all. So, I don't really see te problem there would have to study the matter more to see what the issue there is. But locking on another class rather than something specifically intended as a mutex does seem to me like it's asking for trouble. But maybe that mindset comes from using mutexes primarily as opposed to synchronized statements (as most of that sort of programming that I've done has been in C++), and I don't necessarily understand all of the ways that programmers screw up with synchronized blocks.

- Jonathan M Davis

On Monday, 28 May 2012 at 23:55:04 UTC, Alex Rønne Petersen wrote:
> On 29-05-2012 01:46, Jonathan M Davis wrote:
>> On Tuesday, May 29, 2012 01:38:25 Alex Rønne Petersen wrote:
>>> I should probably add that Java learned it long ago, and yet we adopted
>>> it anyway... blergh.
>>
>> The "lesson learned" from Java that TDPL enumerates is the mistake of having
>> synchronized on functions rather than entire classes, but clearly even that is
>> currently TDPL-only and not actually properly implemented yet.
>>
>> - Jonathan M Davis
>
> But synchronized on entire classes is exactly equally flawed... the fundamental problem is still that they synchronize on a public resource.

The fundamental problem is shared memory ;)
NMS

Le 29/05/2012 00:36, Alex Rønne Petersen a écrit :
> 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.
>
> The solution to (1) is to simply use a Mutex internally (or an Object
> with synchronized statements), and for (2), to simply use private global
> objects.
>
> (Now, regarding (1), you might argue that anyone who locks on an
> arbitrary object is doing it wrong, but you can't really blame them -
> it's frankly D's fault for allowing monitors on arbitrary objects, which
> is a horrible mess.)
>
> Anyone against this?
>

I already did some comment about this.

Making any object synchronization is a very bad design decision. It is deadlock prone, it is liquid lock prone, it cause an overhead on any object, and even worse, it is useless most of the time as D promote thread locality (which is very good).

OOP and concurrency are 2 orthogonal topics, and should be handled as such.

Le 29/05/2012 01:38, Alex Rønne Petersen a écrit :
> On 29-05-2012 01:35, Alex Rønne Petersen wrote:
>> On 29-05-2012 01:24, Jonathan M Davis wrote:
>>> On Tuesday, May 29, 2012 01:11:49 Alex Rønne Petersen wrote:
>>>> I have no idea how synchronized classes work; they are not a documented
>>>> feature of the language. We have synchronized functions which
>>>> synchronize on the this reference. Perhaps synchronized classes just
>>>> make all functions in a class do this.
>>>
>>> Per TDPL, having individually synchronized functions is illegal.
>>> Either all
>>> of the functions in a class are synchronized or none of them are.
>>> Putting
>>> synchronized on a function should actually be illegal. It belongs
>>> only on
>>> classes or in synchronized blocks, never on functions.
>>>
>>> Now, unfortuntately, I don't believe that the compiler enforces any of
>>> that
>>> right now, so you end up synchronizing indivdual functions rather than
>>> whole
>>> classes, but the synchronized functions themselves should function the
>>> same.
>>>
>>>> Either way, this is a fundamental language design fallacy. This is
>>>> anti-pattern number one when it comes to locking:
>>>>
>>>> * http://stackoverflow.com/a/251668/438034
>>>> * http://msdn.microsoft.com/en-us/library/ms173179.aspx (The lock and
>>>> SyncLock Keywords section)
>>>
>>> Well, then you should probably be arguing about the design of
>>> synchronized
>>> classes/functions rather than synchronized(this). However, given the
>>> design of
>>> synchronized classes, synchronized(this) would probably never be
>>> appropriate.
>>> If you're using a synchronized class, then you don't need it. And if
>>> you're
>>> not using a synchronized class, then you shouldn't be synchronizing your
>>> class. I would definitely think that synchronized blocks should
>>> synchronize on
>>> something else, since they're trying to lock a much smaller area than an
>>> entire class.
>>>
>>> - Jonathan M Davis
>>
>> I don't think arguing about them makes sense at this point. Way too much
>> code would break if we changed the semantics. I'd consider it a mistake
>> and a lesson learned, rather.
>
> I should probably add that Java learned it long ago, and yet we adopted
> it anyway... blergh.
>

That is what I was about to say. No point of doing D if it is to repeat previously done errors.

Le 29/05/2012 01:35, Alex Rønne Petersen a écrit :
> On 29-05-2012 01:24, Jonathan M Davis wrote:
>> On Tuesday, May 29, 2012 01:11:49 Alex Rønne Petersen wrote:
>>> I have no idea how synchronized classes work; they are not a documented
>>> feature of the language. We have synchronized functions which
>>> synchronize on the this reference. Perhaps synchronized classes just
>>> make all functions in a class do this.
>>
>> Per TDPL, having individually synchronized functions is illegal.
>> Either all
>> of the functions in a class are synchronized or none of them are. Putting
>> synchronized on a function should actually be illegal. It belongs only on
>> classes or in synchronized blocks, never on functions.
>>
>> Now, unfortuntately, I don't believe that the compiler enforces any of
>> that
>> right now, so you end up synchronizing indivdual functions rather than
>> whole
>> classes, but the synchronized functions themselves should function the
>> same.
>>
>>> Either way, this is a fundamental language design fallacy. This is
>>> anti-pattern number one when it comes to locking:
>>>
>>> * http://stackoverflow.com/a/251668/438034
>>> * http://msdn.microsoft.com/en-us/library/ms173179.aspx (The lock and
>>> SyncLock Keywords section)
>>
>> Well, then you should probably be arguing about the design of
>> synchronized
>> classes/functions rather than synchronized(this). However, given the
>> design of
>> synchronized classes, synchronized(this) would probably never be
>> appropriate.
>> If you're using a synchronized class, then you don't need it. And if
>> you're
>> not using a synchronized class, then you shouldn't be synchronizing your
>> class. I would definitely think that synchronized blocks should
>> synchronize on
>> something else, since they're trying to lock a much smaller area than an
>> entire class.
>>
>> - Jonathan M Davis
>
> I don't think arguing about them makes sense at this point. Way too much
> code would break if we changed the semantics. I'd consider it a mistake
> and a lesson learned, rather.
>
> But I take it you agree that synchronized (this) and similar
> "uncontrollable" synchronization blocks should be avoided?
>

I would say that breaking things here, with the right deprecation process, is the way to go.

shared isn't working properly ATM, so anyway, things will have to change in regard of shared memory support in the language.

On Tue, 29 May 2012 01:08:59 +0100, Jonathan M Davis <jmdavisProg@gmx.com> wrote:

> On Tuesday, May 29, 2012 01:54:59 Alex Rønne Petersen wrote:
>> On 29-05-2012 01:41, Jonathan M Davis wrote:
>> > On Tuesday, May 29, 2012 01:35:23 Alex Rønne Petersen wrote:
>> >> I don't think arguing about them makes sense at this point. Way too  
>> much
>> >> code would break if we changed the semantics. I'd consider it a  
>> mistake
>> >> and a lesson learned, rather.
>> >>
>> >> But I take it you agree that synchronized (this) and similar
>> >> "uncontrollable" synchronization blocks should be avoided?
>> >
>> > I'm not an expert on threading stuf, but it would be my opinion that  
>> if
>> > you're not intending to protect the entire class with locks that it  
>> makes
>> > no sense to lock on the class itself. You're locking for something
>> > specific, in which case, your sychronized block should be locking on
>> > something else specific to what you're trying to protect. Certainly,
>> > that's how I'd approach it with mutexes. You don't have a mutex for an
>> > entire class unless it's actually used for all of the class'  
>> functions.
>> > Rather, you use mutexes specific to what you're trying to protect.
>> >
>> > - Jonathan M Davis
>>
>> Right, but even if you really *are* protecting the entire class, you can
>> still create mysterious deadlocks if users of your code lock on your
>> class. So I'm arguing that no matter the use case, never lock on a
>> 'this' reference exposed outside of some API layer.
>
> Well, it seems pretty abysmal to me to be locking on something that you don't control. Making a mutex that your class used public would just be stupid.

Interestingly this is what C#s Array type does with SyncRoot (intentionally).

> With synchronized classes/functions, you're basically creating and using an
> implicit mutex for the whole class, which would then be the same as if you locked it at the beginning of every member function call and unlocked it at its end, which doesn't expose the mutex at all. So, I don't really see te
> problem there would have to study the matter more to see what the issue there is.

According to the docs here:
http://dlang.org/class.html#synchronized-functions

A synchronized function locks "this" and as "this" is exposed publicly...  In the following code the "lock" statement and "synchronized void bar" lock the same mutex.

class Foo {
  synchronized void bar() { ...statements... }
}

void main()
{
  Foo foo = new Foo();
  lock(foo)
  {
    ...statements...
  }
}

> But locking on another class rather than something specifically intended as a mutex does seem to me like it's asking for trouble.

Yep.  It commonly arises where you have a class/object which is either not synchronized itself (because it might be used in single threaded situations and you want performance) like a collection class for example, or is synchronized but you need to lock a sequence of member function calls i.e. a lookup and insert on the collection.  What happens then, is people just call lock(<object>) and end up locking the same mutex as the class does internally.  What happens next to cause a deadlock is that inside that lock they call one or more functions or methods which internally call methods on another synchronized object.  This results in a locking pattern of object1, object2.  In another piece of code, running in another thread, they do something similar which results in a locking pattern of object2, object1 and eventually these threads will deadlock each other.

R

-- 
Using Opera's revolutionary email client: http://www.opera.com/mail/

On Tue, 29 May 2012 13:07:29 +0100, Regan Heath <regan@netmail.co.nz> wrote:
> void main()
> {
>    Foo foo = new Foo();
>    lock(foo)
>    {
>      ...statements...
>    }
> }

Apologies..I've been writing more C# than D lately.  "lock" above should be "synchronized" for D.

R

-- 
Using Opera's revolutionary email client: http://www.opera.com/mail/

On 29.05.2012 16:07, Regan Heath wrote:
>
> According to the docs here:
> http://dlang.org/class.html#synchronized-functions
>
> A synchronized function locks "this" and as "this" is exposed
> publicly... In the following code the "lock" statement and "synchronized
> void bar" lock the same mutex.
>
> class Foo {
> synchronized void bar() { ...statements... }
> }
>
> void main()
> {
> Foo foo = new Foo();
> lock(foo)
> {
> ...statements...
> }
> }
>
>> But locking on another class rather than something specifically
>> intended as a mutex does seem to me like it's asking for trouble.

I'd be darned but every Object in D has monitor fields. If I'm not mistaken it's the mutex you are looking for ;)


-- 
Dmitry Olshansky

On 29-05-2012 14:07, Regan Heath wrote:
> On Tue, 29 May 2012 01:08:59 +0100, Jonathan M Davis
> <jmdavisProg@gmx.com> wrote:
>
>> On Tuesday, May 29, 2012 01:54:59 Alex Rønne Petersen wrote:
>>> On 29-05-2012 01:41, Jonathan M Davis wrote:
>>> > On Tuesday, May 29, 2012 01:35:23 Alex Rønne Petersen wrote:
>>> >> I don't think arguing about them makes sense at this point. Way
>>> too much
>>> >> code would break if we changed the semantics. I'd consider it a
>>> mistake
>>> >> and a lesson learned, rather.
>>> >>
>>> >> But I take it you agree that synchronized (this) and similar
>>> >> "uncontrollable" synchronization blocks should be avoided?
>>> >
>>> > I'm not an expert on threading stuf, but it would be my opinion
>>> that if
>>> > you're not intending to protect the entire class with locks that it
>>> makes
>>> > no sense to lock on the class itself. You're locking for something
>>> > specific, in which case, your sychronized block should be locking on
>>> > something else specific to what you're trying to protect. Certainly,
>>> > that's how I'd approach it with mutexes. You don't have a mutex for an
>>> > entire class unless it's actually used for all of the class'
>>> functions.
>>> > Rather, you use mutexes specific to what you're trying to protect.
>>> >
>>> > - Jonathan M Davis
>>>
>>> Right, but even if you really *are* protecting the entire class, you can
>>> still create mysterious deadlocks if users of your code lock on your
>>> class. So I'm arguing that no matter the use case, never lock on a
>>> 'this' reference exposed outside of some API layer.
>>
>> Well, it seems pretty abysmal to me to be locking on something that
>> you don't control. Making a mutex that your class used public would
>> just be stupid.
>
> Interestingly this is what C#s Array type does with SyncRoot
> (intentionally).

Microsoft has officially dismissed the SyncRoot approach to synchronization. I don't have a link handy, but there's an article *somewhere* on MSDN about it.

>
>> With synchronized classes/functions, you're basically creating and
>> using an
>> implicit mutex for the whole class, which would then be the same as if
>> you locked it at the beginning of every member function call and
>> unlocked it at its end, which doesn't expose the mutex at all. So, I
>> don't really see te
>> problem there would have to study the matter more to see what the
>> issue there is.
>
> According to the docs here:
> http://dlang.org/class.html#synchronized-functions
>
> A synchronized function locks "this" and as "this" is exposed
> publicly.... In the following code the "lock" statement and
> "synchronized void bar" lock the same mutex.
>
> class Foo {
> synchronized void bar() { ...statements... }
> }
>
> void main()
> {
> Foo foo = new Foo();
> lock(foo)
> {
> ...statements...
> }
> }
>
>> But locking on another class rather than something specifically
>> intended as a mutex does seem to me like it's asking for trouble.
>
> Yep. It commonly arises where you have a class/object which is either
> not synchronized itself (because it might be used in single threaded
> situations and you want performance) like a collection class for
> example, or is synchronized but you need to lock a sequence of member
> function calls i.e. a lookup and insert on the collection. What happens
> then, is people just call lock(<object>) and end up locking the same
> mutex as the class does internally. What happens next to cause a
> deadlock is that inside that lock they call one or more functions or
> methods which internally call methods on another synchronized object.
> This results in a locking pattern of object1, object2. In another piece
> of code, running in another thread, they do something similar which
> results in a locking pattern of object2, object1 and eventually these
> threads will deadlock each other.
>
> R
>

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