Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hashcode changing between different executions (#2478) #2479

Merged
merged 1 commit into from
Aug 5, 2023
Merged

Fix hashcode changing between different executions (#2478) #2479

merged 1 commit into from
Aug 5, 2023

Conversation

Mr-EmPee
Copy link
Contributor

@Mr-EmPee Mr-EmPee commented Jul 13, 2023

Fixes the issue that was preventing the deserialization of PacketContainers

closes #2478

@Mr-EmPee
Copy link
Contributor Author

Another fix would be making the hashcode variable transient.
I prefer the suggested approach cause if anyone is using the hashcode function he can expect the value to remain the same

@derklaro
Copy link
Contributor

Well if you open a PR to fix a bug it woild be really nice if you could explain in depth why the issue occurred in the first place ;)

@dmulloy2
Copy link
Owner

i think the real issue here is that it's relying on the hashCode being stable, which is not a safe assumption

@Mr-EmPee
Copy link
Contributor Author

Well if you open a PR to fix a bug it woild be really nice if you could explain in depth why the issue occurred in the first place ;)

All the details are in the linked issue.

i think the real issue here is that it's relying on the hashCode being stable, which is not a safe assumption

I mean we could just put a transient on the cached variable, that would solve the problem without assuming that they maintain the same hashcode

@derklaro
Copy link
Contributor

The linked issue does not describe where the problem originates from. It only generically describes an issue with serialising/deserializing packet containers...

@Mr-EmPee
Copy link
Contributor Author

The linked issue does not describe where the problem originates from. It only generically describes an issue with serialising/deserializing packet containers...

It seems that when looking up the the clazz of a PacketType it fails to find it that's because the hashcode of the deserialized PacketType doesn't match with the hashCode of the packetType inside the registry.

This is expected behavior bc the specs doesn't guarantee the same hashcode for different executions

To add more context, you serialize a packetContainer and when trying to deserialize it protocol lib does what I've describeded above and it fails.

@lukalt
Copy link
Contributor

lukalt commented Jul 14, 2023

I think using hashCode() is unreliable when working with the Minecraft protocol in general. Most NMS packets do not properly implement equals() and hashCode()

@dmulloy2
Copy link
Owner

The linked issue does not describe where the problem originates from. It only generically describes an issue with serialising/deserializing packet containers...

It seems that when looking up the the clazz of a PacketType it fails to find it that's because the hashcode of the deserialized PacketType doesn't match with the hashCode of the packetType inside the registry.

This is expected behavior bc the specs doesn't guarantee the same hashcode for different executions

To add more context, you serialize a packetContainer and when trying to deserialize it protocol lib does what I've describeded above and it fails.

right, so in my mind a proper fix addresses the fact that serialization relies on hashCode instead of something stable

@Mr-EmPee
Copy link
Contributor Author

Mr-EmPee commented Jul 14, 2023

right, so in my mind a proper fix addresses the fact that serialization relies on hashCode instead of something stable

okay, I'm gonna try to add the modifier transient to the hashcode cache variable, that should fix the problem without
relaying on a specific custom behavior

--- edit
tested it and the problem is fixed that way

@dmulloy2 dmulloy2 enabled auto-merge (squash) August 5, 2023 19:04
@dmulloy2 dmulloy2 merged commit 9e9b39a into dmulloy2:master Aug 5, 2023
3 checks passed
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.

Unable to deserialize a PacketContainer
4 participants