-
Notifications
You must be signed in to change notification settings - Fork 18
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
harden ethmonitor for block header streaming errors #118
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice work here @marino39
// We default to polling if streaming is not enabled | ||
m.log.Info("ethmonitor: starting poll head listener") | ||
|
||
retryStreamingTimer := time.NewTimer(m.options.StreamingRetryAfter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defer retryStreamingTimer.Stop()
goto reconnect | ||
} | ||
|
||
blockTimer := time.NewTimer(3 * m.options.ExpectedBlockInterval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: defer blockTimer.Stop()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource leak, agrh... defer will not help as it never goes out of function scope. I will get that fixed asap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#119 -- hows this..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it is good. The timer will close itself after it fires and will be garbage collected. So there is no need to close it. It will also not cause process to hang. There will be at most a couple of them.
// After waits for the duration to elapse and then sends the current time
// on the returned channel.
// It is equivalent to NewTimer(d).C.
// The underlying Timer is not recovered by the garbage collector
// until the timer fires. If efficiency is a concern, use NewTimer
// instead and call Timer.Stop if the timer is no longer needed.
func After(d Duration) <-chan Time {
return NewTimer(d).C
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“a little temporary timer leak” is fine imho
it’d probably be better to close it, though, to be accurate
(a leaked timeout might cause an unnecessary OS interrupt)
The change can be tested by setting up local nodegateway, pointing watch cmd at it and stoping/restarting node gateway service.