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

Add some helpers for substream upgrades #896

Merged
merged 7 commits into from
Jan 29, 2019
Merged

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Jan 28, 2019

cc #833

Adds the write_one, read_one and request_response methods in upgrade. 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.

@ghost ghost assigned tomaka Jan 28, 2019
@ghost ghost added the in progress label Jan 28, 2019
@tomaka
Copy link
Member Author

tomaka commented Jan 28, 2019

Ping is blocked by #884.

Actually, since the Ping protocol doesn't have any length prefix, we can't use these helpers.


/// 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];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut len_data = [0; 10];
let mut len_data = unsigned_varint::encode::u64_buffer();

// 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)
Copy link
Contributor

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]);

then: TThen,
) -> ReadOne<TSocket, TThen>
where
TThen: FnOnce(Vec<u8>) -> Result<TOut, TErr>,
Copy link
Contributor

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.

Copy link
Member Author

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.

@tomaka tomaka merged commit b8dfa72 into libp2p:master Jan 29, 2019
@ghost ghost removed the in progress label Jan 29, 2019
@tomaka tomaka deleted the upgrade-helpers branch January 29, 2019 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants