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

[Jason Mehrens]

Looks good.  Shouldn't 'boolean sealed' be declared volatile?
 
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
> >
>