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

Panic fix on gorilla ws when connection lost. #135

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ElecTwix
Copy link
Contributor

fix linter errors with shadow checking
upgrade linter

fix linter errors with shadow checking
upgrade linter
@ElecTwix
Copy link
Contributor Author

To be Noted: this PR changes how to create a connection it added a new step to call db.Initialize method to get ws start reading from db.
this is needed because if something goes bad with the read it needs to return an error instead of panicking and crashing the program.

Copy link
Contributor

@phughk phughk left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good, but I am wondering about the need for a separate step. That step will always be immediately called after Connect. Is it possible to tie it together with that?

name: lint
runs-on: ubuntu-latest
steps:
- uses: actions/setup-go@v3
- uses: actions/setup-go@v5
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit of a nitpick here, but will happily glance over it for this PR - could such changes be included as separate PRs? It makes them easier to review and diagnose when something goes wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bit of a nitpick here, but will happily glance over it for this PR - could such changes be included as separate PRs? It makes them easier to review and diagnose when something goes wrong.

opened #136 for separate PR.

@ElecTwix
Copy link
Contributor Author

Thanks! Looks good, but I am wondering about the need for a separate step. That step will always be immediately called after Connect. Is it possible to tie it together with that?

I tried that first but I'm not sure it is possible, we can add an error channel but I'm not sure that it will be easily understandable I think this is more straightforward than that.
Still, I will experiment and get back if I can manage to create something like that.

ElecTwix added a commit to ElecTwix/surrealdb.go that referenced this pull request May 15, 2024
@ElecTwix
Copy link
Contributor Author

Hi @phughk,

After some testing, I've added a channel-based version of the implementation. I'm not sure if it's an improvement, and I hesitated to include it here since it might not be accepted. However, here's the commit for reference: ElecTwix@908a377

@ElecTwix ElecTwix marked this pull request as draft May 21, 2024 17:53
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.

2 participants