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

Close behavior #88

Open
mbonneau opened this issue Jan 1, 2019 · 2 comments
Open

Close behavior #88

mbonneau opened this issue Jan 1, 2019 · 2 comments

Comments

@mbonneau
Copy link
Member

mbonneau commented Jan 1, 2019

This issue relates to PR #86.

Right now calling close() only calls end() on the underlying socket. This can be problematic if the resource does not close in a timely manner as there is no timeout and there is no way to force the connection closed.

The timeout solution would not solve the problem of closing the socket if the loop is stopped prior to flushing and closing.

Forcing the connection closed when close() is called could cause data loss or unclean closes on the server end.

Would it make sense to change the WebSocket object to use similar semantics to the underlying connection by added an end() that ends cleanly and a close() that just closes the connection?

This would allow the user to manage the connection close by adding their own close timeout and giving them the ability to just close the underlying connection if desired.

@caucik
Copy link

caucik commented Apr 29, 2019

This would be a better example to demonstrate the issue. In this example the exception is thrown 5 seconds after the connection is established to show that even after catching and handling the exception the connection can no longer be resumed nor properly closed.

exception_example.zip

Maybe the thing is not to have the ability to "force-close" the connection but have the ability for React\EventLoop to recover from exception?
(Also if you find my code misusing the API I would be glad to be corrected.)

Edit: wording

@clue
Copy link
Member

clue commented Jun 28, 2019

@mbonneau I agree that the current situation is sub-optimal. We face the same problem in the underlying ReactPHP libraries, that's why we offer an end() and close() method depending on the use case.

In a higher-level protocol implementation like WebSocket, I think it makes sense to align with common implementations, such as those found in browsers. The usual WebSocket API only provides a single close() method with (apparently) no way to forcefully terminate a connection. Some APIs (websockets/ws#1033) provide an additional "terminate" or "force-close" method, so this might be an option.

From a consumer perspective I don't think this is something a lot of consumers are actually going to implement properly, so perhaps an automatic soft-close timeout might be an option? I.e. first soft-close, then wait for a maximum of x seconds and then force-close if the connection is still active. This will probably solve this for next to 99% of the use cases.

@caucik I'm not sure what you're suggesting, can you rephrase this to highlight how this is related to this ticket?

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

3 participants