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

[Daniel Fuchs]

On 8/29/13 7:58 PM, Mandy Chung wrote:
> 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.

Hi Mandy,

Are you suggesting I should put checkPermission() within
the synchronized block? It might be more consistent with
what the other setters do. Although as you state it does
not matter much.

(its' MemoryHandler - not StreamHandler BTW)

best regards,

-- daniel



>
> 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
>>>
>>
>