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 packet compression handling #159

Merged
merged 2 commits into from Aug 5, 2023
Merged

Fix packet compression handling #159

merged 2 commits into from Aug 5, 2023

Conversation

ItsDrike
Copy link
Member

@ItsDrike ItsDrike commented Aug 5, 2023

This fixes a bug in how we previously handled packet compression. Originally, our handling was done based on a boolean, either do enable compression, or don't.

However this doesn't really work, as the way we recognize whether to enable compression is through the server, sending us the LoginSetCompression packet, which only contains a compression threshold value. This value can be -1, in which case compression should be disabled, it can also be 0, in which case compression would indeed always be enabled, but for any other value, compression should only be enabled if the packet length surpasses this threshold, and we only know the packet length during writing, so our interaction functions need to be aware of it.

Note that if this threshold isn't crossed, but is set, the packet format still changes (there is an extra varint after packet length, which generally contains the uncompressed length, but when compression wasn't used - below threshold, it will be set to 0, and the packet id and data fields below remain uncompressed).

This PR changes sync_write_packet and async_write_packet functions and replaces compressed bool parameter to compression_threshold parameter, which is now being used properly. This however breaks compatibility, as these writer functions are a part of the public API, and the compressed parameter will no longer be available. Considering it didn't really even work properly before, it's unlikely that someone was actually using it yet, nevertheless it is a breaking change.

This does also affect how packet reading works, however there is no breaking change, as only the logic needed to change, to account for the possibility of the data length being 0, in which case compression wasn't used. This function still only takes a compressed bool flag parameter, rather than a compression_threshold, as if the threshold is non-negative, the packet format fundamentally changes, so we should already be reading the packets differently. For that reason, a bool flag is sufficient here, and should just be set to True whenever the threshold is >= 0.

Edit: Even though that makes logical sense, I've decided against making the reader use compressed bool flag, as it might be annoying for the user to have to do compressed=compresssion_threshold >= 0 every time when calling the function, as opposed to just compression_threshold=compression_threshold. So while the reader doesn't need to know this value, it also doesn't hurt to know it, and we just figure out whether or not compression is enabled ourselves, instead of bothering the user with it. Making this a breaking change too. The underlying _deserialize_packet function still only takes a compressed bool flag though.

@ItsDrike ItsDrike added t: bug Something isn't working p: 0 - critical This needs to be addressed ASAP z: breaking This introduces backwards compatibility breaking changes to the public API a: packets Related to packets (packet classes, or their reading/writing) a: API Related to exposed core API of the project labels Aug 5, 2023
This fixes a bug in how we previously handled packet compression.
Originally, our handling was done based on a boolean, either do enable
compression, or don't.

However this doesn't really work, as the way we recognize whether to
enable compression is through the server, sending us the
`LoginSetCompression` packet, which only contains a compression
threshold value. This value can be -1, in which case compression should
be disabled, it can also be 0, in which case compression would indeed
always be enabled, but for any other value, compression should only be
enabled if the packet length surpasses this threshold, and we only know
the packet length during writing, so our interaction functions need to
be aware of it.

Note that if this threshold isn't crossed, but is set, the packet format
still changes (there is an extra varint after packet length, which
generally contains the uncompressed length, but when compression wasn't
used - below threshold, it will be set to 0, and the packet id and data
fields below remain uncompressed).

This changes `sync_write_packet` and `async_write_packet` functions and
replaces compressed bool parameter to compression_threshold parameter,
which is now being used properly. This however breaks compatibility, as
these writer functions are a part of the public API, and the compressed
parameter will no longer be available. Considering it didn't really even
work properly before, it's unlikely that someone was actually using it
yet, nevertheless it is a breaking change.

This does also affect how packet reading works, however there is no
breaking change, as only the logic needed to change, to account for the
possibility of the data length being 0, in which case compression wasn't
used. This function still only takes a compressed bool flag parameter,
rather than a compression_threshold, as if the threshold is
non-negative, the packet format fundamentally changes, so we should
already be reading the packets differently. For that reason, a bool flag
is sufficient here, and should just be set to True whenever the
threshold is >= 0.
Even though the reader technicaly doesn't need to know the exact
threshold, it only needs to know whether it's non-negative (if
compression is used at all), since the writer fucntion takes the
threshold directly, this is nicer for user experience, as it would be
very annoying to do `compressed=compression_threshold >= 0` each time
manually.
@ItsDrike ItsDrike merged commit 6394022 into main Aug 5, 2023
10 of 12 checks passed
@ItsDrike ItsDrike deleted the fix-packet-compression branch August 5, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: API Related to exposed core API of the project a: packets Related to packets (packet classes, or their reading/writing) p: 0 - critical This needs to be addressed ASAP t: bug Something isn't working z: breaking This introduces backwards compatibility breaking changes to the public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant