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

David Holmes david.holmes at oracle.com
Fri Aug 30 01:27:52 UTC 2013

Hi Daniel,

As usual with these kinds of changes we start by trying to complete an 
incomplete/incorrect synchronization pattern and get diverted into 
changing the synchronization pattern. :) The latter of course has more 
risk than the former.

Making everything synchronized has the benefit that we don't need to 
analyse the "real" methods to check what would happen if one of these 
data values changed mid-method. Might be harmless, might not. Atomicity 
makes things easy to reason about.

Making the fields volatile and un-sync'ing the getters is a potential 
optimization for readers. But are these fields that are read frequently 
by multiple threads? The downside of doing so is a negative performance 
impact inside the "real" methods where every volatile field access 
requires (platform-dependent) memory barriers.


On 30/08/2013 5:54 AM, Daniel Fuchs wrote:
> 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

More information about the core-libs-dev mailing list