Use tk_listen to absorb errors on the TcpListener #402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
assigned 
gnunicorn
reviewed
Aug 8, 2018
tcp-transport/src/lib.rs
Outdated
| @@ -108,6 +111,8 @@ impl Transport for TcpConfig { | |||
| .map(|listener| { | |||
| // Pull out a stream of sockets for incoming connections | |||
| listener.incoming() | |||
| .sleep_on_error(Duration::from_secs(1)) | |||
There was a problem hiding this comment.
The examples all use 100ms, why did you opt for waiting a full second on the connection that errored?
There was a problem hiding this comment.
I guess this should be in the TcpConfig.
| .map_err(|err| { | ||
| debug!("Error in TCP listener: {:?}", err); | ||
| err | ||
| }) | ||
| }) |
There was a problem hiding this comment.
While on it, why not also add the .listen(MAX_SIMULTANEOUS_CONNECTIONS) from the example - we could even make that configurable, putting the TcpConfig to good use finally.
There was a problem hiding this comment.
What listen does is handled by the swarm, although the swarm doesn't have a limit to the maximum number of connections at the moment and should have one.
gnunicorn
approved these changes
Aug 8, 2018
3 tasks
mxinden
added a commit
to mxinden/rust-libp2p
that referenced
this pull request
Aug 26, 2022
The `sleep_on_error` mechanism in `libp2p-tcp` would delay the next poll on the listener stream when an error happens. This mechanism was introduced in libp2p#402 based on the [`tk_listen`](https://docs.rs/tk-listen/latest/tk_listen/) crate also referenced in the tokio documentation. When running out of file descriptors, the listening socket would return an `EMFILE`. Instead of polling the socket again, thus likely receiving another `EMFILE`, one would instead wait for 100ms. Modern operating systems should run with high file descriptor limits and thus this error should not happen in the wild. In addition, delaying the next poll only covers up the issue, but does not solve it. With the above in mind, this pull request removes the daly. (The mention of `tk_listen` has since been removed from tokio.)
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The documentation of TcpListener suggests doing what this PR does.
The
TcpListenercan sometimes produce an error which would just close the listener.I think handling this directly in the TCP transport is the right location. There might be valid reasons why a listener produces an error, therefore the upper layers should continue to drop the listener if an error happens.