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

[Seán Coffey]

Looks fine to me Ivan.

If you have a stacktrace of an example ConcurrentModificationException 
issue, please paste it into the bug report so that it's documented for 
others to find.

Regards,
Sean.

On 24/06/2016 15:56, Ivan Gerasimov wrote:
> 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
>>
>>
>