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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Alex Rønne Petersen | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Jonathan M Davis | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Alex Rønne Petersen | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Alex Rønne Petersen | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Jonathan M Davis | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Alex Rønne Petersen | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Alex Rønne Petersen | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Alex Rønne Petersen | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Jonathan M Davis | 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 |
Copyright © 1999-2021 by the D Language Foundation