[15] RFR: 8241130: com.sun.jndi.ldap.EventSupport.removeDeadNotifier: java.lang.NullPointerException

[Chris Yin]

Hi, Daniel

Thank you for reviewing this, inline

> On 18 Mar 2020, at 7:24 PM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
> 
> Hi Chris,
> 
> Thanks for taking care of this followup fix.
> 
> I see at least three other places where `notifiers` is accessed
> without a null check. It might be good to fix these three other
> places too just in case.
> (two addNamingListener methods and 1 removeNamingListener)
> All of them are already synchronized so a simple null check should
> be enough.

Yes, that's reasonable to handle the 3 places as you mentioned, just the situation seems more complex as below. I considered a lot, but no good idea yet, so finally decided to leave them untouched for now since it’s no harm to current bug, we could have some discussion if you have some idea or suggestions, then maybe create a follow up - follow up item to track it.

Current fix change is trying to handle the potential multithreading issue which caused by background thread that out of user’s control (EventSupport.removeDeadNotifier and EventSupport.queueEvent may been called and only been called by background thread such as ‘com.sun.jndi.ldap.NamingEventNotifier’)

For the 3 other places (two addNamingListener methods and 1 removeNamingListener), searching the usages, they could only been called from ‘com.sun.jndi.ldap.LdapCtx’ which is controlled by user, that means to hit NullPointerException, user need to close ldap context then try to add/remove namingListener with previous closed ldap context, that sounds like a user programing error. Per javadoc from EventContext and EventDirContext interface which implemented by LdapCtx that will call EventSupport, "NamingException should be thrown if a problem was encountered" when add/remove NamingListener, that requires us to construct a meaningful NamingException and get back to user to indicate problem encountered instead of 'simple null check’ + skip (like current fix), certainly, where is the suitable place to put the logic (maybe LdapCtx or EventSupport) still to be determined. Even worse, if some legacy user code already follow original NullPointerException way to handle special case, well, incredible.

> 
> Also you could had the @bug 8241130
> to RemoveNamingListenerTest.java, since the test can be used
> to verify the fix (if run often enough).

Sure, updated webrev as below, thanks

http://cr.openjdk.java.net/~xyin/8241130/webrev.01/

Regards,
Chris

> 
> best regards,
> 
> -- daniel
> 
> On 18/03/2020 08:16, Chris Yin wrote:
>> Hello
>> Please review following changes to fix corner issue in com.sun.jndi.ldap.EventSupport, thanks
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8241130
>> Webrev: http://cr.openjdk.java.net/~xyin/8241130/webrev.00/
>> This is a follow up item which related to 8202117. When testing the second revision fix for 8202117 (http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-March/065270.html), intermittent failure with java.lang.NullPointerException been observed(Thanks Daniel for additional testing), that exposed another issue in com.sun.jndi.ldap.EventSupport.
>> “com.sun.jndi.ldap.NamingEventNotifier" is a background thread which been created when addNamingListener with ldap context, per code logic, it will call EventSupport.removeDeadNotifier when encounter NamingException (maybe wrapped a SocketException) in some case, if user closed source ldap context first (EventSupport.cleanup been triggered), the call to EventSupport.removeDeadNotifier will throw NullPointerException since var “notifiers” already been set to null in previous EventSupport.cleanup. Maybe in real world, it’s hard to get all criteria been satisfied to reproduce, but it’s still a potential multithreading issue and caught by test with repeat run.
>> Combined this change and 8202117 second revision change, run test com/sun/jndi/ldap/RemoveNamingListenerTest.java on 4 platforms for total 2000 times, no failure been observed, also tier1, tier2, tier3 tests passed.
>> Thanks,
>> Chris
>