Re: RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

2017-12-11 Thread Peter Levart
Thanks Naoto, Alan, David and Venkat. The change is in. Regards, Peter Naoto Sato je 11. 12. 2017 ob 19:41 napisal: Hi Peter, Thanks for the tests. Looks good to me. One nit: it should throw an Exception instead of AssertionError when the test fails. No further review is needed. > Can

Re: RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

2017-12-11 Thread Naoto Sato
Hi Peter, Thanks for the tests. Looks good to me. One nit: it should throw an Exception instead of AssertionError when the test fails. No further review is needed. > Can this go into JDK 10 ? You can push it before the JDK 10 fork. Naoto On 12/9/17 2:33 PM, Peter Levart wrote: Hi Naoto,

Re: RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

2017-12-09 Thread Peter Levart
Hi Naoto, Thank you for reviewing. Naoto Sato je 06. 12. 2017 ob 20:41 napisal: Hi Peter, Venkat, Thank you for the fix. It looks good to me. Improved performance is a nice bonus! Would you be able to provide with a regression test? Sure, here it is:

Re: RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

2017-12-06 Thread Naoto Sato
Hi Peter, Venkat, Thank you for the fix. It looks good to me. Improved performance is a nice bonus! Would you be able to provide with a regression test? Naoto On 12/6/17 6:10 AM, Peter Levart wrote: Hi, On 12/06/2017 02:30 PM, Alan Bateman wrote: I think this class is normally maintained

Re: RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

2017-12-06 Thread Peter Levart
Hi, On 12/06/2017 02:30 PM, Alan Bateman wrote: I think this class is normally maintained on i18n-dev but I think introducing the Cache object looks good and making this much easier to understand. -Alan Thanks Alan, I'm forwarding to i18n-dev to see if maintainers of that part of JDK

Re: RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

2017-12-06 Thread Alan Bateman
On 06/12/2017 11:32, David Holmes wrote: Hi Peter, On 6/12/2017 9:08 PM, Peter Levart wrote: Hi David, Can I consider your comment as a Review? I'd like to get this patch into JDK10 if possible. No sorry. I see what you're doing and I think it is okay but the regular owners/maintainers of

Re: RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

2017-12-06 Thread David Holmes
Hi Peter, On 6/12/2017 9:08 PM, Peter Levart wrote: Hi David, Can I consider your comment as a Review? I'd like to get this patch into JDK10 if possible. No sorry. I see what you're doing and I think it is okay but the regular owners/maintainers of this code need to have the say on any

Re: RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

2017-12-06 Thread Peter Levart
Hi David, Can I consider your comment as a Review? I'd like to get this patch into JDK10 if possible. Regards, Peter On 11/28/2017 08:17 AM, David Holmes wrote: Hi Peter, I like what you have done here. That said the general thread-unsafeness of the code in SimpleTimeZone still causes me

Re: RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

2017-11-27 Thread David Holmes
Hi Peter, I like what you have done here. That said the general thread-unsafeness of the code in SimpleTimeZone still causes me concern - but what you are doing is not breaking anything more than it is already broken. David On 25/11/2017 9:32 AM, Peter Levart wrote: Hi, @Venkat: Sorry for

RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

2017-11-24 Thread Peter Levart
Hi, @Venkat: Sorry for late response, but I had to try something 1st. This is an official request for reviewing a patch for fixing a data race between cloning a SimpleTimeZone object and lazily initializing its 3 cache fields which may produce a clone with inconsistent cache state. Here's