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

error codes: provide error codes on stream reset and connection close #623

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Jul 16, 2024

No description provided.

[RFC](https://www.rfc-editor.org/rfc/rfc9000.html#section-19.19-6.2.1).

### Yamux
Yamux has no `STOP_SENDING` frame, so there's no way to signal an error on
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's an addition we could add to the spec to support this in a backwards compatible way. Not worth doing now, but maybe could be added in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I made some suggestions for the wire encoding in #479. You'd have to check if existing implementations handle this gracefully.

Copy link
Member Author

Choose a reason for hiding this comment

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

@marten-seemann do you mean suggestions for sending an error code? There's this PR for yamux for that #622

Copy link
Member Author

Choose a reason for hiding this comment

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

@MarcoPolo I think we can add this, there are 16 flag bits, so we do have space for a new flag and implementations only look at the flag bits they care about. I created #627

Copy link
Contributor

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

Seems reasonable, let's do it!

error-codes/README.md Show resolved Hide resolved
Copy link
Contributor

@yiannisbot yiannisbot left a comment

Choose a reason for hiding this comment

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

Mostly cosmetic nits. Thanks for putting this together.

error-codes/README.md Outdated Show resolved Hide resolved
error-codes/README.md Outdated Show resolved Hide resolved

Error codes provide a best effort guarantee that the error will be propogated to
the applciation layer. This provides backwards compatibility with older nodes,
allows smaller implementations and using transports that don't provide a
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we mean "less popular" here maybe?

Suggested change
allows smaller implementations and using transports that don't provide a
allows less popular implementations that are using transports that don't provide a

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about popularity here - maybe "simpler" instead of "smaller"?

Either way this needs an Oxford comma:

Suggested change
allows smaller implementations and using transports that don't provide a
allows smaller implementations, and using transports that don't provide a

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 actually do prefer "smaller" in terms of code size smaller. I like simpler too. Happy to change if you have strong opinions.

information to the receiver. Receiving a `Bad Request: Connection Closed` error
on reading from a stream also tells the receiver that no more streams can be
started on the same connection. Implementations MUST provide the Connection
Close error code on streams that are reset as a result of remote closing the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Close error code on streams that are reset as a result of remote closing the
Close error code on streams that are reset as a result of the remote closing the

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need this?

error-codes/README.md Outdated Show resolved Hide resolved
error-codes/README.md Outdated Show resolved Hide resolved
error-codes/README.md Outdated Show resolved Hide resolved
error-codes/README.md Outdated Show resolved Hide resolved
error-codes/README.md Outdated Show resolved Hide resolved
error-codes/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

You probably want to create a registry, so application protocols can register ranges of values. Otherwise, it's likely that you'll end up with collisions.

Comment on lines 4 to 6
In the event that a node detects violation of a protocol or is unable to
complete the necessary steps required for the protocol, it's useful to provide a
reason for disconnection to the other end. This error code can be sent on both
Copy link
Contributor

Choose a reason for hiding this comment

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

It's broader than this. This doesn't only apply to protocol violations (which should be rare), but also to common events like running into resource limits, connections being pruned by the connection manager, etc.

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've rephrased this and removed the specific reference to protocol errors.

[RFC](https://www.rfc-editor.org/rfc/rfc9000.html#section-19.19-6.2.1).

### Yamux
Yamux has no `STOP_SENDING` frame, so there's no way to signal an error on
Copy link
Contributor

Choose a reason for hiding this comment

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

I made some suggestions for the wire encoding in #479. You'd have to check if existing implementations handle this gracefully.

For Connection Close, the 32bit Length field is to be interpreted as the error
code. The error code MUST be sent as an Big Endian unsigned 32 bit integer.

For Stream Resets, the error code is sent as the first 4 bytes of the Data Frame
Copy link
Contributor

Choose a reason for hiding this comment

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

Linking this here since this will change: #622 (comment)

@sukunrt
Copy link
Member Author

sukunrt commented Sep 11, 2024

@marten-seemann

You probably want to create a registry, so application protocols can register ranges of values. Otherwise, it's likely that you'll end up with collisions.

Do we need to? Application protocols can have conflicting error codes. From the application context, it's clear which is the relevant error. I am planning to reserve a space for libp2p to use for its errors and let applications do whatever they like with the application error codes space.
This is similar to how QUIC does it. For example,
DNS over QUIC: https://www.rfc-editor.org/rfc/rfc9250.html
RTP over QUIC: https://www.ietf.org/archive/id/draft-ietf-avtcore-rtp-over-quic-11.html
have conflicting error codes.

@marten-seemann
Copy link
Contributor

This is similar to how QUIC does it. For example,
DNS over QUIC: https://www.rfc-editor.org/rfc/rfc9250.html
RTP over QUIC: https://www.ietf.org/archive/id/draft-ietf-avtcore-rtp-over-quic-11.html
have conflicting error codes.

They're not conflicting, since the application protocol is negotiated during the QUIC handshake.

From the application context, it's clear which is the relevant error.

This is not correct, thanks to multistream.

@sukunrt
Copy link
Member Author

sukunrt commented Sep 11, 2024

I see. All connections are just libp2p connections and they can speak multiple application protocols any of which may close the underlying connection on error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

Proposal: provide error codes when closing connections and resetting streams
5 participants