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

[Ivan Gerasimov]

Thanks Seán!

I added an example of stacktrace to the description.

With kind regards,
Ivan


On 30.06.2016 17:31, Seán Coffey wrote:
> 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
>>>
>>>
>>
>
>