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

feat: don't report inbound stream upgrade errors to handler #3605

Merged
merged 32 commits into from
May 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
7443d0a
Don't report inbound errors on stream upgrades to handler
thomaseizinger Mar 13, 2023
0a29bae
Don't report timeout
thomaseizinger Mar 13, 2023
fad8b5b
Remove unnecessary comment
thomaseizinger Mar 13, 2023
04bee57
Reorder code
thomaseizinger Mar 13, 2023
dfa099c
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger Mar 15, 2023
982718c
Log timeout of inbound stream
thomaseizinger Mar 15, 2023
931d4d1
Remove `ConnectionHandlerUpgrErr::Timer` variant
thomaseizinger Mar 15, 2023
76e0ac7
Remove `libpp2::relay::Event::CircuitReqReceiveFailed`
thomaseizinger Mar 15, 2023
b38841f
Remove `libp2p::relay::Event::InboundCircuitReqFailed`
thomaseizinger Mar 15, 2023
cea2a3d
Remove `libp2p_request_response::InboundFailure::UnsupportedProtocols`
thomaseizinger Mar 15, 2023
27bb8ec
Add unreleased tag
thomaseizinger Mar 15, 2023
1026e4b
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger Mar 21, 2023
4010813
Add changelog entry
thomaseizinger Mar 21, 2023
f433dc7
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger Mar 22, 2023
6d43cff
Fix perf protocol
thomaseizinger Mar 22, 2023
6248fd2
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger Mar 24, 2023
40a3d3d
Remove wrong changelog entry
thomaseizinger Mar 24, 2023
ff67637
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger Mar 29, 2023
c1d2c37
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger Apr 3, 2023
028f37b
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger Apr 11, 2023
e0ad03b
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger Apr 12, 2023
c0363dd
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger Apr 16, 2023
9a3e6bf
Merge branch 'feat/no-report-inbound-error' of github.com:libp2p/rust…
thomaseizinger Apr 16, 2023
7f47dda
Fix compile errors
thomaseizinger Apr 16, 2023
7ff7f85
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger May 1, 2023
8a5274b
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger May 2, 2023
165e4be
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger May 2, 2023
a2c5944
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger May 4, 2023
5c89fd7
Fix compile errors
thomaseizinger May 4, 2023
956b814
Update changelog
thomaseizinger May 4, 2023
d53aca9
Merge branch 'master' into feat/no-report-inbound-error
thomaseizinger May 5, 2023
ae2ba86
Merge branch 'master' into feat/no-report-inbound-error
mergify[bot] May 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions misc/metrics/src/relay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ enum EventType {
ReservationReqDenied,
ReservationReqDenyFailed,
ReservationTimedOut,
CircuitReqReceiveFailed,
CircuitReqDenied,
CircuitReqDenyFailed,
CircuitReqOutboundConnectFailed,
Expand All @@ -75,9 +74,6 @@ impl From<&libp2p_relay::Event> for EventType {
EventType::ReservationReqDenyFailed
}
libp2p_relay::Event::ReservationTimedOut { .. } => EventType::ReservationTimedOut,
libp2p_relay::Event::CircuitReqReceiveFailed { .. } => {
EventType::CircuitReqReceiveFailed
}
libp2p_relay::Event::CircuitReqDenied { .. } => EventType::CircuitReqDenied,
libp2p_relay::Event::CircuitReqOutboundConnectFailed { .. } => {
EventType::CircuitReqOutboundConnectFailed
Expand Down
45 changes: 6 additions & 39 deletions protocols/dcutr/src/handler/relayed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,45 +212,12 @@ impl Handler {
<Self as ConnectionHandler>::InboundProtocol,
>,
) {
match error {
ConnectionHandlerUpgrErr::Timeout => {
self.queued_events.push_back(ConnectionHandlerEvent::Custom(
Event::InboundNegotiationFailed {
error: ConnectionHandlerUpgrErr::Timeout,
},
));
}
ConnectionHandlerUpgrErr::Timer => {
self.queued_events.push_back(ConnectionHandlerEvent::Custom(
Event::InboundNegotiationFailed {
error: ConnectionHandlerUpgrErr::Timer,
},
));
}
ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(NegotiationError::Failed)) => {
// The remote merely doesn't support the DCUtR protocol.
// This is no reason to close the connection, which may
// successfully communicate with other protocols already.
self.keep_alive = KeepAlive::No;
self.queued_events.push_back(ConnectionHandlerEvent::Custom(
Event::InboundNegotiationFailed {
error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(
NegotiationError::Failed,
)),
},
));
}
_ => {
// Anything else is considered a fatal error or misbehaviour of
// the remote peer and results in closing the connection.
self.pending_error = Some(error.map_upgrade_err(|e| {
e.map_err(|e| match e {
Either::Left(e) => Either::Left(e),
Either::Right(v) => void::unreachable(v),
})
}));
}
}
self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(
Copy link
Member

Choose a reason for hiding this comment

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

I think we are on the same page when I say that libp2p-relay should not be closing the connection based on pending_error via ConnectionHandlerEvent::Close further below in poll. That said, let's not fix it here but along with #3591.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should be a much easier change once this lands too!

match error {
Either::Left(e) => Either::Left(e),
Either::Right(v) => void::unreachable(v),
},
)));
}

fn on_dial_upgrade_error(
Expand Down
2 changes: 1 addition & 1 deletion protocols/gossipsub/src/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ impl ConnectionHandler for Handler {
handler.on_fully_negotiated_outbound(fully_negotiated_outbound)
}
ConnectionEvent::DialUpgradeError(DialUpgradeError {
error: ConnectionHandlerUpgrErr::Timeout | ConnectionHandlerUpgrErr::Timer,
error: ConnectionHandlerUpgrErr::Timeout,
..
}) => {
log::debug!("Dial upgrade error: Protocol negotiation timeout");
Expand Down
9 changes: 1 addition & 8 deletions protocols/perf/src/client/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,7 @@ impl ConnectionHandler for Handler {
}));
}
ConnectionEvent::ListenUpgradeError(ListenUpgradeError { info: (), error }) => {
match error {
ConnectionHandlerUpgrErr::Timeout => {}
ConnectionHandlerUpgrErr::Timer => {}
ConnectionHandlerUpgrErr::Upgrade(error) => match error {
libp2p_core::UpgradeError::Select(_) => {}
libp2p_core::UpgradeError::Apply(v) => void::unreachable(v),
},
}
void::unreachable(error)
}
}
}
Expand Down
12 changes: 2 additions & 10 deletions protocols/perf/src/server/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ use libp2p_swarm::{
ConnectionEvent, DialUpgradeError, FullyNegotiatedInbound, FullyNegotiatedOutbound,
ListenUpgradeError,
},
ConnectionHandler, ConnectionHandlerEvent, ConnectionHandlerUpgrErr, KeepAlive, StreamProtocol,
SubstreamProtocol,
ConnectionHandler, ConnectionHandlerEvent, KeepAlive, StreamProtocol, SubstreamProtocol,
};
use log::error;
use void::Void;
Expand Down Expand Up @@ -106,14 +105,7 @@ impl ConnectionHandler for Handler {
}
ConnectionEvent::AddressChange(_) => {}
ConnectionEvent::ListenUpgradeError(ListenUpgradeError { info: (), error }) => {
match error {
ConnectionHandlerUpgrErr::Timeout => {}
ConnectionHandlerUpgrErr::Timer => {}
ConnectionHandlerUpgrErr::Upgrade(error) => match error {
libp2p_core::UpgradeError::Select(_) => {}
libp2p_core::UpgradeError::Apply(v) => void::unreachable(v),
},
}
void::unreachable(error)
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions protocols/relay/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@
- Hide internals of `Connection` and expose only `AsyncRead` and `AsyncWrite`.
See [PR 3829].

- Remove `Event::CircuitReqReceiveFailed` and `Event::InboundCircuitReqFailed` variants.
These variants are no longer constructed.
See [PR 3605].

[PR 3605]: https://github.com/libp2p/rust-libp2p/pull/3605
[PR 3715]: https://github.com/libp2p/rust-libp2p/pull/3715
[PR 3829]: https://github.com/libp2p/rust-libp2p/pull/3829

Expand Down
13 changes: 0 additions & 13 deletions protocols/relay/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,6 @@ pub enum Event {
},
/// An inbound reservation has timed out.
ReservationTimedOut { src_peer_id: PeerId },
CircuitReqReceiveFailed {
src_peer_id: PeerId,
error: ConnectionHandlerUpgrErr<void::Void>,
},
/// An inbound circuit request has been denied.
CircuitReqDenied {
src_peer_id: PeerId,
Expand Down Expand Up @@ -537,15 +533,6 @@ impl NetworkBehaviour for Behaviour {
};
self.queued_actions.push_back(action.into());
}
handler::Event::CircuitReqReceiveFailed { error } => {
self.queued_actions.push_back(
ToSwarm::GenerateEvent(Event::CircuitReqReceiveFailed {
src_peer_id: event_source,
error,
})
.into(),
);
}
handler::Event::CircuitReqDenied {
circuit_id,
dst_peer_id,
Expand Down
49 changes: 6 additions & 43 deletions protocols/relay/src/behaviour/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,6 @@ pub enum Event {
inbound_circuit_req: inbound_hop::CircuitReq,
endpoint: ConnectedPoint,
},
/// Receiving an inbound circuit request failed.
CircuitReqReceiveFailed {
error: ConnectionHandlerUpgrErr<void::Void>,
},
/// An inbound circuit request has been denied.
CircuitReqDenied {
circuit_id: Option<CircuitId>,
Expand Down Expand Up @@ -252,10 +248,6 @@ impl fmt::Debug for Event {
.debug_struct("Event::CircuitReqReceived")
.field("endpoint", endpoint)
.finish(),
Event::CircuitReqReceiveFailed { error } => f
.debug_struct("Event::CircuitReqReceiveFailed")
.field("error", error)
.finish(),
Event::CircuitReqDenied {
circuit_id,
dst_peer_id,
Expand Down Expand Up @@ -471,41 +463,16 @@ impl Handler {

fn on_listen_upgrade_error(
&mut self,
ListenUpgradeError { error, .. }: ListenUpgradeError<
ListenUpgradeError {
error: inbound_hop::UpgradeError::Fatal(error),
..
}: ListenUpgradeError<
<Self as ConnectionHandler>::InboundOpenInfo,
<Self as ConnectionHandler>::InboundProtocol,
>,
) {
let non_fatal_error = match error {
ConnectionHandlerUpgrErr::Timeout => ConnectionHandlerUpgrErr::Timeout,
ConnectionHandlerUpgrErr::Timer => ConnectionHandlerUpgrErr::Timer,
ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
upgrade::NegotiationError::Failed,
)) => ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
upgrade::NegotiationError::Failed,
)),
ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
upgrade::NegotiationError::ProtocolError(e),
)) => {
self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
upgrade::UpgradeError::Select(upgrade::NegotiationError::ProtocolError(e)),
));
return;
}
ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Apply(
inbound_hop::UpgradeError::Fatal(error),
)) => {
self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
upgrade::UpgradeError::Apply(Either::Left(error)),
));
return;
}
};

self.queued_events.push_back(ConnectionHandlerEvent::Custom(
Event::CircuitReqReceiveFailed {
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
error: non_fatal_error,
},
self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
upgrade::UpgradeError::Apply(Either::Left(error)),
));
}

Expand All @@ -524,10 +491,6 @@ impl Handler {
ConnectionHandlerUpgrErr::Timeout,
proto::Status::CONNECTION_FAILED,
),
ConnectionHandlerUpgrErr::Timer => (
ConnectionHandlerUpgrErr::Timer,
proto::Status::CONNECTION_FAILED,
),
ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
upgrade::NegotiationError::Failed,
)) => {
Expand Down
8 changes: 0 additions & 8 deletions protocols/relay/src/priv_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,6 @@ pub enum Event {
src_peer_id: PeerId,
limit: Option<protocol::Limit>,
},
InboundCircuitReqFailed {
relay_peer_id: PeerId,
error: ConnectionHandlerUpgrErr<void::Void>,
},
/// An inbound circuit request has been denied.
InboundCircuitReqDenied { src_peer_id: PeerId },
/// Denying an inbound circuit request failed.
Expand Down Expand Up @@ -282,10 +278,6 @@ impl NetworkBehaviour for Behaviour {
handler::Event::InboundCircuitEstablished { src_peer_id, limit } => {
Event::InboundCircuitEstablished { src_peer_id, limit }
}
handler::Event::InboundCircuitReqFailed { error } => Event::InboundCircuitReqFailed {
relay_peer_id: event_source,
error,
},
handler::Event::InboundCircuitReqDenied { src_peer_id } => {
Event::InboundCircuitReqDenied { src_peer_id }
}
Expand Down
43 changes: 6 additions & 37 deletions protocols/relay/src/priv_client/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,6 @@ pub enum Event {
src_peer_id: PeerId,
limit: Option<protocol::Limit>,
},
/// An inbound circuit request has failed.
InboundCircuitReqFailed {
error: ConnectionHandlerUpgrErr<void::Void>,
},
/// An inbound circuit request has been denied.
InboundCircuitReqDenied { src_peer_id: PeerId },
/// Denying an inbound circuit request failed.
Expand Down Expand Up @@ -295,41 +291,16 @@ impl Handler {

fn on_listen_upgrade_error(
&mut self,
ListenUpgradeError { error, .. }: ListenUpgradeError<
ListenUpgradeError {
error: inbound_stop::UpgradeError::Fatal(error),
..
}: ListenUpgradeError<
<Self as ConnectionHandler>::InboundOpenInfo,
<Self as ConnectionHandler>::InboundProtocol,
>,
) {
let non_fatal_error = match error {
ConnectionHandlerUpgrErr::Timeout => ConnectionHandlerUpgrErr::Timeout,
ConnectionHandlerUpgrErr::Timer => ConnectionHandlerUpgrErr::Timer,
ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
upgrade::NegotiationError::Failed,
)) => ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
upgrade::NegotiationError::Failed,
)),
ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
upgrade::NegotiationError::ProtocolError(e),
)) => {
self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
upgrade::UpgradeError::Select(upgrade::NegotiationError::ProtocolError(e)),
));
return;
}
ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Apply(
inbound_stop::UpgradeError::Fatal(error),
)) => {
self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
upgrade::UpgradeError::Apply(Either::Left(error)),
));
return;
}
};

self.queued_events.push_back(ConnectionHandlerEvent::Custom(
Event::InboundCircuitReqFailed {
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
error: non_fatal_error,
},
self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
upgrade::UpgradeError::Apply(Either::Left(error)),
));
}

Expand All @@ -347,7 +318,6 @@ impl Handler {
OutboundOpenInfo::Reserve { mut to_listener } => {
let non_fatal_error = match error {
ConnectionHandlerUpgrErr::Timeout => ConnectionHandlerUpgrErr::Timeout,
ConnectionHandlerUpgrErr::Timer => ConnectionHandlerUpgrErr::Timer,
ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
upgrade::NegotiationError::Failed,
)) => ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
Expand Down Expand Up @@ -410,7 +380,6 @@ impl Handler {
OutboundOpenInfo::Connect { send_back } => {
let non_fatal_error = match error {
ConnectionHandlerUpgrErr::Timeout => ConnectionHandlerUpgrErr::Timeout,
ConnectionHandlerUpgrErr::Timer => ConnectionHandlerUpgrErr::Timer,
ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
upgrade::NegotiationError::Failed,
)) => ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
Expand Down
5 changes: 5 additions & 0 deletions protocols/request-response/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@
See [PR 3715].
- Remove deprecated `RequestResponse` prefixed items. See [PR 3702].

- Remove `InboundFailure::UnsupportedProtocols` and `InboundFailure::InboundTimeout`.
These variants are no longer constructed.
See [PR 3605].

[PR 3605]: https://github.com/libp2p/rust-libp2p/pull/3605
[PR 3715]: https://github.com/libp2p/rust-libp2p/pull/3715
[PR 3702]: https://github.com/libp2p/rust-libp2p/pull/3702

Expand Down
Loading