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

misc/multistream-select: Differentiate interpretation of EOF #1823

Merged
merged 9 commits into from
Nov 9, 2020

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Nov 5, 2020

Summary / Commit Message

When a listener rejects a protocol with [Message::NotAvailable] and
the dialer does not have alternative protocols to propose then the
dialer will stop the negotiation and drop the corresponding stream. As a
listener interpret an EOF after sending a [Message::NotAvailable] as a
failed negotiation. Interpret an EOF as an io error in all other cases.

Context

The above surfaced in the following way:

With paritytech/substrate#7478 block-requests are send via libp2p-request-response. A full node supports both sending block requests and responding to block requests. A light client only supports sending block requests. When the full node asks the light client whether it supports the block request protocol, the light client replies with Message::NotAvailable. The full node thus drops the stream. Previously a dropped stream was interpreted as an io error on the light client side. Passed on to libp2p-request-response the connection is terminated (see here).

There are two solutions to this:

  • Ignore io errors in libp2p-request-responses.

  • Interpret an EOF after sending a Message::NotAvailable as a negotiation failure and not as an io failure.

This pull request proposes the latter.

> When a listener rejects a protocol with [`Message::NotAvailable`] and
the dialer does not have alternative protocols to propose then the
dialer will stop the negotiation and drop the corresponding stream. As a
listener interpret an EOF after sending a [`Message::NotAvailable`] as a
failed negotiation. Interpret an EOF as an io error in all other cases.
@mxinden
Copy link
Member Author

mxinden commented Nov 5, 2020

Thanks to @tomaka who was a great help debugging this.

Comment on lines -88 to -89
// We don't explicitly check for `Failed` because the client might close the connection when it
// realizes that we have no protocol in common.
Copy link
Member

Choose a reason for hiding this comment

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

👀

Copy link
Member

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Code changes look good to me.
The logic of the change itself might be a bit more debatable, but I agree with this solution

@romanb
Copy link
Contributor

romanb commented Nov 5, 2020

Interpret an EOF after sending a Message::NotAvailable as a negotiation failure and not as an io failure.

How about always interpreting EOF during negotiation as a failed negotiation rather than only in this particular case? Essentially making explicit that dropping or closing an I/O stream is a permissible course of action to "gracefully" fail a negotiation. I'd prefer more uniform behavior over special cases.

@tomaka
Copy link
Member

tomaka commented Nov 5, 2020

The argument is that the multiplexing layer has no choice but to report protocol errors in the form of an io::Error.
By treating io::Error as "failed negotiation", we would silently ignore errors on the Yamux and Mplex protocols.

@mxinden
Copy link
Member Author

mxinden commented Nov 5, 2020

The argument is that the multiplexing layer has no choice but to report protocol errors in the form of an io::Error.
By treating io::Error as "failed negotiation", we would silently ignore errors on the Yamux and Mplex protocols.

If I understand correctly, @romanb is suggesting to treat EOF errors as negotiation errors, not to treat all io::Error as negotiation errors. More concretely to always treat Poll::Ready(None) as NegotiationError::Failed, not only after sending NotAvailable, and not to treat Poll::Ready(Some(Err(_))) as NegotiationError::Failed.

For the reference, the current implementation:

State::RecvMessage { mut io } => {
let msg = match Pin::new(&mut io).poll_next(cx) {
Poll::Ready(Some(Ok(msg))) => msg,
Poll::Ready(None) =>
return Poll::Ready(Err(NegotiationError::from(
ProtocolError::IoError(
io::ErrorKind::UnexpectedEof.into())))),
Poll::Pending => {
*this.state = State::RecvMessage { io };
return Poll::Pending;
}
Poll::Ready(Some(Err(err))) => return Poll::Ready(Err(From::from(err))),
};

@tomaka
Copy link
Member

tomaka commented Nov 5, 2020

Right, I misread.
👍 that is a sensible option as well.

@romanb
Copy link
Contributor

romanb commented Nov 5, 2020

If I understand correctly, @romanb is suggesting to treat EOF errors as negotiation errors [..]

Yes, I'm only referring to io::ErrorKind::UnexpectedEof respectively Poll::Ready(None) for Stream APIs. I see no reason not to treat unexpected EOF uniformly as NegotiationFailed, rather than only giving it special treatment when reading after sending NotAvailable.

@mxinden
Copy link
Member Author

mxinden commented Nov 9, 2020

@romanb do my recent changes via 01b54b8 look good to you? To make sure I did not misinterpret your suggestion I will wait for an approval from you. I don't mean to rush and appreciate the help.

@romanb romanb self-requested a review November 9, 2020 13:48
Copy link
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

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

Looks good to me, though I would have expected that the change is made for all cases of Poll::Ready(None) during negotiation, i.e. for all occurrences in dialer_select.rs and listener_select.rs, instead of only at one place for the listener.

@mxinden
Copy link
Member Author

mxinden commented Nov 9, 2020

Looks good to me, though I would have expected that the change is made for all cases of Poll::Ready(None) during negotiation, i.e. for all occurrences in dialer_select.rs and listener_select.rs, instead of only at one place for the listener.

Agreed. adf31c8 makes it consistent across all cases of Poll::Ready(None). Thanks for the suggestion.

@mxinden mxinden merged commit 70bb2d7 into libp2p:master Nov 9, 2020
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.

3 participants