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

If a close frame fails to send, set the state back to OPEN #504

Open
NoahAndrews opened this issue Aug 2, 2018 · 5 comments
Open

If a close frame fails to send, set the state back to OPEN #504

NoahAndrews opened this issue Aug 2, 2018 · 5 comments

Comments

@NoahAndrews
Copy link

Note: This is all based on reading the code for the latest 2.x release, not observation. I could be wrong. I did a quick check of the master branch, and it looks like it works the same way.

Right now, if close() is called on a WebSocket instance, and the close frame fails to send, then an IOException is thrown, and the WebSocket is left in the CLOSING state. If you were to then call close() again, it would finish closing down the socket, without even trying to re-send.

It seems like the correct behavior in this instance would be to catch the IOException, attempt to resend the close frame, and then immediately call doClose().

Maybe my specific idea isn't the best, but I think the code as it exists needs a little reworking.

@NoahAndrews
Copy link
Author

NoahAndrews commented Aug 2, 2018

Actually, a better solution would be to throw the IOException after putting the WebSocket back in its previous state (a forceClose parameter could be added to override this behavior and finish shutting down the websocket)

@NoahAndrews NoahAndrews changed the title NanoWSD.close() should handle IOException internally If a close frame fails to send, set the state back to OPEN Aug 3, 2018
@LordFokas
Copy link
Member

Actually, the best approach would be to understand why the server would fail to send a close frame and only then think of the correct way of dealing with each scenario.

Tell me, would you be interested in helping rewrite the entire library from the ground up? Patching on top of patches is kinda not cutting it anymore, especially if you take into account how half assed the initial implementations were...

@NoahAndrews
Copy link
Author

I'm afraid that's not likely something I'm going to be able to help you with.

@LordFokas
Copy link
Member

Well if things do go that way, I intend to do most of the hard work anyways, but having people like you pointing out inconsistencies does help a lot.

Either way, thank you for your contributions so far.

Back to business, since you've been looking into it, how and why do you think it fails to send close frames?

@NoahAndrews
Copy link
Author

NoahAndrews commented Aug 7, 2018

You're welcome, I hope I'll end up being of further help in the future (I've already got an actual fix for #328 that I need to submit). I'll try to keep track of the code being submitted here going forward.

I've never actually seen it fail to send a close frame. I was just implementing some code that would close all of the open sockets, and of course I had to handle the IOException. Naturally, I went back to look at the code that would throw that exception, and I noticed that if it did get thrown for whatever reason, you'd end up in an incorrect state. I imagine that you could trigger this by disconnecting from the network at just the right split second. So it's not about an underlying issue that needs fixing, just about keeping things sane if an error were to happen.

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