8029281: Synchronization issues in Logger and LogManager

[Mandy Chung]

On 12/3/2013 10:57 AM, Daniel Fuchs wrote:
> Hi Mandy,
>
> I have updated the webrev taking your comments into account.
>
> <http://cr.openjdk.java.net/~dfuchs/webrev_8029281/webrev.02/>
>

Looks much cleaner.  Thanks for the update.

Nit LogManager.java L1050: formatting - one extra space.  No need to 
generate a new webrev.

Mandy

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