revdotcom / revai-go

Rev.ai golang client
https://www.rev.ai/docs
MIT License
21 stars 12 forks source link

Bug/repeated read leads to panic #16

Closed jdupl123 closed 3 years ago

jdupl123 commented 3 years ago

this draft pr needs further testing but aims to fix #15.

threeaccents commented 3 years ago

Do you think we should add a buffer? maybe we make it a map[string][int]. The key would be the error message int would be the count. We can say if the same error occurs 5 or more times close connection.

There should be a reset for the errors map.

jdupl123 commented 3 years ago

The error map idea is interesting, but we don't want to kill the connection just because we got an error 5 times. For instance 5 malformed json messages Is ok. but 5 errors from https://github.com/gorilla/websocket/blob/e8629af678b7fe13f35dff5e197de93b4148a909/conn.go#L965 is not.

One way of solving this issues is, call https://github.com/gorilla/websocket/blob/e8629af678b7fe13f35dff5e197de93b4148a909/json.go#L50 any error from this is permanent and we need to redo the connection.

Then do https://github.com/gorilla/websocket/blob/e8629af678b7fe13f35dff5e197de93b4148a909/json.go#L54 separately.

jdupl123 commented 3 years ago

@threeaccents we may want to address the errors getting silently dropped later, but for now this update should help stop the app from crashing when there is a repeated error.

threeaccents commented 3 years ago

@jdupl123 Thank you!

Would there be a case where maybe a repeated error occurs and inbetween maybe another error pops up?

same_error...
same_error...
same_error...
OTHER_ERROR...
same_error...
same_error...
same_error...

If so maybe a map would be better than the strings we have now. map[string]int where the key is the error string and the value is the count. So we can track multiple errors.

jdupl123 commented 3 years ago

@threeaccents

for the case we are trying to catch we will always get the same error returned.

we call: https://github.com/gorilla/websocket/blob/e8629af678b7fe13f35dff5e197de93b4148a909/json.go#L50

which calls: https://github.com/gorilla/websocket/blob/e8629af678b7fe13f35dff5e197de93b4148a909/conn.go#L965

as per the docs for NextReader() Once this method returns a non-nil error, all subsequent calls to this method return the same error.

which will return to us https://github.com/gorilla/websocket/blob/e8629af678b7fe13f35dff5e197de93b4148a909/json.go#L52

So we don't need it for addressing this issues.

In terms of other potential errors which we currently drop silently, I think we should discuss those in a seperate pr.

This bug is causing us grief in production so I would like to close it out as soon as possible.

threeaccents commented 3 years ago

Sounds good! Merged it in!

jdupl123 commented 3 years ago

Thanks!