RFR: JDK-6823527: java.util.logging.Handler has thread safety issues
daniel.fuchs at oracle.com
Thu Aug 29 15:38:48 UTC 2013
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!
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.
More information about the core-libs-dev