8029281: Synchronization issues in Logger and LogManager

Daniel Fuchs daniel.fuchs at oracle.com
Tue Dec 3 18:57:31 UTC 2013


Hi Mandy,

I have updated the webrev taking your comments into account.

<http://cr.openjdk.java.net/~dfuchs/webrev_8029281/webrev.02/>

Changes are small and outlined below:

On 12/3/13 1:49 AM, Mandy Chung wrote:
> On 11/29/2013 3:41 AM, Daniel Fuchs wrote:
>> Hi,
>>
>> Here is a new revision that includes Peter's findings.
>> Thanks again for that Peter!
>>
>> <http://cr.openjdk.java.net/~dfuchs/webrev_8029281/webrev.01/>
>>
>
> LogManager.java
>     L156: make props volatile is good.
>
>     I think the reason why LoggerWeakRef.node and parentRef don't need
> to be volatile because of the synchronized block to check and set
> disposed flag that ensures that only one thread will be doing the node
> and parentRef cleanup.  I agree that the two lines should be done with
> synchronized on the LoggerContext.
>
>       node.context.removeLogger(name);
>       node.loggerRef = null;  // clear LogNode's weak ref to us
>
> I don't see anything obvious wrong with this patch.   Have you
> considered having the dispose() method simply synchronizes on
> node.context (probably need to get the LoggerContext when instantiating
> LoggerWeakRef and break dispose method into two - one for the node and
> the other for the parentRef)?    I'm not proposing to do this in JDK 8
> since we are ramping down and get a low risk fix in.   It may be
> something to revisit for jdk 9 to a cleaner version.

I believe that nulling the instance variables in LoggerWeakRef
is probably no longer needed - since we now should be calling
dispose only once - but I didn't want to change the code too
much. I agree it would be better to revisit that in 9.


> Logger.java
>     The assert this.loggerBundle == NO_RESOURCE_BUNDLE; in the Logger
> constructors doesn't seem to be needed.

OK - removed.

> Having an immutable LoggerBundle class is good to ensure the
> resourceBundleName and resourceBundle are consistent.
>
>     L1930: is lb.userBundle and lb.resourceBundleName null when getting
> to this line?  Might be better to rename setupResourceInfo to
> setResourceBundleName (that creates a LoggerBundle with a null RB). On
> the other hand,  setResourceBundle will instantiate a LoggerBundle with
> non-null rbname and RB.  It wasn't obvious to me what does what.

Right. Let's keep the old name for now. We can revisit the naming in 9.
You right that when we reach the last line lb.userBundle must be null,
otherwise we would have either returned or thrown an exception, since
lb.userBundle != null => lb.resourceBundleName != null.

Therefore I added an assert lb.userBundle == null and
replaced the call to
   LoggerBundle.get(name, lb.userBundle)
by
   LoggerBundle.get(name, null)

Hopefully it will make what happens more obvious.

> 2137             final String rbName = getResourceBundleName();
> 2138             if (!SYSTEM_LOGGER_RB_NAME.equals(rbName)
> 2139                     &&
> !SYSTEM_LOGGER_RB_NAME.equals(b.getBaseBundleName())) {
> 2140                 return LoggerBundle.get(rbName, b);
> 2141             } else {
> 2142                 return SYSTEM_BUNDLE;
> 2143             }
>
> To get to the above line, either lb.userBundle is null or
> getResourceBundle() isoverridden.  L2126 already checks that this logger
> does not associate with the system rbname.  It seems to me that the
> special case here is when the Logger subclass overrides
> getResourceBundleName() to returnSYSTEM_LOGGER_RB_NAME or override
> getResourceBundle to return the "sun.util.logging.resources.logging" RB.
> I tink that we might just simplify the above and just return
> LoggerBundle.get(rbName, b) and no need to worry about those overridden
> case which does no use to such a Logger subclass with unmatched rbname
> and RB.  Same comment applied to L2157-2165.

Ah. I'm glad to hear that. This is a great simplification.
done.

-- daniel


>
> thanks
> Mandy




More information about the core-libs-dev mailing list