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

Fix duplicate call of flushAndClose() if status code is PROTOCOL_ERROR on close() method. #567

Merged
merged 1 commit into from
Oct 12, 2017

Conversation

felippeduran
Copy link

This pull request fixes what seems to be a bug.

If the handshake is refused when establishing connection with server, flushAndClose() is called twice, first with CloseFrame.NEVER_CONNECTED, then with CloseFrame.PROTOCOL_ERROR.

@marci4
Copy link
Collaborator

marci4 commented Oct 6, 2017

Hello @felippeduran,

thx for your pull request.

Do you have some steps to repeat for this bug?
Or have you just found it by code review?

Greetings
marci4

@felippeduran
Copy link
Author

Hi, @marci4,

Thanks for the reply!

I found it by inspecting the code. Before that, I was trying to debug a case where the server would return me 503 during the HTTP handshake and I'd get the error CloseFrame.NEVER_CONNECTED, which didn't make much sense for me. Then I realized that the error code was actually CloseFrame.PROTOCOL_ERROR (as the Draft_6455 can't read the success headers to Upgrade the connection) and it was being changed somewhere during the closing state, which led me to this pull request.

So the steps to reproduce would be just to make your server return a status code like 503 during handshake and then try to connect with it.

Cheers!

@marci4
Copy link
Collaborator

marci4 commented Oct 12, 2017

Hello @felippeduran,

thank you for your answer and your pull request.

Greetings
marci4

@felippeduran felippeduran deleted the fix_protocol_error branch October 12, 2017 16:51
@marci4 marci4 added this to the Release 1.3.5 milestone Oct 13, 2017
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.

2 participants