On 22 Dec 2015, at 17:40, Claes Redestad <[email protected]> wrote:
> Hi! > > On 2015-12-22 18:16, Chris Hegarty wrote: >> On 22 Dec 2015, at 17:14, Chris Hegarty <[email protected]> wrote: >> >>> On 22 Dec 2015, at 16:13, Claes Redestad <[email protected]> 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
