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.ma...@oracle.com
<mailto:stuart.ma...@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 <patr...@reini.net
<mailto:patr...@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.redes...@oracle.com <mailto:claes.redes...@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.
