search

CaliDog / certstream-go

Go library for connecting to CertStream
MIT License
140 stars 33 forks source link

Break on error in ReadJSON #7

Closed VegarLH closed 6 years ago

VegarLH commented 6 years ago

Break on error in ReadJSON instead of continuing and panicing on NewQuery later on. Fixes panic on connection loss. Fixes issue #6 for me.

Fitblip commented 6 years ago

Hey thanks for the PR!

What do you think about also adding a break here:

https://github.com/tcpepidemic/certstream-go/blob/8cc628cade1d2d4a845b43902aceed695c916789/certstream.go#L40

That way if any errors are encountered it just loops back and re-connects.

It's also probably worth adding c.Close() outside the nested for loop to ensure that we're not leaking resources and connections are being cleaned up properly I think.

https://github.com/tcpepidemic/certstream-go/blob/8cc628cade1d2d4a845b43902aceed695c916789/certstream.go#L48

VegarLH commented 6 years ago

Hi, glad to help. wrt the secound break i think maybe using continue would be better since nil there would mean just an error finding message_type. Since ReadJSON already succeeded it would only happen due to malformed json i presume. So reconnecting would not be needed. Maybe changing the error to errStream <- errors.Wrap(err, "Error malformed json input!") and adding a continue after it would make such errors more obvious. Regarding the c.Close() you already have a defer c.Close() on line 25 which i think should be sufficient?

VegarLH commented 6 years ago

I've added the suggestion to the branch now. I'm not very used to git but apparently the pull request got updated automatically so you can see the suggestion there :)

Fitblip commented 6 years ago

Ah yes, I think you're right WRT to the continue thing. That way it wouldn't kill an entire batch with one bad piece of data - good idea.

For the c.Close() thing, if you look at the code if it breaks out of the internal infinite loop (https://github.com/tcpepidemic/certstream-go/blob/8cc628cade1d2d4a845b43902aceed695c916789/certstream.go#L28), then it'll basically re-connect with a new connection, re-assigning the connection to the variable c. This wouldn't necessarily free that resource properly because the defer would only get run on termination of the go-routine (which, in theory, can't happen without everything shutting down due to the nested infinite for loops).

Fitblip commented 6 years ago

Perfect, looks good to me! :)