RFR: 8145862: Improve lazy initialization of fields in java.net.URI

[Chris Hegarty]

On 22 Dec 2015, at 17:40, Claes Redestad <claes.redestad at oracle.com> wrote:

> Hi!
> 
> On 2015-12-22 18:16, Chris Hegarty wrote:
>> On 22 Dec 2015, at 17:14, Chris Hegarty <chris.hegarty at oracle.com> wrote:
>> 
>>> On 22 Dec 2015, at 16:13, Claes Redestad <claes.redestad at oracle.com> wrote:
>>> 
>>>> Hi,
>>>> 
>>>> please review this patch to clean up and remove volatile from a number of lazily initialized fields in java.net.URI.
>>> So ‘string’ is the only remaining volatile field. Is there any specific reason for this?
> 
> writeObject must first ensure the string has been defined, then have it re-read by the ObjectOutputStream, breaking the safe pattern of always operating on the newly generated, local value.
> 
> I guess we might be able to turn to more explicit serialization using persistentSerializationFields so that we can deal with that benign race properly as well, but I feel that's out of scope for this improvement.

Agreed, and thanks for the explanation.

>>> 
>>>> This is safe as long as the fields are always accessed through their accessors and the accessors return the local result of the calculation.
>>> Ok, I see the pattern that you are using.
>>> 
>>>> Since initialization was done with no synchronization, there was never any guarantee that different Strings couldn't escape from concurrent calls to these getters. So even if this becomes more likely by dropping volatile, this should be of no real consequence.
>>> Right. This all seems a little fragile, but no worse than before your changes,
>>> and I get why some of these fields are “lazy”.
>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8145862
>>>> Webrev: http://cr.openjdk.java.net/~redestad/8145862/webrev.01
>>> I’m ok with these changes.
>>> 
>>> I notice that the public javadoc has "Instances of this class are immutable”. I wonder
>>> if the fields, that are no longer volatile, deserve a comment explaining that their
>>> initialization is racy?
>> Sorry, I mean: “… is safe in the face of multiple threads racing to initialize them”.
> 
> How about:
> 
>    // The remaining fields may be computed on demand, which is safe even in
>    // the face of multiple threads racing to initialize them

Perfect. Thanks,

-Chris.

> Thanks for reviewing!
> 
> /Claes