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

[Staffan Larsen]

Looks good!

/Staffan

On 4 maj 2012, at 14:31, 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 ...