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.
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
Thanks for reviewing!
/Claes
