RFR: JDK-6823527: java.util.logging.Handler has thread safety issues
jason_mehrens at hotmail.com
Thu Aug 29 18:07:12 UTC 2013
Looks good. Shouldn't 'boolean sealed' be declared volatile?
> 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.
> 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
More information about the core-libs-dev