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

[David Holmes]

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.

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

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
>>>>>
>>>>
>>>
>>
>>
>
>