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