Skip to content

Fixed SockJS closure with correct close status code #811

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

Merged
merged 3 commits into from
Jan 30, 2018

Conversation

slinkydeveloper
Copy link
Member

As explained here #809 and here #804

@vietj
Copy link
Contributor

vietj commented Jan 19, 2018

we also need to handle the EventBusBridge#checkCallHook method basically wherever the EB bridge closes the socket we need to check which code to use

@slinkydeveloper
Copy link
Member Author

What do you think about 1002? "1002 indicates that an endpoint is terminating the connection due to a protocol error."

@slinkydeveloper
Copy link
Member Author

The close call you are talking about is called after the bridge rejected the socket connection right? In this case maybe is better 1011 "The server is terminating the connection because it encountered an unexpected condition that prevented it from fulfilling the request." or 1013 "The server is terminating the connection due to a temporary condition, e.g. it is overloaded and is casting off some of its clients." (found here: https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent )

@slinkydeveloper slinkydeveloper changed the title Fixed SockJS timeout with correct close status code Fixed SockJS closure with correct close status code Jan 19, 2018
@vietj
Copy link
Contributor

vietj commented Jan 19, 2018

@pmlopes any opinion on the code to use ?

@pmlopes
Copy link
Member

pmlopes commented Jan 22, 2018

@slinkydeveloper @vietj regarding the code, i can't find a clear reference that states which code to use, but it seems that 1001 is ok as the eventbus is going away.

Regarding the checkCallHook i think we need to make the new extra method close(int code, String message) a public API so it's up to the user to properly close it as their wishes.

@vietj
Copy link
Contributor

vietj commented Jan 22, 2018

@pmlopes for the new API, the status code has only meaning for Websocket and does not have for other transports...

Copy link
Contributor

@vietj vietj left a comment

Choose a reason for hiding this comment

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

Need to test the case of invalid message, the client sends an invalid message that force the connection to be closed by the bridge

@slinkydeveloper
Copy link
Member Author

slinkydeveloper commented Jan 22, 2018

Investigating a bit inside sockjs-client it seems that both status code and reason are handled by client APIs, but it's not clear how they are transported with transports different from Websocket.

@vietj
Copy link
Contributor

vietj commented Jan 22, 2018

for other transport, we just close like we do today.

@vietj
Copy link
Contributor

vietj commented Jan 24, 2018

Can you add the handling for protocol error @slinkydeveloper ?

@slinkydeveloper
Copy link
Member Author

@vietj any tips on how test it?

# Conflicts:
#	vertx-web/src/main/java/io/vertx/ext/web/handler/sockjs/impl/EventBusBridgeImpl.java
@pmlopes pmlopes merged commit 2bcc06f into vert-x3:master Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants