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

Handle CLOSE reason as Incoming #31

Merged
merged 11 commits into from
May 27, 2021
Merged

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Apr 14, 2021

When a remote sends us a CLOSE message it may contain a reason code and "application data" with further details about why the connection was closed.

Before this PR there was no way for applications using soketto to know what the code/data was sent from the remote.

Further context here.

This is an alternative to #29 and returns the close reason as Incoming::Closed instead of Error::Closed(reason).

src/data.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Looks good to me after TODOs are fixed

@dvdplm dvdplm marked this pull request as ready for review April 16, 2021 13:47
@dvdplm
Copy link
Contributor Author

dvdplm commented Apr 16, 2021

@tomaka I think this is good to go now. The only relevant change since you last looked at this is f5b9090 (don't bail early if we can't echo back their CLOSE e.g. if they already closed the connection).

Copy link
Contributor

@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.

👍

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

LGMT, I don't know why we are now ignoring the errors on sending the ACK to close, but I also don't see how that would break anything.

@dvdplm
Copy link
Contributor Author

dvdplm commented May 27, 2021

I don't know why we are now ignoring the errors on sending the ACK to close

Because otherwise we'd do an early exit and the code higher up the call chain would not see the CloseReason. The send errors stem from the other side having closed the connection already (which in my testing was pretty common).

@niklasad1 niklasad1 merged commit 00815e5 into develop May 27, 2021
@tomaka tomaka deleted the dp-close-reason-as-incoming branch May 27, 2021 10:38
mxinden added a commit to libp2p/rust-libp2p that referenced this pull request Oct 30, 2021
Pass websocket CLOSE reason to the app as an
`Incoming::Closed(soketto::CloseReason)`.

This PR is a companion to paritytech/soketto#31 and lets
applications know what the remote end claimed to be the reason for closing the
connection (if any).

`IncomingData` is now renamed to `Incoming` and the actual data (text or binary)
is moved into its own `Data` enum, which in turn allows `into_bytes()` and the
`AsRef` impl to apply more sanely to something that is actually data.

Co-authored-by: David Palm <[email protected]>
santos227 pushed a commit to santos227/rustlib that referenced this pull request Jun 20, 2022
Pass websocket CLOSE reason to the app as an
`Incoming::Closed(soketto::CloseReason)`.

This PR is a companion to paritytech/soketto#31 and lets
applications know what the remote end claimed to be the reason for closing the
connection (if any).

`IncomingData` is now renamed to `Incoming` and the actual data (text or binary)
is moved into its own `Data` enum, which in turn allows `into_bytes()` and the
`AsRef` impl to apply more sanely to something that is actually data.

Co-authored-by: David Palm <[email protected]>
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.

4 participants