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):


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.


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