-
Notifications
You must be signed in to change notification settings - Fork 998
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
Add some helpers for substream upgrades #896
Conversation
Actually, since the Ping protocol doesn't have any length prefix, we can't use these helpers. |
core/src/upgrade/transfer.rs
Outdated
|
||
/// Builds a buffer that contains the given integer encoded as variable-length. | ||
fn build_int_buffer(num: usize) -> io::Window<[u8; 10]> { | ||
let mut len_data = [0; 10]; |
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.
let mut len_data = [0; 10]; | |
let mut len_data = unsigned_varint::encode::u64_buffer(); |
core/src/upgrade/transfer.rs
Outdated
// already in `len_buf`. | ||
let data_start = &data_start[..cmp::min(data_start.len(), len)]; | ||
let mut data_buf = vec![0; len]; | ||
std::io::Write::write_all(&mut data_buf.as_mut_slice(), data_start) |
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.
How about:
let n = cmp::min(data_start.len(), len);
let mut data_buf = vec![0; len];
data_buf[.. n].copy_from_slice(&data_start[.. n]);
core/src/upgrade/transfer.rs
Outdated
then: TThen, | ||
) -> ReadOne<TSocket, TThen> | ||
where | ||
TThen: FnOnce(Vec<u8>) -> Result<TOut, TErr>, |
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.
Instead of taking a TThen
parameter, why could the ReadOne
future not just yield the Vec<u8>
and the existing future combinators are used?
The Floodsub protocol implementation would be
upgrade::read_one(socket, 2048).from_err().and_then(|packet| { ... })
instead of
upgrade::read_one(socket, 2048, |packet| { ... })
Granted, the associated FloodsubConfig::Future
type is somewhat more difficult with
type Future =
AndThen<
FromErr<upgrade::ReadOne<TSocket>, FloodsubDecodeError>,
Result<FloodsubRpc, FloodsubDecodeError>,
fn(Vec<u8>) -> Result<FloodsubRpc, FloodsubDecodeError>
>;
But that just reflects the state of the current futures system and if performance is less of an issue one could always use trait objects instead.
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.
I'll add both versions.
cc #833
Adds the
write_one
,read_one
andrequest_response
methods inupgrade
. Makes it possible to easily write one message, read one message, or write then read one message.Makes
floodsub
use these new methods.Eventually I'll change Kademlia to also use them (cc #830).
Ping is blocked by #884. Either this PR should be rebased over #884, or the opposite.