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