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

Correct web socket closing #1227

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

Serpion-ua
Copy link
Contributor

Correct web socket closing in case if Server caught error

@Serpion-ua
Copy link
Contributor Author

@marci4 reopened PR

@Serpion-ua
Copy link
Contributor Author

@PhilipRoman could you please take a look? Thanks.

@PhilipRoman
Copy link
Collaborator

I'm looking at it right now and as far as I can see, there are 3 changes:

  1. new option to pass message as parameter to stop()
  2. in handleFatal(), WebSocketWorker shutdown loop moved after call to stop()
  3. ws.close() not happening when catching fatal error in run()

Can you explain what 2. and 3. are supposed to do?

@Serpion-ua
Copy link
Contributor Author

Serpion-ua commented Mar 26, 2022

@PhilipRoman
2. Yes, it allows us to correctly stop the server by sending the correct close message to the clients. Previously we try to send such messages by calling stop() AFTER selector/worker threads are stopped, thus no close messages are reached to clients. Now we will first send such messages to the clients and only after stop threads.
3. We no longer require such closing because we now correctly close socket in handleFatal()

Overall, you could reproduce the problem at current master branch by removing ws.close() in fatal error handling (as you mentioned in 3 change) and launch org.java_websocket.issues.Issue1160Test#fatalErrorShallNotBeHandledByServer test. That test expect to receive a close message at the client but will never get it. With that change close message are successfully received by the client.

@marci4 marci4 added this to the Release 1.5.3 milestone Mar 28, 2022
@marci4 marci4 merged commit 2c9b091 into TooTallNate:master Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants