Rework protocol IO, add Buffer abstraction & fix socket write issues#1127
Merged
Rework protocol IO, add Buffer abstraction & fix socket write issues#1127
Conversation
PerchunPak
reviewed
Mar 7, 2026
cc85e88 to
33da8fc
Compare
ItsDrike
commented
Mar 26, 2026
b3be0f4 to
542c1d0
Compare
ItsDrike
commented
Mar 26, 2026
0a03398 to
c5b1661
Compare
The previous protocol implementation mixed multiple responsibilities in `Connection`. It acted both as a network transport and as an in-memory buffer for packet construction/parsing. This made the code confusing, hard to reason about, and resulted in duplicated serialization logic. This change restructures the protocol IO layer and cleans up the design: - Extract binary read/write primitives into `_protocol.io.base_io` (BaseSync/AsyncReader and BaseSync/AsyncWriter). - Introduce a dedicated `Buffer` type for in-memory packet construction and decoding instead of abusing connection objects as buffers. - Move socket implementations into `_protocol.io.connection`. - Update protocol clients to use the new IO abstractions and the `StructFormat` helpers for typed serialization. Besides the structural cleanup, this also fixes several important issues in the previous implementation: * The synchronous TCP connection used `socket.send(data)`, which does not guarantee that all bytes are sent. The new implementation correctly uses `socket.sendall()`. * The asynchronous TCP write method was synchronous. It was just calling `writer.write(data)` but never awaited `drain()`, meaning the actual network write could happen at an arbitrary later time. In practice this rarely broke because a read always followed the write, which implicitly allowed the event loop to flush the buffer, but the behavior was still incorrect and fragile. The new async connection now uses async write method and awaits `writer.drain()` to ensure proper write semantics.
This was not used anywhere anyways and it's better to force being explicit than to "magically" encode into utf-8.
136a1ce to
2102cc9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The previous protocol implementation mixed multiple responsibilities in
Connection. It acted both as a network transport and as an in-memory buffer for packet construction/parsing. This made the code confusing, hard to reason about, and resulted in duplicated serialization logic.This change restructures the protocol IO layer and cleans up the design:
_protocol.io.base_io(BaseSync/AsyncReader and BaseSync/AsyncWriter).Buffertype for in-memory packet construction and decoding instead of abusing connection objects as buffers._protocol.io.connection.StructFormathelpers for typed serialization.Besides the structural cleanup, this also fixes several important issues in the previous implementation:
The synchronous TCP connection used
socket.send(data), which does not guarantee that all bytes are sent. The new implementation correctly usessocket.sendall().The asynchronous TCP write method was synchronous. It was just calling
writer.write(data)but never awaiteddrain(), meaning the actual network write could happen at an arbitrary later time. In practice this rarely broke because a read always followed the write, which implicitly allowed the event loop to flush the buffer, but the behavior was still incorrect and fragile. The new async connection now uses async write method and awaitswriter.drain()to ensure proper write semantics.Inspiration
A lot of the new design takes direct inspiration from how mcproto handles protocol interactions.