RFR: JDK-8134373: explore potential uses of convenience factories within the JDK
Stuart Marks
stuart.marks at oracle.com
Fri Sep 30 02:13:23 UTC 2016
Hi Jonathan, all,
I've started to look at this changeset. I'm looking at the one Patrick Reinhart
posted a couple weeks ago (! sorry):
http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.01/
I looked at only a few files and I'm already starting to have questions. Not
because anybody did anything wrong, but because there are some subtle issues
that hadn't been raised.
1) ModuleFinder.java
static ModuleFinder compose(ModuleFinder... finders) {
- final List<ModuleFinder> finderList = Arrays.asList(finders);
- finderList.forEach(Objects::requireNonNull);
+ final List<ModuleFinder> finderList = List.of(finders);
return new ModuleFinder() { ...
It's important to note that Arrays.asList() wraps a list around an array,
whereas List.of() has copying semantics. Initially I thought that copying here
is unnecessary, but after further analysis I've come to believe it's necessary.
The compose() method is public in the ModuleFinder interface, and the list is
captured by the ModuleFinder anonymous inner class below, which is returned to
the caller. Therefore, if Arrays.asList() is used, the caller can mutate the
array argument and cause the ModuleFinder instance to misbehave. This is a
TOCTOU problem. Oddly, TOCTOU wasn't discussed with respect to this change,
though it was with another.
In any case switching to the copying semantics of List.of() in this case is
correct and prevents TOCTOU problems.
2) CookieManager.java
The old code created a HashMap and wrapped it in a Collections.unmodifiableMap()
and returned it to the caller. The new code returns some instance created by
Map.of(). This is a public API. While both versions conform to the specification
(it says an "immutable" Map is returned) certain behavioral differences are
visible. In particular, the different objects will serialize differently. It's a
Map<String, List<String>>, and it looks like the values are instances of
ArrayList, so the whole thing will be serializable. It's conceivable that
applications might take this thing and serialize it. This potentially exposes
applications to serial interoperability problems if they wanted to exchange
serial data containing this object between Java 8 and 9.
My hunch is that this problem is fairly unlikely to occur, but I think it would
be good idea to do some searching to see how often this API is used. If it's
used rarely, or the typical usages don't involve storing the result somewhere
(e.g., iterating over the values), then we can proceed.
I use grepcode.com for code searches. I'd be interested in hearing about
alternative code search engines as well.
3) FileTreeIterator.java
Here's the one where a comment was added describing the copying semantics of
List.of in order to prevent TOCTOU problems. The options array is passed from
the outside and thus can be mutated by the caller. But the only time it's used
is within FileTreeWalker, and there it's iterated over once and never used
again. This differs from case (1) above where (in the old code) the array
reference is captured by a long-lived object.
Anyway, in this case, I don't think the copying is necessary, so the old
Arrays.asList() code is probably fine.
4) Not a specific change, but I saw mention upthread of some change that caused
one of the java.time tests to fail, something about 'resolverFields' containing
null. Which change was this, and what's its status in the current webrev?
I note that Stephen Colebourne said that the changes seem "reasonable" but I
don't know if he saw the mention of the java.time test failure, or whether the
changeset he reviewed included a change that would have caused it.
* * *
I still need to look at the other changes. If we end up getting hung up on one
or two, it might make sense to break up the changeset and push part of it. It
might also make sense to split out the java.time changes, since they're a
logical chunk, and they potentially have different reviewers. But no need to do
anything on that just yet.
s'marks
On 9/15/16 12:36 PM, Jonathan Bluett-Duncan wrote:
> Hi all,
>
> Stuart, thank you very much for your thorough response.
>
> Regarding serializability concerns, I've just checked my changes and all
> non-test code in the JDK which calls it, and it doesn't seem to me that they
> affect any fields in `Serializable` classes or the return values of methods
> which either return instances of `Serializable` classes or whose javadoc
> mention that the returned object is serializable. So I'm somewhat certain that
> my changes are serialization-compatible, but only somewhat, because I'm not
> that familiar with the intricacies of serialization...
>
> Considering how busy Stuart is, would anyone else be happy to sponsor this change?
>
> Kind regards,
> Jonathan
>
> On 15 September 2016 at 18:20, Stuart Marks <stuart.marks at oracle.com
> <mailto:stuart.marks at oracle.com>> wrote:
>
> Hi all,
>
> Unfortunately I don't have time to work on any of this at the moment,
> because of JavaOne preparation, and JavaOne next week.
>
> Jonathan, thanks for pushing forward with this. I'm glad that others have
> picked it up.
>
> Patrick, thanks for posting the changeset on Jonathan's behalf. This is
> very helpful.
>
> A few comments regarding issues raised up-thread.
>
> Regarding the (non)singleton-ness of the empty collections, this is covered by
>
> https://bugs.openjdk.java.net/browse/JDK-8156079
> <https://bugs.openjdk.java.net/browse/JDK-8156079>
> consider making empty instances singletons
>
> It wasn't a design decision to make them not singletons. The spec
> requirement is only that the returned instance satisfy the requirements of
> the interfaces it implements (e.g., List) and nothing more. Certainly
> there is no spec requirement regarding object identity.
>
> Making the empty collections singletons is the "obvious" thing to do, but
> it's often the case that the "obvious" thing isn't the right thing. That
> said, it may still be the right thing to make them singletons. Given the
> proposed extension to the JDK 9 schedule, it might be possible to change
> this in JDK 9.
>
> Note that List.of() should be functionally equivalent to
> Collections.emptyList() -- and correspondingly for Set and Map -- but they
> do differ. In particular, they have different serialization formats.
>
> Also on this topic, please note comments that Daniel Fuchs and I have added to
>
> https://bugs.openjdk.java.net/browse/JDK-8134373
> <https://bugs.openjdk.java.net/browse/JDK-8134373>
>
> regarding serialization compatibility. Reviewers should take care that
> updating code to use these new collection factories doesn't change any
> serialization formats. Unfortunately I am not confident that we have
> sufficient tests for serialization compatibility.
>
> s'marks
>
>
>
> On 9/15/16 7:02 AM, Jonathan Bluett-Duncan wrote:
>
> Wow, lots of discussion went on since I was busy doing other stuff!
>
> Thanks Patrick for doing the work of creating a new webrev for me. Really
> appreciated!
>
> Pavel already mentioned it, but I think List.of instead of
> Collections.emptyList in ZoneOffsetTransition is the right thing to do for
> visual and behavioural consistency. If it turns out that we need to revert
> to Collections.empty* and Collections.unmodifiable* for e.g.
> serializability or class loading concerns, then I'd be happy to revert
> both
> of the lines I touched. Otherwise I believe that List.of should be used
> consistently.
>
> I think Stuart made List.of() non-singleton because there wasn't any
> evidence that it made List.of() more memory- or time-intensive than
> Collections.emptyList(), but I might be wrong on this. I'm sure he can
> explain more or correct me in this case.
>
> Kind regards,
> Jonathan
>
>
> On 15 September 2016 at 13:33, Patrick Reinhart <patrick at reini.net
> <mailto:patrick at reini.net>> wrote:
>
> Hello together,
>
> I tried to process all suggested change input into the following new
> webrev:
>
> http://cr.openjdk.java.net/~reinhapa/reviews/8134373/webrev.01
> <http://cr.openjdk.java.net/%7Ereinhapa/reviews/8134373/webrev.01>
>
> Give me feedback if something is missing/wrong
>
> -Patrick
>
>
> On 2016-09-15 13:48, Pavel Rappo wrote:
>
> Daniel, Claes,
>
> List.of() and Collections.emptyList() are not the same. The
> behaviours are
> different. Moreover, immutable static factory methods return
> instances
> which are
> value-based. I believe it also means we are not tied with
> unconditional
> instantiation, and in case of empty collections/maps could
> probably
> return the
> same object every time.
>
> We should ask Stuart why it has been done like that in the
> first place.
> Maybe
> out of concern people might synchronize of those objects? I
> don't know.
> Let's
> say for now it's an implementation-specific detail.
>
> On 15 Sep 2016, at 12:35, Claes Redestad
> <claes.redestad at oracle.com <mailto:claes.redestad at oracle.com>>
>
> wrote:
>
> +1
>
> I don't mind List.of() aesthetically, but there are places
> where
> startup/footprint is important where Collections.emptyList()
> is simply superior, e.g., constituting permanent data
> structures
> such as the module graph during early bootstrap.
>
>
>
More information about the core-libs-dev
mailing list