Re: RFR: 8180352: Add Stream.toList() method [v2]
On 11/18/20 3:55 AM, Florian Weimer wrote: I think it's also needed for an efficient null element check in List::copyOf. I have a hunch that with the posted implementation, it would incorrectly produce an immutable list that contains null elements. (Sorry for the delay; oddly, this comment didn't make it into the PR.) Yes, there was a bug in List::copyOf in that it would pass through a list containing nulls, a clear violation of its spec. I fixed this in my second-most recent commit on this PR. s'marks
Re: RFR: 8180352: Add Stream.toList() method [v2]
On Tue, 17 Nov 2020 20:04:58 GMT, Stuart Marks wrote: >>> @plevart wrote: >>> >>> > But the question is how does having a separate CollSer.IMM_LIST_NULLS >>> > type prevent that from happening? >>> >>> When a serialized list with IMM_LIST_NULLS is deserialized on an older JDK, >>> it'll throw InvalidObjectException since that tag isn't valid on older >>> JDKs. Obviously this is still an error, but it's a fail-fast approach that >>> avoids letting nulls leak into a data structure where they might cause a >>> problem some arbitrary time later. >>> >> >> Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both >> apps run on JDK16+, there will be no exception. >> >> What I'm trying to say is that the same problem of injecting unexpected >> nulls via serialization/deserialization can happen also if App V2 starts >> using ArrayList to construct the data structure and serialize it while App >> V1 deserializes it and expects non-null values only. App V1 would already >> have to guard against null values during deserialization in that case, >> because possibility of null values in deserialized data structure is nothing >> new for App V1. > > @plevart wrote: >> Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both >> apps run on JDK16+, there will be no exception. > > Sure, the IMM_LIST_NULLS tag only helps with serialization compatibility > across JDK releases. There are lots of ways an app can make incompatible > changes to the serialized forms of its objects that we have no way of > detecting. >> Sure, the IMM_LIST_NULLS tag only helps with serialization compatibility >> across JDK releases. > I would say it goes the other way - it worsens the serialization compatibility. OK, I was imprecise. The IMM_LIST_NULLS tag has an effect only on serialization across JDK releases, not changes to the application's serialization format using the same JDK release, or even on many changes to the app's serialization format across JDK releases. By "helps with serialization compatibility" I meant that this new serialized form helps the general issue of serialization compatibility (really, incompatibility) by failing fast in certain cases, instead of possibly allowing polluted data to leak into the receiving application and causing some arbitrary exception later during the run. But as you noted last, this is a different kind of object, and it has different behavior, so it needs a different encoding in the serialized form. I'll update this PR shortly with changes to fix null handling and other issues. - PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v2]
- Mail original - > De: "Florian Weimer" > À: "Peter Levart" > Cc: "Stuart Marks" , "core-libs-dev" > > Envoyé: Mercredi 18 Novembre 2020 12:55:02 > Objet: Re: RFR: 8180352: Add Stream.toList() method [v2] > * Peter Levart: > >> But I see that the new IMM_LIST_NULLS type is needed for one other >> thing - the immutable list implementation of that type has different >> behavior of indexOf and lastIndexOf methods (it doesn't throw NPE when >> null is passed to those methods) so this behavior has to be preserved in >> the deserialized instance and there is not way to achieve that on old >> JDK with existing serialization format, so there has to be an >> incompatible change in the serialization format for that matter. > > I think it's also needed for an efficient null element check in > List::copyOf. I have a hunch that with the posted implementation, it > would incorrectly produce an immutable list that contains null > elements. yes ! Rémi
Re: RFR: 8180352: Add Stream.toList() method [v2]
* Peter Levart: > But I see that the new IMM_LIST_NULLS type is needed for one other > thing - the immutable list implementation of that type has different > behavior of indexOf and lastIndexOf methods (it doesn't throw NPE when > null is passed to those methods) so this behavior has to be preserved in > the deserialized instance and there is not way to achieve that on old > JDK with existing serialization format, so there has to be an > incompatible change in the serialization format for that matter. I think it's also needed for an efficient null element check in List::copyOf. I have a hunch that with the posted implementation, it would incorrectly produce an immutable list that contains null elements.
Re: RFR: 8180352: Add Stream.toList() method [v2]
On 11/17/20 9:08 PM, Stuart Marks wrote: @plevart wrote: Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both apps run on JDK16+, there will be no exception. Sure, the IMM_LIST_NULLS tag only helps with serialization compatibility across JDK releases. I would say it goes the other way - it worsens the serialization compatibility. You are introducing a serialization format that is incompatible with older JDK releases. OTOH the problem of introducing nulls into List(s) in serializable data structures already exists because serializable List implementations that allow nulls already exist, so by introducing another one that is able to be deserialized on older JDK would not worsen the situation. There are lots of ways an app can make incompatible changes to the serialized forms of its objects that we have no way of detecting. So why bother with making the result of serialized stream.toList() not de-serializable on older JDK(s)? By making a favor to a hypothetical app that runs on older JDK and does not expect nulls in Lists of a data structure, you are also preventing an app that allows nulls in similar data structure from deserializing the datastructure on older JDK when the serialized form was produced with serializing stream.toList() on newer JDK. But I see that the new IMM_LIST_NULLS type is needed for one other thing - the immutable list implementation of that type has different behavior of indexOf and lastIndexOf methods (it doesn't throw NPE when null is passed to those methods) so this behavior has to be preserved in the deserialized instance and there is not way to achieve that on old JDK with existing serialization format, so there has to be an incompatible change in the serialization format for that matter. Peter
Re: RFR: 8180352: Add Stream.toList() method [v2]
On Tue, 10 Nov 2020 09:34:56 GMT, Peter Levart wrote:
>> I can see that having a separate IMM_LIST_NULLS type might be necessary to
>> preserve the allows-null/disallows-null behaviour of indexOf and lastIndexOf
>> methods...
>>
>> NOTE ALSO that ListN.equals(o) and ListN.hashCode() are inherited from
>> AbstractImmutableList:
>>
>> @Override
>> public boolean equals(Object o) {
>> if (o == this) {
>> return true;
>> }
>>
>> if (!(o instanceof List)) {
>> return false;
>> }
>>
>> Iterator oit = ((List) o).iterator();
>> for (int i = 0, s = size(); i < s; i++) {
>> if (!oit.hasNext() || !get(i).equals(oit.next())) {
>> return false;
>> }
>> }
>> return !oit.hasNext();
>> }
>> and
>> public int hashCode() {
>> int hash = 1;
>> for (int i = 0, s = size(); i < s; i++) {
>> hash = 31 * hash + get(i).hashCode();
>> }
>> return hash;
>> }
>>
>> ...which means they will throw NPE when the list contains null. The same
>> goes for SubList.
>
>> @plevart wrote:
>>
>> > But the question is how does having a separate CollSer.IMM_LIST_NULLS type
>> > prevent that from happening?
>>
>> When a serialized list with IMM_LIST_NULLS is deserialized on an older JDK,
>> it'll throw InvalidObjectException since that tag isn't valid on older JDKs.
>> Obviously this is still an error, but it's a fail-fast approach that avoids
>> letting nulls leak into a data structure where they might cause a problem
>> some arbitrary time later.
>>
>
> Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both
> apps run on JDK16+, there will be no exception.
>
> What I'm trying to say is that the same problem of injecting unexpected nulls
> via serialization/deserialization can happen also if App V2 starts using
> ArrayList to construct the data structure and serialize it while App V1
> deserializes it and expects non-null values only. App V1 would already have
> to guard against null values during deserialization in that case, because
> possibility of null values in deserialized data structure is nothing new for
> App V1.
@plevart wrote:
> Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both
> apps run on JDK16+, there will be no exception.
Sure, the IMM_LIST_NULLS tag only helps with serialization compatibility across
JDK releases. There are lots of ways an app can make incompatible changes to
the serialized forms of its objects that we have no way of detecting.
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v2]
On Sun, 8 Nov 2020 15:55:58 GMT, Peter Levart wrote:
>> Hi Stuart,
>>
>> I would like to discuss the serialization. You introduce new
>> CollSer.IMM_LIST_NULLS type of immutable collection. This means that if this
>> change goes into JDK16 for example, JDK15 and before will not be able to
>> deserialize such list as they know nothing about IMM_LIST_NULLS even if such
>> lists don't contain nulls. The reason you say to chose new type of
>> serialization format is the following:
>>
>>> "Suppose I had an application that created a data structure that used lists
>>> from List.of(), and I had a global assertion over that structure that it
>>> contained no nulls. Further suppose that I serialized and deserizalized
>>> this structure. I'd want that assertion to be preserved after
>>> deserialization. If another app (or a future version of this app) created
>>> the structure using Stream.to
>>> List(), this would allow nulls to leak into that structure and violate
>>> that assertion. Therefore, the result of Stream.toList() should not be
>>> serialization-compatible with List.of() et. al. That's why there's the new
>>> IMM_LIST_NULLS tag in the serial format"
>>
>> I don't quite get this reasoning. Let's try to decompose the reasoning
>> giving an example. Suppose we had the following data structure:
>>
>> public class Names implements Serializable {
>> private final List names;
>> Names(List names) {
>> this.names = names;
>> }
>> public List names() { return names; }
>> }
>>
>> App v1 creates such structures using new Names(List.of(...)) and
>> serializes/deserializes them. They keep the invariant that no nulls are
>> present. Now comes App v2 that starts using new Names(stream.toList()) which
>> allows nulls to be present. When such Names instance from app v2 is
>> serialized and then deserialized in app v1, nulls "leak" into data structure
>> of app v1 that does not expect them.
>>
>> But the question is how does having a separate CollSer.IMM_LIST_NULLS type
>> prevent that from happening?
>
> I can see that having a separate IMM_LIST_NULLS type might be necessary to
> preserve the allows-null/disallows-null behaviour of indexOf and lastIndexOf
> methods...
>
> NOTE ALSO that ListN.equals(o) and ListN.hashCode() are inherited from
> AbstractImmutableList:
>
> @Override
> public boolean equals(Object o) {
> if (o == this) {
> return true;
> }
>
> if (!(o instanceof List)) {
> return false;
> }
>
> Iterator oit = ((List) o).iterator();
> for (int i = 0, s = size(); i < s; i++) {
> if (!oit.hasNext() || !get(i).equals(oit.next())) {
> return false;
> }
> }
> return !oit.hasNext();
> }
> and
> public int hashCode() {
> int hash = 1;
> for (int i = 0, s = size(); i < s; i++) {
> hash = 31 * hash + get(i).hashCode();
> }
> return hash;
> }
>
> ...which means they will throw NPE when the list contains null. The same goes
> for SubList.
> @plevart wrote:
>
> > But the question is how does having a separate CollSer.IMM_LIST_NULLS type
> > prevent that from happening?
>
> When a serialized list with IMM_LIST_NULLS is deserialized on an older JDK,
> it'll throw InvalidObjectException since that tag isn't valid on older JDKs.
> Obviously this is still an error, but it's a fail-fast approach that avoids
> letting nulls leak into a data structure where they might cause a problem
> some arbitrary time later.
>
Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both
apps run on JDK16+, there will be no exception.
What I'm trying to say is that the same problem of injecting unexpected nulls
via serialization/deserialization can happen also if App V2 starts using
ArrayList to construct the data structure and serialize it while App V1
deserializes it and expects non-null values only. App V1 would already have to
guard against null values during deserialization in that case, because
possibility of null values in deserialized data structure is nothing new for
App V1.
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v2]
On Sun, 8 Nov 2020 15:55:58 GMT, Peter Levart wrote:
>> Hi Stuart,
>>
>> I would like to discuss the serialization. You introduce new
>> CollSer.IMM_LIST_NULLS type of immutable collection. This means that if this
>> change goes into JDK16 for example, JDK15 and before will not be able to
>> deserialize such list as they know nothing about IMM_LIST_NULLS even if such
>> lists don't contain nulls. The reason you say to chose new type of
>> serialization format is the following:
>>
>>> "Suppose I had an application that created a data structure that used lists
>>> from List.of(), and I had a global assertion over that structure that it
>>> contained no nulls. Further suppose that I serialized and deserizalized
>>> this structure. I'd want that assertion to be preserved after
>>> deserialization. If another app (or a future version of this app) created
>>> the structure using Stream.to
>>> List(), this would allow nulls to leak into that structure and violate
>>> that assertion. Therefore, the result of Stream.toList() should not be
>>> serialization-compatible with List.of() et. al. That's why there's the new
>>> IMM_LIST_NULLS tag in the serial format"
>>
>> I don't quite get this reasoning. Let's try to decompose the reasoning
>> giving an example. Suppose we had the following data structure:
>>
>> public class Names implements Serializable {
>> private final List names;
>> Names(List names) {
>> this.names = names;
>> }
>> public List names() { return names; }
>> }
>>
>> App v1 creates such structures using new Names(List.of(...)) and
>> serializes/deserializes them. They keep the invariant that no nulls are
>> present. Now comes App v2 that starts using new Names(stream.toList()) which
>> allows nulls to be present. When such Names instance from app v2 is
>> serialized and then deserialized in app v1, nulls "leak" into data structure
>> of app v1 that does not expect them.
>>
>> But the question is how does having a separate CollSer.IMM_LIST_NULLS type
>> prevent that from happening?
>
> I can see that having a separate IMM_LIST_NULLS type might be necessary to
> preserve the allows-null/disallows-null behaviour of indexOf and lastIndexOf
> methods...
>
> NOTE ALSO that ListN.equals(o) and ListN.hashCode() are inherited from
> AbstractImmutableList:
>
> @Override
> public boolean equals(Object o) {
> if (o == this) {
> return true;
> }
>
> if (!(o instanceof List)) {
> return false;
> }
>
> Iterator oit = ((List) o).iterator();
> for (int i = 0, s = size(); i < s; i++) {
> if (!oit.hasNext() || !get(i).equals(oit.next())) {
> return false;
> }
> }
> return !oit.hasNext();
> }
> and
> public int hashCode() {
> int hash = 1;
> for (int i = 0, s = size(); i < s; i++) {
> hash = 31 * hash + get(i).hashCode();
> }
> return hash;
> }
>
> ...which means they will throw NPE when the list contains null. The same goes
> for SubList.
@plevart wrote:
> But the question is how does having a separate CollSer.IMM_LIST_NULLS type
> prevent that from happening?
When a serialized list with IMM_LIST_NULLS is deserialized on an older JDK,
it'll throw InvalidObjectException since that tag isn't valid on older JDKs.
Obviously this is still an error, but it's a fail-fast approach that avoids
letting nulls leak into a data structure where they might cause a problem some
arbitrary time later.
> NOTE ALSO that ListN.equals(o) and ListN.hashCode() are inherited from
> AbstractImmutableList [...] which means they will throw NPE when the list
> contains null. The same goes for SubList.
Good catch! Yes, this is a problem. I'll do some rearranging here and add more
test cases. Thanks for spotting this.
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v2]
On Sun, 8 Nov 2020 10:47:08 GMT, Peter Levart wrote:
>>> Simon Roberts wrote:
>>
>>> This discussion of unmodifiable lists brings me back to the thought that
>>> there would be good client-side reasons for inserting an UnmodifiableList
>>> interface as a parent of LIst, not least because all our unmodifiable
>>> variants from the Collections.unmodifiableList proxy onward fail the Liskov
>>> substitution test for actually "being contract-fulfilling Lists".
>>
>> At some point there probably will need to be a long article explaining all
>> the issues here, but at the moment the best writeup I have is this one:
>>
>> https://stackoverflow.com/a/57926310/1441122
>>
>> TL;DR there are a few different ways to approach retrofitting something like
>> this, but they all have enough compromises that the net benefits are unclear.
>
> Hi Stuart,
>
> I would like to discuss the serialization. You introduce new
> CollSer.IMM_LIST_NULLS type of immutable collection. This means that if this
> change goes into JDK16 for example, JDK15 and before will not be able to
> deserialize such list as they know nothing about IMM_LIST_NULLS even if such
> lists don't contain nulls. The reason you say to chose new type of
> serialization format is the following:
>
>> "Suppose I had an application that created a data structure that used lists
>> from List.of(), and I had a global assertion over that structure that it
>> contained no nulls. Further suppose that I serialized and deserizalized this
>> structure. I'd want that assertion to be preserved after deserialization. If
>> another app (or a future version of this app) created the structure using
>> Stream.to
>> List(), this would allow nulls to leak into that structure and violate that
>> assertion. Therefore, the result of Stream.toList() should not be
>> serialization-compatible with List.of() et. al. That's why there's the new
>> IMM_LIST_NULLS tag in the serial format"
>
> I don't quite get this reasoning. Let's try to decompose the reasoning giving
> an example. Suppose we had the following data structure:
>
> public class Names implements Serializable {
> private final List names;
> Names(List names) {
> this.names = names;
> }
> public List names() { return names; }
> }
>
> App v1 creates such structures using new Names(List.of(...)) and
> serializes/deserializes them. They keep the invariant that no nulls are
> present. Now comes App v2 that starts using new Names(stream.toList()) which
> allows nulls to be present. When such Names instance from app v2 is
> serialized and then deserialized in app v1, nulls "leak" into data structure
> of app v1 that does not expect them.
>
> But the question is how does having a separate CollSer.IMM_LIST_NULLS type
> prevent that from happening?
I can see that having a separate IMM_LIST_NULLS type might be necessary to
preserve the allows-null/disallows-null behaviour of indexOf and lastIndexOf
methods...
NOTE ALSO that while ListN.equals(o) method is using Objects.equals(o1, o2) to
compare elements, hashCode is inherited from AbstractImmutableList:
public int hashCode() {
int hash = 1;
for (int i = 0, s = size(); i < s; i++) {
hash = 31 * hash + get(i).hashCode();
}
return hash;
}
...which means it will throw NPE when the list contains null. The same goes for
SubList.
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v2]
On Fri, 6 Nov 2020 03:05:45 GMT, Stuart Marks wrote:
>> @ePaul wrote:
>>
>>> The Stream API works just as well with [third party] collection libraries
>>> instead of the java.util ones, just using different collectors. Adding a
>>> method which directly returns a java.util.List somewhat couples it to the
>>> Java Collection API.
>>>
>>> Now this was mentioned two and a half year ago. Did something change which
>>> made this consideration irrelevant? I would expect at least some mention of
>>> it in the discussion here.
>>
>> The separation between streams and the java.util Collections Framework is a
>> good design principle, but it isn't an ironclad rule. It's still easy to
>> have streams create instances of other collections libraries using the
>> Collector interface. What's different here is that the Collections Framework
>> has "leaked into" streams a little bit more, so that they're now more
>> interdependent. This doesn't seem to have any disadvantages; it seems
>> unlikely that the Collections Framework will ever be unplugged from the JDK.
>> However, the benefits are that a List is the closest thing we have to an
>> unmodifiable array that also plays well with generics and that can be
>> value-based; these benefits are considerable.
>
>> Simon Roberts wrote:
>
>> This discussion of unmodifiable lists brings me back to the thought that
>> there would be good client-side reasons for inserting an UnmodifiableList
>> interface as a parent of LIst, not least because all our unmodifiable
>> variants from the Collections.unmodifiableList proxy onward fail the Liskov
>> substitution test for actually "being contract-fulfilling Lists".
>
> At some point there probably will need to be a long article explaining all
> the issues here, but at the moment the best writeup I have is this one:
>
> https://stackoverflow.com/a/57926310/1441122
>
> TL;DR there are a few different ways to approach retrofitting something like
> this, but they all have enough compromises that the net benefits are unclear.
Hi Stuart,
I would like to discuss the serialization. You introduce new
CollSer.IMM_LIST_NULLS type of immutable collection. This means that if this
change goes into JDK16 for example, JDK15 and before will not be able to
deserialize such list as they know nothing about IMM_LIST_NULLS even if such
lists don't contain nulls. The reason you say to chose new type of
serialization format is the following:
> "Suppose I had an application that created a data structure that used lists
> from List.of(), and I had a global assertion over that structure that it
> contained no nulls. Further suppose that I serialized and deserizalized this
> structure. I'd want that assertion to be preserved after deserialization. If
> another app (or a future version of this app) created the structure using
> Stream.to
> List(), this would allow nulls to leak into that structure and violate that
> assertion. Therefore, the result of Stream.toList() should not be
> serialization-compatible with List.of() et. al. That's why there's the new
> IMM_LIST_NULLS tag in the serial format"
I don't quite get this reasoning. Let's try to decompose the reasoning giving
an example. Suppose we had the following data structure:
public class Names implements Serializable {
private final List names;
Names(List names) {
this.names = names;
}
public List names() { return names; }
}
App v1 creates such structures using new Names(List.of(...)) and
serializes/deserializes them. They keep the invariant that no nulls are
present. Now comes App v2 that starts using new Names(stream.toList()) which
allows nulls to be present. When such Names instance from app v2 is serialized
and then deserialized in app v1, nulls "leak" into data structure of app v1
that does not expect them.
But the question is how does having a separate CollSer.IMM_LIST_NULLS type
prevent that from happening?
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v2]
> This change introduces a new terminal operation on Stream. This looks like a > convenience method for Stream.collect(Collectors.toList()) or > Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this > method directly on Stream enables it to do what can't easily by done by a > Collector. In particular, it allows the stream to deposit results directly > into a destination array (even in parallel) and have this array be wrapped in > an unmodifiable List without copying. > > In the past we've kept most things from the Collections Framework as > implementations of Collector, not directly on Stream, whereas only > fundamental things (like toArray) appear directly on Stream. This is true of > most Collections, but it does seem that List is special. It can be a thin > wrapper around an array; it can handle generics better than arrays; and > unlike an array, it can be made unmodifiable (shallowly immutable); and it > can be value-based. See John Rose's comments in the bug report: > > https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065 > > This operation is null-tolerant, which matches the rest of Streams. This > isn't specified, though; a general statement about null handling in Streams > is probably warranted at some point. > > Finally, this method is indeed quite convenient (if the caller can deal with > what this operation returns), as collecting into a List is the most common > stream terminal operation. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Merge ListNNullsAllowed into ListN. Update spec for Stream.toList. Add nulls-allowed empty list. Simplify indexOf/lastIndexOf. Reduce tests, add contains(null) tests. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1026/files - new: https://git.openjdk.java.net/jdk/pull/1026/files/3e05564d..cf849755 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1026=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1026=00-01 Stats: 181 lines in 3 files changed: 16 ins; 117 del; 48 mod Patch: https://git.openjdk.java.net/jdk/pull/1026.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1026/head:pull/1026 PR: https://git.openjdk.java.net/jdk/pull/1026
