RFR: JDK-6823527: java.util.logging.Handler has thread safety issues

[Mandy Chung]

On 8/29/13 9:46 AM, Daniel Fuchs wrote:
> Here is the new webrev implementing Jason's suggestion.
> Compared to previous webrev only Handler.java & StreamHandler.java
> have changed.
>
> <http://cr.openjdk.java.net/~dfuchs/webrev_6823527/webrev.01/>
>

Looks good.  I also agree that making those fields to be volatile is a 
good suggestion and only requires writer to acquire the lock.

A minor comment:  The setter methods in Handler class are synchronized 
at the method entry whereas StreamHandler.setPushLevel synchronizes 
after checkPermission() is called.  checkPermission doesn't need to be 
synchronized.  Either way is fine with me and it'd be good to do it 
consistently.

Mandy

> best regards,
>
> -- daniel
>
>
>>
>> -- daniel
>>
>> On 8/29/13 5:29 PM, Jason Mehrens wrote:
>>>  > Yes - this is a possibility I envisaged too.
>>>  > But would that be better?
>>>  >
>>>  > I didn't want to remove any synchronized blocks because I'm
>>>  > really not sure of what consequences it might have.
>>>  > getLevel()/setLevel() are already synchronized on Handler.
>>>  > It is my belief that most operation already need to call
>>>  > getLevel(). So synchronizing on getFilter/setFilter etc.
>>>  > should not be such a big issue.
>>> The only reason that I can think of for synchronizing any of the
>>> j.u.Handler methods is that the code was created prior to the JMM
>>> revision of volatile in JDK 1.5.  The lock policy is required for the 3
>>> abstract methods so that a single LogRecord is published all or
>>> nothing.  The level, filter, error manager, encoding could all be
>>> declared volatile with no locking.  For the subclasses the locking for
>>> publish is too coarse.  The isLoggable method should be called outside
>>> the lock.
>>>
>>>  > Also - synchronizing on 'this' has the advantage that the lock
>>>  > can be shared with subclasses - I mean - that's what a
>>>  > subclass would usually expect...
>>> The same was true for COWList
>>> (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6282140) but, the
>>> lock policy changed over time.   This is bug is about thread safety
>>> issues so subclass should have little expectations.
>>>
>>>
>>>  > But if I hear strong opinions that 'volatile' is definitely better
>>>  > for this case I'm prepared to alter my patch.
>>>  > I personally tend to use 'volatile' only sparsely - when it's clear
>>>  > that it's the better solution.
>>> The common case is that Handler.getFoo are a hot methods and
>>> Handler.setFoo are cold.  You could always declare the level, filter,
>>> error manager, and encoding volatile and have only the setFoo methods
>>> synchronize when setting the property.
>>> Jason
>>
>