[jdk9] RFR: 8159822: Non-synchronized access to shared members of com.sun.jndi.ldap.pool.Pool

[Ivan Gerasimov]

Hi Aleksey!

I've double-checked the Pool class and found no other code that has to 
be additionally synchronized.
Please note that the method expungeStaleConnections() is thread-safe and 
can be called from outside the synchronized blocks.
The method Pool.getConnections(Object) accesses the `map` field, but it 
is only called from synchronized blocks, so it's fine.

Please let me know, if I'm missing something obvious.

With kind regards,
Ivan


On 21.06.2016 19:25, Ivan Gerasimov wrote:
>
>
> On 21.06.2016 18:43, Aleksey Shipilev wrote:
>> On 06/21/2016 06:14 PM, Ivan Gerasimov wrote:
>>> Hello!
>>>
>>> The Pool has a member `map`, which is accessed from different threads,
>>> thus the access is synchronized.
>>> However, in some code paths (mostly in debug printing) it is accessed
>>> without proper synchronization, which results in intermediate
>>> ConcurrentModificationException when the debug output is turned on.
>>>
>>> Would you please help review the fix?
>>>
>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8159822
>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8159822/00/webrev/
>> Missed synchronized-s:
>>
>>   154     private Connections getConnections(Object id) {
>>   155         ConnectionsRef ref = map.get(id);
>>   156         return (ref != null) ? ref.getConnections() : null;
>>   157     }
>>
>> ...gets called via:
>>
>>   168     public void expire(long threshold) {
>>   169         synchronized (map) {
>>        ...
>>   179         }
>>   180         expungeStaleConnections();
>>   181     }
>>
>> ...and also via:
>>
>>   188     private static void expungeStaleConnections() {
>> ...
>>   192             Connections conns = releaseRef.getConnections();
>> ...
>>   203          }
>>   204     }
> But this is ConnectionsWeakRef.getConnections() not 
> Pool.getConnections(Object).
> As far as I can see, an instance of ConnectionsWeakRef cannot be 
> accessed concurrently.
>
>>
>> ...
>>
>>   117     public PooledConnection getPooledConnection(Object id, long
>> timeout,
>>   118         PooledConnectionFactory factory) throws NamingException {
>> ...
>>              // no synchronized prior here
>>   127         expungeStaleConnections();
>> ...
>>
> expungeStaleConnections() should be safe to call concurrently as long 
> as ReferenceQueue.poll() is thread-safe.
>
> With kind regards,
> Ivan
>
>