Skip to content

Add default overrides for getFirst and getLast in MutableList and Mut…#1461

Merged
donraab merged 1 commit intoeclipse-collections:masterfrom
donraab:master
Jun 13, 2023
Merged

Add default overrides for getFirst and getLast in MutableList and Mut…#1461
donraab merged 1 commit intoeclipse-collections:masterfrom
donraab:master

Conversation

@donraab
Copy link
Contributor

@donraab donraab commented Jun 12, 2023

…ableSortedSet to fix JDK 21 EA build failure

+ "SGFzaE1hcAUH2sHDFmDRAwACRgAKbG9hZEZhY3RvckkACXRocmVzaG9sZHhwP0AAAAAAAAB3CAAA\n"
+ "ABAAAAAAeAA=",
UnmodifiableMutableOrderedMap.of(OrderedMaps.adapt(new LinkedHashMap<>())));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that there's no else block makes me wonder what happens in the new version. Is the serialized form changed? Does serialization now throw? Why?

Copy link
Contributor Author

@donraab donraab Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the serialized form has changed. Something has changed in regards to serialization for LinkedHashMap but I do not have an easy way of determining what changed. I wanted to fix the builds for now and decide amongst the committers how best to handle the else situation, or remove the jdk version test altogether. We are not responsible for guaranteeing LinkedHashMap serialization, so we may want to rethink this test to guarantee our own serialization, not the JDKs.

I reported the issue to the OpenJDK Quality Outreach Program.

Copy link
Contributor Author

@donraab donraab Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@motlin I'm going to add a temporary serialization test just for LinkedHashMap so we can see the build failure and confirm it is unrelated to our types.

Copy link
Contributor Author

@donraab donraab Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@motlin As I expected, the JDK 21-ea build fails on a new serialization test for an empty LinkedHashMap.

Error: 9:951 [ERROR] UnmodifiableMutableOrderedMapSerializationTest.serializedFormLinkedHashMap:43 Serialization was broken. expected:<...zaE1hcDTATlwQbMD7AgA[BWgALYWNjZXNzT3JkZXJ4
cgARamF2YS51dGlsLkhhc2hNYXAFB9rBwxZg0QMAAkYACmxvYWRGYWN0b3JJAAl0aHJlc2hvbGR4
cD9AAAAAAAAAdwgAAAAQAAAAAHgA]> but was:<...zaE1hcDTATlwQbMD7AgA[CWgALYWNjZXNzT3JkZXJJ
AAdwdXRNb2RleHIAEWphdmEudXRpbC5IYXNoTWFwBQfawcMWYNEDAAJGAApsb2FkRmFjdG9ySQAJ
dGhyZXNob2xkeHA/QAAAAAAAAHcIAAAAEAAAAAB4AAAAAAA=]>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! The only recent change to LinkedHashMap was to make it a SequencedCollection, and they did not change its serialVersionUID, so it's possible this is a bug upstream.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's possible this is a bug upstream.

I agree. Someone should report this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi all, JDK Sequenced Collections guy here. First, I hope I'm not causing you folks too much trouble. :-) @donraab brought this to my attention. Thanks Don!

Regarding this serialized form change, this was a mistake; there was no intent to change the serialized form. I've filed bug https://bugs.openjdk.org/browse/JDK-8309882 to restore to previous serialized form. It will likely be fixed in the next build of JDK 21.

That said, adding (and removing) fields from an object's serialized form is a compatible change; the behavior of "extra" and "missing" fields is well-defined in the serialization specification. We add or remove fields from time to time without changing the object's serialVersionUID. (Changing the serialVersionUID is incompatible and is almost impossible for clients to cope with.) That might mean this test is overly brittle, depending on what it's trying to do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for everything @stuart-marks! In the tests of the serialized forms of our wrapper classes, we're often wrapping JDK types. I agree with you that our tests are overly brittle. These tests have been quietly asserting that the serialized form doesn't change for empty ArrayLists, LinkedLists, HashSets, HashMaps, LinkedHashMaps, etc. for years. This is the first time I remember seeing a change to the serialized form, and I see that the addition of transient will bring back the old serialized form. I bet the serialized forms of these classes will never change. It might be helpful to add tests like these to the JDK! But if not, you can rely on us to ask about it when the form changes :)

While I have your attention, I have an unrelated question. Would it make sense to check for RandomAccess in List.getLast()?

    default E getLast() {
        if (this.isEmpty()) {
            throw new NoSuchElementException();
        } else if (this instanceof RandomAccess) {
            return this.get(this.size() - 1);
        } else {
            return this.reversed().iterator().next();
        }
    }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @motlin! Haven't talked in a while. Yeah, the serialized forms of collections haven't changed much if at all over time. Other classes like BigInteger have changed some though. And their internal representations have changed too, so there are several layers of compatibility code that handle mapping of different serialized forms to the current internal representation.

And yes, I'm surprised that nothing in the JDK tests caught this change. I know we've added checks to prevent some obvious mistakes (such as forgetting to add a serialVersionUID declaration) but apparently not accidental changes to serialized forms. I'll have to check into this.

On the getLast() default implementation... the JDK's LinkedList implementation -- which is the most common List implementation I'm aware of that doesn't implement RandomAccess -- will traverse from the end if the requested index is closer to the end. Thus access to size() - 1 will still be pretty much constant time. I suppose there might be some sequential list out there that doesn't know its size and that has to traverse from the front to find a particular index, but that would seem pretty unusual. I'm hard pressed to think of cases where doing a RandomAccess check will make a significant difference. If you're aware of any examples though, by all means let me know.

Copy link
Contributor

@motlin motlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have the energy for it, I think we uncovered one or two issues worth reporting upstream. Either way, I think this looks like a good way to get compiling with all versions of Java again.

…ableSortedSet to Fixes #1460.

Signed-off-by: Donald Raab <Donald.Raab@bnymellon.com>
@donraab donraab merged commit ab92319 into eclipse-collections:master Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants