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 a TransportExt trait #533

Merged
merged 7 commits into from Oct 26, 2018
Merged

Add a TransportExt trait #533

merged 7 commits into from Oct 26, 2018

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Oct 1, 2018

No description provided.

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Some minimal module level docs + an example would be nice. Otherwise lgtm.

TransportTimeout::with_outgoing_timeout(self, timeout)
}

/// Adds a timeout to all the ingoing sockets created by the transport.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe s/ingoing/incoming/ ?

@tomaka
Copy link
Member Author

tomaka commented Oct 2, 2018

Tests are failing because of a compiler ICE. I can't reproduce it locally, so I assume it's been fixed in the nightly version of rustc.

@tomaka
Copy link
Member Author

tomaka commented Oct 2, 2018

I commented out the with_rate_limit function which causes the ICE.


/// Adds a timeout to all the outgoing sockets created by the transport.
#[inline]
fn with_outgoing_timeout(self, timeout: Duration) -> TransportTimeout<Self>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think outgoing is an unfortunate choice of words here. How about with_outbound_timeout? "outgoing" also means "extrovert" which reads a bit funny.

(Same for ingoing below, not really a word – how about "inbound"?).


// TODO: this method causes an ICE in Rust 1.29 but works in Rust 1.30 ; restore it when
// 1.30 is released
/*/// Adds a maximum transfert rate to the sockets created with the transport.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/transfert/transfer/

Does the rate limit apply to both in- and outbound sockets? I'd assume it's only for inbound traffic, but might be good to specify, so maybe: "Sets a maximum transfer rate on inbound sockets created with the transport."

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'd assume it's only for inbound traffic, but might be good to specify

Why do you assume it's only for inbound traffic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it's common to see inbound traffic as a possible cause for trouble and worth controlling/limiting while leaving outbound traffic free. If that is not the case then maybe that's worth mentioning explicitly in the docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

If that is not the case then maybe that's worth mentioning explicitly in the docs?

That's what I did in the latest commit.

fn with_rate_limit(
self,
max_read_per_sec: usize,
max_write_per_sec: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the max_read and max_write limits referring to the number of payloads (i.e. number of reads/writes of any size) or the number of bytes read/written?

Copy link
Member Author

Choose a reason for hiding this comment

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

The size of a payload is totally unpredictable, so I don't see how using a number of payloads would be a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an example of "per payload" rate limiting, Nginx rate limiting works on the number of requests (of any kind/size). Anyway: these are bytes then.

/// use std::time::Duration;
///
/// let _transport = TcpConfig::new()
/// .with_timeout(Duration::from_secs(20));
Copy link
Contributor

Choose a reason for hiding this comment

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

When does the clock start ticking, on first poll? On first read/write?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a timeout for when you connect, not when you read/write.

@tomaka
Copy link
Member Author

tomaka commented Oct 25, 2018

Restored the with_rate_limit method.
This will cause the build to fail until Rust 1.30 is released.

@tomaka tomaka requested a review from dvdplm October 25, 2018 15:58
@tomaka tomaka merged commit 16d5bc7 into libp2p:master Oct 26, 2018
@tomaka tomaka deleted the transport-ext branch October 26, 2018 09:08
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Oct 29, 2018
…e-handled_node

* upstream/master:
  Add protocol to report external address view. (libp2p#566)
  Add a TransportExt trait (libp2p#533)
  libp2p#399 remove tokio_current_thread tests (libp2p#577)
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Oct 29, 2018
…e-handled_node_tasks

* upstream/master:
  Add protocol to report external address view. (libp2p#566)
  Add a TransportExt trait (libp2p#533)
  libp2p#399 remove tokio_current_thread tests (libp2p#577)
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Oct 29, 2018
…lection_stream

* upstream/master:
  Add protocol to report external address view. (libp2p#566)
  Add a TransportExt trait (libp2p#533)
  libp2p#399 remove tokio_current_thread tests (libp2p#577)
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Nov 1, 2018
…ref-debug-impl

* upstream/master:
  Use paritytech/rust-secp256k1 (libp2p#598)
  Use websocket 0.21.0 (libp2p#597)
  Reexport multihash from the facade (libp2p#587)
  Add substrate to the list of projects using libp2p (libp2p#595)
  Remove spaces before semicolons (libp2p#591)
  Add protocol to report external address view. (libp2p#566)
  Add a TransportExt trait (libp2p#533)
  libp2p#399 remove tokio_current_thread tests (libp2p#577)
  Remove even more unused files (libp2p#581)
  Fix the polling process in handled node (libp2p#582)
  Fix panicking when Kad responder is destroyed (libp2p#575)
  Remove other unused files (libp2p#570)
  Fix panic in raw swarm (libp2p#571)
  Remove two unused files (libp2p#567)
  New core (libp2p#568)
  Remove the old API (libp2p#565)
  Change some `nat_traversal`s to consider a prefix. (libp2p#550)
  Add some documentation to listeners stream (libp2p#547)
  Add shutdown functionality to `NodeStream`. (libp2p#560)
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.

None yet

3 participants