On 04/08/2018 17:22, Claes Redestad wrote:
Hi,
Jiangli and I discussed a subtle issue with the archiving changes here
last week (she's currently on vacation, and I was at JVMLS this week,
so it was never brought to the list, sorry!)
Basically: the empty configuration uses things like
Collections.emptySet(), which means it will end up deserializing an
object from the archive that doesn't (necessarily) have the same
identity as the object retrieved from direct uses of
Collection.emptySet().
Same goes for the Set.of()/Map.of()/List.of() singletons which might
now already exist at runtime in two versions since the change to
archive ModuleDescriptors. Oops!
This glitch is probably fine semantically for most intents and
purposes, but could lead to some unnecessary inefficiencies and
theoretically real surprises/bugs, so we probably need to ensure that
all singletons we serialize retain the property that they are
identical to the object a caller of the respective getter would receive.
I think this can be deferred to a must-fix follow-up, mainly since
there's some benefit in first cleaning up a few things here first:
- currently we have uses of both Collections.emptyFoo() and Foo.of()
in the ModuleDescriptor/Configuration code
- to be correct, we thus need to archive the Collections.emptyFoo()
singletons *and* the various java.util.ImmutableCollections ones
- if we first consolidated all uses of Collections.emptyFoo() to
Foo.of() (in java.lang.module), we'd only need to archive the
immutable singletons in java.util.ImmutableCollections
It's good to bring this up. I don't think there is a real issue right
now but it is a lurking hazard. As I recall, we started out in
java.lang.module long before the immutable collections were added so
this is why it has been using Collections.emptyXXX(). There shouldn't be
any issues changing all these usages to XXX.of().
A discussion for core-libs-dev but I wonder if we could get to the point
where emptyXXX() and XXX.of() returns the same empty collection so that
we don't have any issues elsewhere.
-Alan
