RFR: JDK-6823527: java.util.logging.Handler has thread safety issues
daniel.fuchs at oracle.com
Thu Aug 29 19:54:50 UTC 2013
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.
>>> 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
>> 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
> 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.
More information about the core-libs-dev