RFR: 8129957 - Deadlock in JNDI LDAP implementation when closing the LDAP context

[Vincent Ryan]

Your proposed fix looks fine Rob.
Thanks.


> On 14 Sep 2015, at 16:25, Rob McKenna <rob.mckenna at oracle.com> wrote:
> 
> Hi folks,
> 
> So on further investigation it looks like we could get away with reducing the amount of locking in LdapClient. Here is a proposed fix followed by a description:
> 
> http://cr.openjdk.java.net/~robm/8129957/webrev.02/
> 
> processConnectionClosure():
> - Remove the synchronization from processConnectionClosure and handle it further down in notifyUnsolicited
> 
> removeUnsolicited():
> - Remove the synchronized block in removeUnsolicited as its redundant. Vectors are synchronized already.
> 
> processUnsolicited()
> - Remove the initial synchronized block in processUnsolicited and limit it to the area around the unsolicited size check / notice creation. (again, due to the notifyUnsolicited changes)
> - Remove the redundant unsolicited.size check from the end of processUnsolicited
> 
> notifyUnsolicited():
> - Synchronize on the unsolicited vector in order to create a copy of that vector and empty it if e is a NamingException.
> - Outside the notifyUnsolicited synchronize block, loop through the copy of unsolicited and call fireUnsolicited on each element.
> - The main potential problem with this fix would be if an LdapCtx became unsolicited before we got to it in the for loop. However since both LdapCtx.fireUnsolicited and LdapCtx.removeUnsolicited sync on eventSupport and LdapCtx.fireUnsolicited double checks to make sure it is still unsolicited, that should be fine.
> 
> 	-Rob
> 
> 
> On 10/08/15 14:06, Rob McKenna wrote:
>> Hi folks,
>> 
>> We have a hang between LdapClient / Ctx due to the fact that
>> Connection.cleanup() calls LdapClient.processConnectionClosure which
>> locks the unsolicited vector and in turn calls LdapCtx.fireUnsolicited
>> which locks the eventSupport object. Unfortunately when an
>> LdapCtx.close() occurs at the same time, its removeUnsolicited method
>> locks the eventSupport object first and then attempts to call
>> LdapClient.removeUnsolicited which attempts to lock the unsolicited vector.
>> 
>> So:
>> 
>> thread 1:
>> 
>> Connection.cleanup ->
>> LdapClient.processConnectionClosure (LOCK VECTOR) ->
>> LdapCtx.fireUnsolicited (LOCK EVENTSUPPORT)
>> 
>> (LdapClient is looping through LdapCtx objects in the unsolicited vector)
>> 
>> thread 2:
>> 
>> LdapCtx.close (LOCK LDAPCTX) ->
>> LdapCtx.removeUnsolicited (LOCK EVENTSUPPORT) ->
>> LdapClient.removeUnsolicited (LOCK VECTOR)
>> 
>> (A single LdapCtx removes itself from its LdapClient unsolicited list)
>> 
>> 
>> My proposed solution is to have both threads lock the LdapClient before
>> locking either the unsolicited vector or the eventSupport object.
>> 
>> Webrev at: http://cr.openjdk.java.net/~robm/8129957/webrev.01/
>> 
>>     -Rob