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

Stabilize WS tests #91

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Stabilize WS tests #91

merged 1 commit into from
Aug 9, 2023

Conversation

mumoshu
Copy link
Contributor

@mumoshu mumoshu commented Aug 9, 2023

The tests were failing time to time for me saying this:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9f9f27]

goroutine 23 [running]:
github.com/surrealdb/surrealdb.go/pkg/gorilla.(*WebSocket).initialize.func1()
	/home/mumoshu/p/surrealdb.go/pkg/gorilla/gorilla.go:205 +0xe7
created by github.com/surrealdb/surrealdb.go/pkg/gorilla.(*WebSocket).initialize
	/home/mumoshu/p/surrealdb.go/pkg/gorilla/gorilla.go:196 +0x69

The paniic points to https://github.com/surrealdb/surrealdb.go/blob/main/pkg/gorilla/gorilla.go#L205, and I see ws.logger being nil when the panic happens.

We should make the logger non-nil, or make the ws.read goroutine wait until the logger is initialized. Otherwise, every time ws.read gets an error from the connection (e.g. the connection is closed?), AND the logger is nil (which is always the case for our no-opt gorilla ws impl in the tests), the panic happens.

That's said, let's always initialize the logger for now.

We may make the logger getting lazily initialized on demand, but I think that's not a priority for now :)

The tests were failing time to time for me saying this:

```
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9f9f27]

goroutine 23 [running]:
github.com/surrealdb/surrealdb.go/pkg/gorilla.(*WebSocket).initialize.func1()
	/home/mumoshu/p/surrealdb.go/pkg/gorilla/gorilla.go:205 +0xe7
created by github.com/surrealdb/surrealdb.go/pkg/gorilla.(*WebSocket).initialize
	/home/mumoshu/p/surrealdb.go/pkg/gorilla/gorilla.go:196 +0x69
```

The paniic points to https://github.com/surrealdb/surrealdb.go/blob/main/pkg/gorilla/gorilla.go#L205,
and I see `ws.logger` being `nil` when the panic happens.

We should make the logger non-nil, or make the `ws.read` goroutine wait until the logger is initialized.
Otherwise, every time `ws.read` got an error from the connection (e.g. the connection is closed?),
AND the logger is nil (which is always the case for our no-opt `gorilla` ws impl in the tests), the panic happens.

That's said, let's always initialize the logger for now.

We may make the logger getting lazily initialized on demand, but I think that's
not a priority for now :)
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 for fixing and describing!

@phughk phughk merged commit bbf7fdf into surrealdb:main Aug 9, 2023
3 checks passed
@mumoshu mumoshu deleted the stabilize-ws-tests branch August 9, 2023 10:20
@ElecTwix
Copy link
Contributor

ElecTwix commented Aug 9, 2023

I have identified that there is a bug in the code causing std.out to be used instead of https://github.com/surrealdb/surrealdb.go/blob/main/pkg/logger/logger.go#L51C2-L51C2 when the path is empty. Rest assured, I will create a PR to fix this issue promptly. Thank you for bringing it to my attention.

ElecTwix added a commit to ElecTwix/surrealdb.go that referenced this pull request Aug 9, 2023
ElecTwix added a commit to ElecTwix/surrealdb.go that referenced this pull request Aug 9, 2023
@ElecTwix ElecTwix mentioned this pull request Aug 9, 2023
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.

3 participants