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

[David Holmes]

On 30/08/2013 6:38 PM, Daniel Fuchs wrote:
> On 8/30/13 9:21 AM, David Holmes wrote:
>> StreamHandler.java
>>
>> writer doesn't need to be volatile - AFAICS it is only accessed within
>> synchronized methods.
>
> Ah - no - it's called in isLoggable() actually. The whole point of using
> volatiles instead of regular synchronization was to allow isLoggable()
> to be called concurrently with e.g. publish() - so I would think
> it's better to make writer volatile too.

Agreed. I simply didn't spot writer in isLoggable.

David

> Thanks for the valuable comments! I will update the webrev and send out
> a new revision...
>
> best regards,
>
> -- daniel
>
>>
>> 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
>>>>>>
>>>>>
>>>
>