On 8/29/13 7:58 PM, Mandy Chung wrote:
On 8/29/13 9:46 AM, Daniel Fuchs wrote:
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/>
Looks good. I also agree that making those fields to be volatile is a
good suggestion and only requires writer to acquire the lock.
A minor comment: The setter methods in Handler class are synchronized
at the method entry whereas StreamHandler.setPushLevel synchronizes
after checkPermission() is called. checkPermission doesn't need to be
synchronized. Either way is fine with me and it'd be good to do it
consistently.
Hi Mandy,
Are you suggesting I should put checkPermission() within
the synchronized block? It might be more consistent with
what the other setters do. Although as you state it does
not matter much.
(its' MemoryHandler - not StreamHandler BTW)
best regards,
-- daniel
Mandy
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
