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

send the correct exit code when closing the websocket #335

Merged
merged 1 commit into from
Jun 25, 2020

Conversation

BartWillems
Copy link
Contributor

@robjtede robjtede requested review from a team June 23, 2020 15:10
Copy link
Member

@fMeow fMeow left a comment

Choose a reason for hiding this comment

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

The curly braces around ctx.close can be removed.

Ok(ws::Message::Close(reason)) => {
                ctx.close(reason);
            }

The code above can be simplied into following:

Ok(ws::Message::Close(reason)) => ctx.close(reason),

It's only about style. The implementation is great.

@BartWillems
Copy link
Contributor Author

@fMeow I've updated the styles

Copy link
Member

@fMeow fMeow left a comment

Choose a reason for hiding this comment

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

That's awesome.

Signed-off-by: Bart Willems <bwillems@protonmail.com>
@robjtede robjtede merged commit 399fbee into actix:master Jun 25, 2020
@mlodato517
Copy link
Contributor

mlodato517 commented Jun 26, 2020

This is a super naiive question, but when is it "Satisfy autobahn tests with library code" vs "Satisfy autobahn tests with client code"? What's the point of the autobahn test suite? Is it, "Ensure the library can support websocket code that satisfies these things", "Ensure that websocket code leveraging this library always satisfies these things", or something else?

@robjtede
Copy link
Member

robjtede commented Jun 26, 2020

@mlodato517 The autobhan test suite is a very general way of verfying compliance of a WS server in a black-box manner. All it asks is that a websocket endpoint be available.

It just so happens that the API we have here, in part due to it's flexibility, requires developers to write some of the ping/pong/close/disconnect handling correctly to be compliant, as we found out with this PR.

As we properly analyze each failed test case in actix/actix-web#1006 we will better understand when its library code vs application code. After that, we'll be able to gather and document any requirements that developers should uphold to keep their WS applications spec-compliant. We may even come up with a more ergonomic API to create WS servers, particularly if it seems too onerous on developers to stay compliant using the current system.

@mlodato517
Copy link
Contributor

@robjtede Makes sense 👍 I think documenting the user-requirements is a good idea. Not sure if we should document as we go? Also, not sure if the websocket-autobahn example could stand as documentation on its own 🤷

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.

websockets always close with error code 1006
4 participants