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

[Deven You]

Hi Dmitry,

Besides David's suggestion, I don't see the @run main tag for the test, 
is it optional?

Thanks a lot!

On 05/06/2012 08:30 PM, Dmitry Samersoff wrote:
> 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
>>>>
>>>
>


-- 
Best Regards,

Deven

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20120507/bc5e4630/attachment.html