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 #119, panic gorilla ws when connection lost. #138

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

ElecTwix
Copy link
Contributor

@ElecTwix ElecTwix commented May 21, 2024

fix #119
instead of panic it will return error when any send function call when connection lost.

instead of panic it will return error when any send function call
@ElecTwix
Copy link
Contributor Author

TLDR: This pull request aims to address issue #135 without requiring an additional function call.

@timpratim
Copy link
Contributor

Hi @ElecTwix , thank you for the PR. Everything looks great. It would be great if we could add a test case for the Send method in the same PR. Happy to merge the PR then.

Refactor db_test.go openConnection for flexible usage
@ElecTwix
Copy link
Contributor Author

ElecTwix commented Jun 12, 2024

It would be great if we could add a test case for the Send method in the same PR.

Hi @timpratim added test named connectionBreakAndClose for test the Send method to checks it's error return. I would like to add tests directly but it require mocks for gorilla itself for make it simpler I came up with this but in my next PRs, I will address this too.

@timpratim
Copy link
Contributor

Thank you @ElecTwix. Appreciate it!
Merging the PR.

@timpratim timpratim merged commit 8f4a698 into surrealdb:main Jun 12, 2024
2 checks passed
var res rpc.RPCResponse
err := ws.read(&res)
if err != nil {
shouldExit := ws.handleError(err)

Choose a reason for hiding this comment

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

The function should exit on any error returned from ws.Conn.ReadMessage. You can fix by inlining WebSocket.read here and exiting when the call to ws.Conn.ReadMessage returns an error. Because handleError does not accurately detect all websocket errors, this thing can loop endlessly and eventually panic (the gorilla detects useless application loops).

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.

Bug: Unhandled errors in websockets can lead to panics
3 participants