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! -- 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
