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.

Sign up for GitHub

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

Jump to bottom

swarm/: Limit negotiating inbound substreams per connection #2697

Merged
merged 8 commits into from Jun 8, 2022
Merged

swarm/: Limit negotiating inbound substreams per connection #2697

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1425886
swarm/src/connection: Limit # of negotiating inbound substreams
mxinden May 2, 2022
a98cf04
swarm/src/: Make max number of negotiating inbound streams configurable
mxinden May 11, 2022
2c97c01
swarm/src/connection: Document that limit applies per connection
mxinden Jun 7, 2022
8c08b30
swarm/: Bump version and add changelog entry
mxinden Jun 7, 2022
7a3fb18
swarm/: Fix intra doc link
mxinden Jun 7, 2022
2b187af
swarm/: Rename to max_negotiating_inbound_streams
mxinden Jun 8, 2022
885a96b
Merge branch 'master' into limit-negotiating-inbound-substreams
mxinden Jun 8, 2022
baabefa
Update swarm/CHANGELOG.md
mxinden Jun 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions swarm/src/connection/handler_wrapper.rs
View file
Open in desktop
Expand Up @@ -35,6 +35,10 @@ use libp2p_core::{
};
use std::{error, fmt, pin::Pin, task::Context, task::Poll, time::Duration};

/// The maximum number of incoming streams concurrently negotiated. Streams are
/// dropped and thus reset.
const MAX_NUM_NEGOTIATING_IN: usize = 128;
Copy link
Contributor

Choose a reason for hiding this comment

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

How was this number chosen?


/// A wrapper for an underlying [`ConnectionHandler`].
///
/// It extends [`ConnectionHandler`] with:
Expand Down Expand Up @@ -243,6 +247,11 @@ where
) {
match endpoint {
SubstreamEndpoint::Listener => {
if self.negotiating_in.len() == MAX_NUM_NEGOTIATING_IN {
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 doing this check here, we could create a wrapper around FuturesUnordered that enforces a limit on the number of concurrent futures. This would foster reuse and actually enforce this limit. At the moment, there is a risk that this check is circumvented in case the FuturesUnordered is modified in a different place without this check.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 for the reusable abstraction, though I would much rather like to remove the entire upgrade mechanism for substreams in the future.

log::warn!("Incoming substream exceeding `MAX_NUM_NEGOTIATING_IN`. Dropping.",);
return;
}

let protocol = self.handler.listen_protocol();
let timeout = *protocol.timeout();
let (upgrade, user_data) = protocol.into_upgrade();
Expand Down