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

[Daniel Fuchs]

On 8/29/13 8:07 PM, Jason Mehrens wrote:
> Looks good.  Shouldn't 'boolean sealed' be declared volatile?

I don't think so because it's only mutated from within the
constructor. Once the Handler is built - 'sealed' never changes.

-- daniel

>
> Jason
>
>  > Date: Thu, 29 Aug 2013 18:46:53 +0200
>  > From: daniel.fuchs at oracle.com
>  > To: core-libs-dev at openjdk.java.net
>  > Subject: Re: RFR: JDK-6823527: java.util.logging.Handler has thread
> safety issues
>  >
>  > 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
>  > >
>  >