Hi, Daniel Thank you for reviewing this, inline
> On 18 Mar 2020, at 7:24 PM, Daniel Fuchs <daniel.fu...@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 >
