RR(S) 7164191: properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Dmitry Samersoff]

David,

Thank you for your response!

On 2012-05-06 16:18, David Holmes wrote:
> Hi Dmitry,
> 
> Sorry for late response but I've been travelling.
> 
> One concern with the test is that it isn't guaranteed to fail with the
> buggy code. Not sure there is anything we can do about that though.

It used to fall, so I guess it's good enough.

> One nit with the test is that it is using an indent of 8 instead of 4.

Will fix it.

-Dmitry

> 
> David
> 
> On 4/05/2012 10:31 PM, Dmitry Samersoff wrote:
>> Hi Everybody,
>>
>> Below is slightly modified version of the fix created by Deven You.
>>
>> http://cr.openjdk.java.net/~dsamersoff/7164191/webrev.00/
>>
>> -Dmitry
>>
>>
>> On 2012-04-26 05:21, Deven You wrote:
>>> Hi Dmitry,
>>>
>>> Thanks for your help. I have created a CR with internal id 2236492 which
>>> hasn't be published yet. So please set this internal CR id as duplicate
>>> to 716419 as well.
>>>
>>> This is the original mail for this problem:
>>>
>>>
>>>
>>> -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>>
>>>
>>> Hi core-libs-devs,
>>>
>>> I am not sure if sun.management.Agent belongs to jmx-dev mailing list,
>>> if so please anyone tell me.
>>>
>>> This issue is that the sun.management.Agent.loadManagementProperties()
>>> will invoke properties.putAll which will throw
>>> ConcurrentModifcationException if there are other threads which modify
>>> the properties concurrently.
>>>
>>> I have made a patch[1] which synchronize the sysProps so that putAll can
>>> work on multi-thread scenario. The test case is also available in [1].
>>>
>>> Thanks a lot!
>>>
>>> [1] http://cr.openjdk.java.net/~littlee/OJDK-256/webrev.00
>>> <http://cr.openjdk.java.net/%7Elittlee/OJDK-256/webrev.00>
>>>
>>> ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>>
>>>
>>>
>>> Thanks a lot!
>>>
>>>
>>>
>>> On 04/25/2012 08:32 PM, Dmitry Samersoff wrote:
>>>> Deven,
>>>>
>>>> CR number is  7164191 .
>>>>
>>>> Could you re-send me your original e-mail with problem description and
>>>> webrev link.
>>>>
>>>>    I'll put it to CR comment field.
>>>>
>>>>
>>>> -Dmitry
>>>>
>>>>
>>>>
>>>> On 2012-04-24 16:15, Dmitry Samersoff wrote:
>>>>> Deven,
>>>>>
>>>>> After close look and off-line discussion with David Holmes,
>>>>> the changes looks good for me.
>>>>>
>>>>> I'll take care of the rest.
>>>>>
>>>>> We have one more place in Agent.java executing exactly the same code
>>>>> so I'll change both of them on your behalf.
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2012-04-23 11:43, Deven You wrote:
>>>>>> Thanks David,
>>>>>>
>>>>>> So is it ok for you to contribute this patch?
>>>>>>
>>>>>> On 04/23/2012 02:36 PM, David Holmes wrote:
>>>>>>> Except of course that Properties is a Hashtable and synchronizes on
>>>>>>> 'this' for all public methods. So locking the properties object
>>>>>>> in the
>>>>>>> client code will guarantee exclusive access to it.
>>>>>>>
>>>>>>> Sorry about that.
>>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>> On 23/04/2012 4:30 PM, David Holmes wrote:
>>>>>>>> Deven,
>>>>>>>>
>>>>>>>> On 23/04/2012 3:54 PM, Deven You wrote:
>>>>>>>>> On 04/18/2012 02:20 PM, Deven You wrote:
>>>>>>>>>> On 04/18/2012 01:34 PM, Mandy Chung wrote:
>>>>>>>>>>>
>>>>>>>>>>> I think this could still run into CME. System Properties is
>>>>>>>>>>> not a
>>>>>>>>>>> synchronized map and the setter methods (System.setProperty or
>>>>>>>>>>> Properties.put method) doesn't synchronize on the Properties
>>>>>>>>>>> object.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> The setter methods I'm referring to are System.setProperty and
>>>>>>>>>>> System.getProperties().put().
>>>>>>>>>>>
>>>>>>>>>> I have gone through the Agent.java, I think other set/put methods
>>>>>>>>>> related to properties are protected properly.
>>>>>>>>>>
>>>>>>>>>> public static void agentmain using parseString(args) which
>>>>>>>>>> return a
>>>>>>>>>> properties which is a local var and is not possible to cause
>>>>>>>>>> concurrent problem when call config_props.putAll(arg_props).
>>>>>>>>>>
>>>>>>>>>> private static synchronized void startLocalManagementAgent() is
>>>>>>>>>> synchronized already.
>>>>>>>>>>
>>>>>>>>>> private static synchronized void
>>>>>>>>>> startRemoteManagementAgent(String
>>>>>>>>>> args) is synchronized also.
>>>>>>>>>>
>>>>>>>>>> Could you point where the CME may ocurr?
>>>>>>>>> Is there any suggestion from the mailing list?
>>>>>>>> The problem is that System.getProperties() returns a globally
>>>>>>>> accessible
>>>>>>>> set of properties. So even if you prevent the Agent code from
>>>>>>>> modifying
>>>>>>>> those properties concurrently with other use in the Agent, you
>>>>>>>> have no
>>>>>>>> such guard for any other piece of code in the system which might
>>>>>>>> also
>>>>>>>> modify the properties. So the race condition you were trying to fix
>>>>>>>> still exists. I don't see any way to fix this. No matter what
>>>>>>>> you do
>>>>>>>> another thread can modify the system properties while you are
>>>>>>>> iterating
>>>>>>>> them. Instead you need to anticipate the CME and try to recover
>>>>>>>> from it
>>>>>>>> (also non-trivial).
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> David Holmes
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>>


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...