On 9/30/16 6:57 AM, Jonathan Bluett-Duncan wrote:
1) It actually didn't occur to me that there was a potential TOCTOU problem in
ModuleFinder.compose, despite the fact that I found a potential problem in
FileTreeIterator - it completely missed me, so thank you for finding it! I'll
see if I can put a similar comment there to what I wrote in FileTreeIterator.
OK. A simple comment such as
// make a copy since it's captured by the ModuleFinder instance belowfinal
List<ModuleFinder> finderList = List.of(finders);
should be sufficient.
2) ... I decided just now to do a search on Grepcode for usages of
CookieManager.get
<http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@8u40-b25@java%24net@CookieManager@get%28java.net.URI%2Cjava.util.Map%29&k=u> and
CookieHandler.get
<http://grepcode.com/search/usages?id=repository.grepcode.com$java$root@jdk$openjdk@8u40-b25@java$net@CookieHandler@get%28java.net.URI,java.util.Map%29&start=0&r=&type=method&k=u>,
and curiously it looks like they're only used in sun classes in the JDK. So
this change seems safe to me, unless I've missed something?
While we're looking at CookieManager.java, please fix this typo
"NetscapeCookieSotre".
84 * by a more sophisticated CookieStore implementation, e.g. a
NetscapeCookieSotre.
So, grepcode is kind of hard to work with. I've found that when doing "Find
Usages" for an API, the results are relative to the JDK version you're looking
at. Apparently at the time grepcode's indexes were run, most of those other
libraries weren't building against any version of JDK 8. But if you start the
search at (say) JDK6 b27, and then go to java.net.CookieHandler.get() and do
"Find usages", you get a bunch of hits:
http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@6-b27@java%24net@CookieHandler@get%28java.net.URI%2Cjava.util.Map%29&k=u
Same with 6-b27 CookieManager.get():
http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@6-b27@java%24net@CookieManager@get%28java.net.URI%2Cjava.util.Map%29&k=u
7u40-b43 CookieHandler.get():
http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@7u40-b43@java%24net@CookieHandler@get%28java.net.URI%2Cjava.util.Map%29&k=u
7u40-b43 CookieManager.get():
http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@7u40-b43@java%24net@CookieManager@get%28java.net.URI%2Cjava.util.Map%29&k=u
I don't know a way to search for the usage of an API across all versions of the
JDK. It's moderately painful to have to do the same searches for several
different JDK versions.
Anyway, I spent some time looking through usage hits from the links above. It's
hard to know how many there are; you just keep hitting "Next" until there aren't
any more. (Is there a way to get statistics instead of just page after page of
matches? I don't know.) There are lots of duplicates -- different versions of
the same library -- so you can skip past those pretty quickly.
After a while some patterns begin to emerge. For this API, the Map that's
returned is almost always iterated over looking for "Cookie" and "Cookie2"
entries, or being probed directly for those entries, and is then thrown away. In
one case it was stored in a private field of a non-serializable class, but the
only usage of that field was to iterate over it in a manner similar to other cases.
I encourage you to look through these usages as well.
In any case, it's fairly clear to me that the most common use of the returned
Map is to read stuff out of it immediately. This isn't definitive, obviously,
but it does look like this is the most common usage pattern. Based on this I
think the serialization difference won't be a problem, and that it's OK to
proceed with this change.
3) In my local copy of jdk9, I've removed the TOCTOU comment in
FileTreeIterator and changed List.of back to Arrays.asList, as your
explanation regarding FileTreeWalker has convinced me that TOCTOU is not a
real problem here....
OK
4) The 'resolverFields' problem/comment was regarding DateTimeFormatter (see
this part of latest webrev
<http://cr.openjdk.java.net/%7Ereinhapa/reviews/8134373/webrev.01/src/java.base/share/classes/java/time/format/DateTimeFormatter.java.cdiff.html>),
where I realised I couldn't use Set.of instead of
Collections.unmodifiableSet(new HashSet<>(Arrays.asList(*))), because I
noticed that one of the java.time tests was testing whether
DateTimeFormatter.withResolverFields(TemporalField...) could accept null
parameters, which made using Set.of impossible since it's null-hostile (as in
it throws NullPointerException upon being constructed with null element(s)).
Hm, yes, it's odd that there's a test for an array containing a null, in
addition to an empty array and a null array. See:
jdk/test/java/time/tck/java/time/format/TCKDateTimeFormatter.java
test_resolverFields_listOfOneNull()
I'm not entirely sure what's intended here. In any case, let's wait until we
hear from Mr. Colebourne.
I looked at the rest of the files and the only questions I have are other places
in the java.time files.
5) For the changes in the Chronology classes, the era() method now returns an
immutable array where it didn't before. (The List returned by Arrays.asList()
can have individual elements modified but its size can't be changed.) The spec
for eras() says "may be immutable" so presumably this is OK. But note, since the
returned List is now immutable, a new instance needn't be created each time. I'm
not sure whether it's worth keeping around a cached copy in case someone calls
eras() again though.
It would be good if we could hear from Stephen on this one as well.
s'marks
