-
Notifications
You must be signed in to change notification settings - Fork 38
tweaks(yamux): Improvements to reducer #1085
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
base: develop
Are you sure you want to change the base?
Conversation
8b6ee76
to
30e5b3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements several improvements to the Yamux reducer handling and stream state management. Key changes include:
- Adjusting window sizing constants and modifying the window update logic.
- Introducing multiple new helper functions in the YamuxStreamState to manage remote/local windows and frame queuing.
- Updating reducer logic to handle new Yamux frame types and improving error dispatching.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
p2p/src/network/yamux/p2p_network_yamux_state.rs | Adjusted window constants and added helper methods for frame handling and window updates. |
p2p/src/network/yamux/p2p_network_yamux_reducer.rs | Refactored frame dispatching logic with new action variants and streamlined error handling. |
p2p/src/network/yamux/p2p_network_yamux_actions.rs | Added new Yamux action variants to support refined frame processing. |
p2p/src/network/yamux/mod.rs | Updated exports to expose the new YamuxStreamState. |
p2p/src/network/scheduler/p2p_network_scheduler_state.rs | Exposed additional utility methods for stream retrieval and incoming stream counting. |
node/src/action_kind.rs | Expanded action kind enumeration and updated the count accordingly. |
Comments suppressed due to low confidence (2)
p2p/src/network/yamux/p2p_network_yamux_state.rs:10
- The MAX_WINDOW_SIZE constant now equals INITIAL_RECV_BUFFER_CAPACITY (256kb), which contradicts the previous comment indicating 16mb. If this change is intentional, please update the comment to prevent confusion.
pub const MAX_WINDOW_SIZE: u32 = INITIAL_RECV_BUFFER_CAPACITY as u32;
p2p/src/network/yamux/p2p_network_yamux_reducer.rs:324
- [nitpick] Consider using a more descriptive error message in the expect() call to clearly document why an accepted frame is mandatory.
frame = accepted.expect("frame should be accepted or error should be returned");
let frame = yamux_state.incoming.pop_front().unwrap(); // Cannot fail | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using unwrap() here assumes the incoming frame queue is never empty; consider using expect() with a descriptive error message to aid future debugging if the invariant is broken.
let frame = yamux_state.incoming.pop_front().unwrap(); // Cannot fail | |
let frame = yamux_state.incoming.pop_front().expect("Expected a frame in the incoming queue"); // Cannot fail |
Copilot uses AI. Check for mistakes.
No description provided.