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