Properly handle success result for live stream

vkuptcov commented 3 months ago

Proposed changes

This PR fixes error handling inside func (c *Client) Stream(r io.Reader) error function. The current implementation tries to call err.Error() in the first switch case strings.Contains(err.Error(), FatalReadSocketErr). It leads to a panic in case of handling successful results.


[Open] Received
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x7d94e3]

goroutine 353 [running]:
github.com/deepgram/deepgram-go-sdk/pkg/client/live.(*Client).Stream(0xc000400600, {0xf879a0, 0xc0007b6180})
        .../go/pkg/mod/github.com/deepgram/deepgram-go-sdk@v1.3.0/pkg/client/live/client.go:356 +0x103

Types of changes

What types of changes does your code introduce to the community Go SDK? Put an x in the boxes that apply

[x] Bugfix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Documentation update or tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

[x] I have read the CONTRIBUTING doc
[x] I have lint'ed all of my code using repo standards
[ ] I have added tests that prove my fix is effective or that my feature works
[x] I have added necessary documentation (if appropriate)

Further comments

dvonthenen commented 3 months ago

Hi @vkuptcov

Nice catch! Thanks for the contribution!

dvonthenen commented 3 months ago

Upon further review, I don't think the code in this PR will do anything, but it wouldn't hurt having it. Hopefully, that fixed your problem.

vkuptcov commented 3 months ago

@dvonthenen bytesRead, err := r.Read(chunk) might return nil error, in that case when err.Error() is called we have a panic. The issue was introduced in this commit https://github.com/deepgram/deepgram-go-sdk/blob/1efb312b6b072c1c16962e0cc0dbb510d9c4677f/pkg/client/live/client.go#L355C4-L355C10 before the error was handled in a different way https://github.com/deepgram/deepgram-go-sdk/blob/a036640bcd2354c13eb60fb96d82eec27360fd5e/pkg/client/live/client.go#L278 so it didn't produced a panic.

In general, I would suggest to get rid of using direct errors comparison like here (err == io.EOF || err == io.ErrUnexpectedEOF) and replace it with errors.Is approach that is described here https://go.dev/wiki/ErrorValueFAQ

dvonthenen commented 3 months ago

You are absolutely right. It will return nil. It should return nil most of the time. When I run without the added code, it drops through in the "default" which you don't really need since it's implied in the switch statement.

The reason why we are using the direct error comparison is because the behavior is different depending on the error. For io.EOF and io.ErrUnexpectedEOF, we want to log the behavior difference and exit gracefully (as you can see in the return nil).

The behavior you are seeing versus what I am seeing appear to be vastly different. What version of go are you using?

I can run the example without any issues or segfaults.

vkuptcov commented 3 months ago

$ go version
go version go1.22.1 linux/amd64

dvonthenen commented 3 months ago

Will give this a try using the latest 1.22.

dvonthenen commented 3 months ago

I dont have any problem running the example on 1.22.1 and 1.22.3 without the added code in this PR but I am also on darwin/arm64. 🤔

In any case, the new code shouldn't hurt.

vkuptcov commented 3 months ago

I'll create a simple project to illustrate the issue.

dvonthenen commented 3 months ago

I'll create a simple project to illustrate the issue.

No need. I definitely understand, but drop it here if you wish.

deepgram / deepgram-go-sdk