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

[David Holmes]

Hi Daniel,

I'm fine with the approach in (c), just wanted to highlight the 
performance considerations (volatile and synchronized need not be 
equivalent in the uncontended case - biased-locking can mean there are 
no barriers emitted.)

A few specific comments on [2]:

Handler.java:

reportError doesn't need changing now that the field is volatile and you 
aren't doing sync.

MemoryHandler.java:

pushLevel should/could be volatile and the sync removed from getPushLevel.

In setPushLevel this seems to be unused code:

  262         LogManager manager = LogManager.getLogManager();

unless there is some obscure side-effect in calling getLogManager ??

setPushLevel might as well also be synchronized at the method level. All 
the Handler methods already include checkPermission inside the 
sync-block. Though I'm actually more inclined to change all the existing 
code to use sync-blocks so that checkPermission is done outside the lock 
region. But a consistent approach either way would be preferable.

StreamHandler.java

writer doesn't need to be volatile - AFAICS it is only accessed within 
synchronized methods.

Thanks,
David

On 30/08/2013 4:45 PM, Daniel Fuchs wrote:
> On 8/30/13 3:27 AM, David Holmes wrote:
>> 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.
> Agreed. We can analyze the 'real methods' of Handler classes in the JDK.
> But I'm sure there are plenty of
> subclasses of handlers in applications out there that we don't know
> about. So we can't really be sure
> what the effect of changing synchronization might have on those subclasses.
>
>> 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?
>
> At least three of them are accessed by isLoggable() which I believe is
> called very frequently by
> multiple threads (level, filter, and writer).
>
>> The downside of doing so is a negative performance impact inside the
>> "real" methods where every volatile field access requires
>> (platform-dependent) memory barriers.
> That - I think - should not be such an issue since the fields are private.
> I mean - we can see whether there are methods that do multiple access to
> the fields in the
> classes that define these fields - and fix that if necessary.
> For subclasses, they will have to call the getter, which will require
> synchronization anyway.
> I mean - for subclasses - isn't the fact that the getter accesses a
> volatile field or the fact that the
> getter is synchronized equivalent in terms of performance?
>
> JDK 8 is a major version of the JDK - so I think if we want to fix this
> bug it's
> probably better to do it now rather than in a minor update.
>
> It seems we have 5 choices:
>
> a. Not fixing
> b. Using regular 'synchronized' pattern [1]
> c. Using volatiles, synchronize setters only [2]
> d. Using volatiles, only synchronize setters when obviously necessary
> e. other? (like introducing new private locks - but I don't think
> anybody would want that)
>
> [1] <http://cr.openjdk.java.net/~dfuchs/webrev_6823527/webrev.00/>
> [2] <http://cr.openjdk.java.net/~dfuchs/webrev_6823527/webrev.01/>
>
> My own preference would lean to either b) or c).
>
> -- daniel
>
>
>>
>> David
>>
>> 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
>>>>
>>>
>