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.fu...@oracle.com > To: core-libs-dev@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 > > >
