Skip to content

Length prefixed streams#1232

Merged
alexcos20 merged 30 commits intomainfrom
debug-longer-timeout
Mar 16, 2026
Merged

Length prefixed streams#1232
alexcos20 merged 30 commits intomainfrom
debug-longer-timeout

Conversation

@giurgiur99
Copy link
Contributor

@giurgiur99 giurgiur99 commented Feb 26, 2026

BREAKING CHANGE!

Changes proposed in this PR: #1231

  • Length-prefixed (lp) sends each message as length + payload. The receiver reads the length, then exactly that many bytes, so it always knows where one message ends and the next begins.
  • Allow large files download retrial with offset

@giurgiur99 giurgiur99 changed the title timeout Length prefixed streams Feb 28, 2026
@giurgiur99 giurgiur99 marked this pull request as ready for review March 11, 2026 14:28
@bogdanfazakas
Copy link
Member

/run-security-scan

Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This pull request introduces significant improvements to the P2P communication layer by refactoring stream handling to utilize LengthPrefixedStream from @libp2p/utils. This change standardizes how commands and data streams are sent and received, enhancing robustness, error handling, and timeout management. Key changes include explicit read/write timeouts (30s) for stream operations, an improved error response mechanism with optional debug information, and a client-side retry mechanism for transient 'stale connection' errors. Additionally, a minor but important fix is included in the HTTP command routes to prevent 'headers already sent' errors during exceptional conditions.

Comments:
• [INFO][other] The adoption of lpStream and the refactoring to use length-prefixed messages represent a fundamental change to the P2P wire protocol. This is a positive move for robustness, but it's crucial to confirm the backward compatibility implications. If older nodes cannot communicate with nodes running this new protocol, a clear upgrade path or versioning mechanism should be considered and communicated.
• [WARNING][performance] The readWriteSignal timeout is set to 30 seconds for each read/write operation. While this prevents indefinite hangs, it's worth considering if this fixed value is optimal for all scenarios. For very large data transfers on slow networks, it might be too short and lead to premature timeouts. Conversely, for quick command-response cycles, it might be too long to detect an unresponsive peer promptly. Could this be configurable, or perhaps dynamic based on the expected operation duration?
• [INFO][other] The addition of errorDebug to sendErrorAndClose is a valuable improvement for diagnostics, especially in a distributed P2P environment where detailed error context is often critical for debugging issues.
• [INFO][other] Moving the rate limiting and deny list checks after reading the initial command is a logical enhancement. This ensures that the client always receives a response, even if it's an error due to rate limiting, and maintains the write->read protocol flow, preventing clients from indefinitely waiting for a response that might never come.
• [INFO][other] The introduction of the send helper method is an excellent abstraction. It centralizes the client-side length-prefixed communication logic, significantly simplifying the sendTo method and improving readability and maintainability.
• [WARNING][bug] The retry condition !streamErr.message.includes('closed') && !streamErr.message.includes('reset') relies on specific error message strings. This can be brittle, as error messages might change in future libp2p versions or be localized. It would be more robust to check for specific error types or codes if libp2p provides them, or to wrap this logic with a more explicit error classification.
• [INFO][other] The retry mechanism for stale connections is a very welcome addition. Given that abortConnectionOnPingFailure is disabled (likely to keep long-running data streams alive), this provides a crucial layer of resilience against transient connection issues, ensuring better reliability for P2P interactions.
• [INFO][bug] The if (!res.headersSent) check is a critical fix to prevent 'headers already sent' errors. This improves the robustness of the HTTP command handler, especially in scenarios where errors occur after some data might have already been streamed or headers partially sent.

@alexcos20 alexcos20 merged commit 5081ba6 into main Mar 16, 2026
10 of 11 checks passed
@alexcos20 alexcos20 deleted the debug-longer-timeout branch March 16, 2026 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants