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

1005 status code is incorrectly treated as "abnormal" shutdown #3

Closed
lpinca opened this issue Jan 30, 2018 · 10 comments
Closed

1005 status code is incorrectly treated as "abnormal" shutdown #3

lpinca opened this issue Jan 30, 2018 · 10 comments

Comments

@lpinca
Copy link

lpinca commented Jan 30, 2018

If the close frame contains no status code (payload length == 0), the close code is 1005. In this case sockette should not reconnect.

Ref: https://tools.ietf.org/html/rfc6455#section-7.1.5

@lukeed
Copy link
Owner

lukeed commented Jan 30, 2018

When do you encounter a 1005 code in a normal setting? In my experience, a 1005 is always unexpected, meaning a closure happened before something had time to write a proper code & reason.

@lpinca
Copy link
Author

lpinca commented Jan 30, 2018

A simple ws.close() from a server client, unless you specify a close code ofc.

@lukeed
Copy link
Owner

lukeed commented Jan 30, 2018

Yeah, I suppose this is the result of my habit(s). I always send ws.close(OK).

The 1005 code is basically the 204 of WebSockets. Technically okay, but not all that informative/useful.

I'll look into accepting 1005 within onclose & see how it fares in my apps, but sending 1000 intentionally will likely stay. Thanks~!

@lpinca
Copy link
Author

lpinca commented Jan 30, 2018

To clarify: if the server sends an empty close frame (0x88 0x00) the client closes with 1005 and that isn't an abnormal closure.

sending 1000 intentionally will likely stay.

It's your lib so you can obviously do whatever you want but it is inconsistent with what browsers do. If you don't specify a close code, the close frame should be empty. You prevent the user from sending an empty close frame by forcing a 1000 close code and increase the bandwidth usage by 2 bytes per frame 😄 .

@lukeed
Copy link
Owner

lukeed commented Jan 30, 2018

Yea, for sure! I've always preferred to be explicit with my codes & am not closing connections on a high frequency, so the extra 2 bytes hasn't been an issue 😆.

I'll look at this tonight. Forcing this practice on others probably isn't the right approach... And, unfortunately, changing this probably requires a really sudden 2.0 release lol

Thanks for the feedback~!

@lukeed lukeed closed this as completed in f8771f0 Jan 30, 2018
@lukeed
Copy link
Owner

lukeed commented Jan 30, 2018

Hey @lpinca, do you mind taking a quick peek and verifying the change for me? Dropped it into my apps and it's still working as expected~

@lpinca
Copy link
Author

lpinca commented Jan 31, 2018

It looks good. If I'm not wrong you can also remove the fn variable declaration which is no longer used.

@lukeed
Copy link
Owner

lukeed commented Jan 31, 2018

Ah, right! What I get with hurried coding...

I'll fix that, but what do you think about the server? Does this constitute a major change, or is it just a minor after all since the interface API hasn't changed

@lpinca
Copy link
Author

lpinca commented Jan 31, 2018

The reconnection change is a bug fix. The close() change is arguably a breaking change but imho it is a bug fix as per comment above (if no close code is specified, the close frame should be empty).

@lukeed
Copy link
Owner

lukeed commented Jan 31, 2018

Released in 1.1.0 -- a minor felt more accurate. Thanks again!

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

No branches or pull requests

2 participants