On 8/29/13 2:14 PM, Jason Mehrens wrote:
Hi Daniel,
Why not take the code the opposite direction and change everything to
volatile? The properties of handler are mostly read and rarely
written. If isLoggable, getLevel, and getFilter were lock free
the LogRecords that are not loggable could barge past the publish
method. That way you would only block threads that are actually
publishing information of interest.
Jason
Hi Jason,
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.
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...
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.
best regards,
-- daniel
> Date: Thu, 29 Aug 2013 21:33:07 +1000
> From: david.hol...@oracle.com
> To: daniel.fu...@oracle.com
> Subject: Re: RFR: JDK-6823527: java.util.logging.Handler has thread
safety issues
> CC: core-libs-dev@openjdk.java.net
>
> Hi Daniel,
>
> On the surface these changes seem reasonable - the code looks the way it
> should have been written in the first place.
>
> The risk with these kinds of changes are always performance and
> deadlocks. :) I would not be surprised, given this is logging, that
> somewhere there is a path that might lead to a deadlock - but I have no
> practical way to ascertain that.
>
> Wait for a true core-libs Reviewer before pushing this one. :)
>
> David
>
> On 29/08/2013 8:42 PM, Daniel Fuchs wrote:
> > Hi,
> >
> > This is a fix for an old bug which rightfully remarks that
> > whereas access to 'level' in handler is synchronized,
> > access to other mutable fields - like 'filter' etc...
> > is not - which is inconsistent.
> >
> > <http://cr.openjdk.java.net/~dfuchs/webrev_6823527/webrev.00/>
> >
> > I have tried to keep the fix simple - simply adding
> > synchronized to method declarations when it was clear that
> > there was an inconsistency.
> >
> > I also looked at the subclasses of j.u.l.Handler
> >
> > In some places I resorted to using a synchronized block to avoid
> > calling overridable / external method from within the lock.
> >
> > The changed methods are simple accessors, and subclasses of
> > Handler are already full of synchronized methods (e.g.
> > methods like publish(...) are already synchronized)
> > so the risk of fixing these simple accessors should be limited.
> >
> > best regards,
> >
> > -- daniel
