View mode: basic / threaded / horizontal-split · Log in · Help
May 28, 2012
synchronized (this[.classinfo]) in druntime and phobos
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?

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org
May 28, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On Tuesday, May 29, 2012 00:36:13 Alex Rønne Petersen wrote:
> 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?

Don't synchronized classes synchronize on the object for all of their function 
calls? How is this any different?

- Jonathan M Davis
May 28, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 29-05-2012 00:42, Jonathan M Davis wrote:
> On Tuesday, May 29, 2012 00:36:13 Alex Rønne Petersen wrote:
>> 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?
>
> Don't synchronized classes synchronize on the object for all of their function
> calls? How is this any different?
>
> - Jonathan M Davis

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.

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)

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org
May 28, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
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.



- Jonathan M Davis
May 28, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
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
May 28, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
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?

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org
May 28, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
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.

>
> But I take it you agree that synchronized (this) and similar
> "uncontrollable" synchronization blocks should be avoided?
>

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org
May 28, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
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
May 28, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
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
May 28, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
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.

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org
« First   ‹ Prev
1 2 3 4 5
Top | Discussion index | About this forum | D home