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

[Daniel Fuchs]

On 8/29/13 5:38 PM, Daniel Fuchs wrote:
> Hi Jason,
>
> Yes - that makes sense. I think we want the setter to use the
> same lock than e.g. publish because we don't want the level
> (or filter or encoding or whatever) to be changed while
> publish is executing.
>
> However I see no harm in allowing other threads to read the
> variables values while setters/publish are executing - since
> all the writes (I means setters) only write 1 single value.
>
> OK - unless I hear an objection to that I'll prepare a new
> changeset where the variables will be volatile, the getters
> will not be synchronized, but the setters will.
>
> Thanks for your feedback!

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/>

best regards,

-- daniel


>
> -- daniel
>
> On 8/29/13 5:29 PM, Jason Mehrens wrote:
>>  > Yes - this is a possibility I envisaged too.
>>  > But would that be better?
>>  >
>>  > I didn't want to remove any synchronized blocks because I'm
>>  > really not sure of what consequences it might have.
>>  > getLevel()/setLevel() are already synchronized on Handler.
>>  > It is my belief that most operation already need to call
>>  > getLevel(). So synchronizing on getFilter/setFilter etc.
>>  > should not be such a big issue.
>> The only reason that I can think of for synchronizing any of the
>> j.u.Handler methods is that the code was created prior to the JMM
>> revision of volatile in JDK 1.5.  The lock policy is required for the 3
>> abstract methods so that a single LogRecord is published all or
>> nothing.  The level, filter, error manager, encoding could all be
>> declared volatile with no locking.  For the subclasses the locking for
>> publish is too coarse.  The isLoggable method should be called outside
>> the lock.
>>
>>  > Also - synchronizing on 'this' has the advantage that the lock
>>  > can be shared with subclasses - I mean - that's what a
>>  > subclass would usually expect...
>> The same was true for COWList
>> (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6282140) but, the
>> lock policy changed over time.   This is bug is about thread safety
>> issues so subclass should have little expectations.
>>
>>
>>  > But if I hear strong opinions that 'volatile' is definitely better
>>  > for this case I'm prepared to alter my patch.
>>  > I personally tend to use 'volatile' only sparsely - when it's clear
>>  > that it's the better solution.
>> The common case is that Handler.getFoo are a hot methods and
>> Handler.setFoo are cold.  You could always declare the level, filter,
>> error manager, and encoding volatile and have only the setFoo methods
>> synchronize when setting the property.
>> Jason
>