RFR: JDK-6823527: java.util.logging.Handler has thread safety issues

[Daniel Fuchs]

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.holmes at oracle.com
>  > To: daniel.fuchs at oracle.com
>  > Subject: Re: RFR: JDK-6823527: java.util.logging.Handler has thread
> safety issues
>  > CC: core-libs-dev at 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