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

[Daniel Fuchs]

On 8/29/13 9:45 PM, Mandy Chung wrote:
> On 8/29/13 11:04 AM, Daniel Fuchs wrote:
>> 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.
>
> Having a second thought - do the setters really need to be
> synchronized?   My guess is that StreamHandler.publish is synchronized
> so as to ensure that the log messages are sequential and not interpersed
> when multiple threads are publishing.  The spec is unclear.  Perhaps it
> worths looking into this further?

I believe at least setEncodings() needs synchronization.
setLevel() had it - so are we going to cause subtle race conditions
if we remove it?

My own feeling is that keeping setters synchronized might be better.

We could of course say that a subclass which needs to prevent
e.g. setFormatter() from being called concurrently with publish()
should override setFormatter just for the purpose of synchronizing
it...

> One more minor note: MemoryHandler.pushLevel can also be made volatile.
> L262: look like the log manager is not needed in the existing code.

hmmm. Right. It's already called once in the super class so removing
this call should have no effect. I will remove it.

-- daniel

>
> Mandy
>