bodyclose
bodyclose copied to clipboard
timakin
False positive with body close inside separate function
I think that bodyclose produces false positive warning on this code: response body is closed in a separate function inside each logic case. I can not use defer response.Body.Close() due to infinite loop with ticker.
func isAppReady(
logger *zap.Logger,
shutdownChannel <-chan bool,
appHost string,
appHealthCheckURI string,
appCheckFrequency int,
) bool {
logger.Debug(
"[Consumer] check that App is available in the infinite loop with frequency",
zap.Int("appCheckFrequency", appCheckFrequency),
zap.String("app url", appHost+appHealthCheckURI),
)
ticker := time.NewTicker(time.Duration(appCheckFrequency) * time.Second)
for {
select {
case <-shutdownChannel:
logger.Info("[Consumer] got Shutdown signal, terminating app health check")
return false
case <-ticker.C:
response, err := http.Get(appHost + appHealthCheckURI)
if err != nil {
logger.Error("[Consumer] error after the app health check request", zap.Error(err))
closeResponse(response.Body, logger)
continue
}
if response.StatusCode == http.StatusOK {
logger.Info("[Consumer] got 200 OK: application started",
zap.String("app url", appHost+appHealthCheckURI),
zap.Int("response.StatusCode", response.StatusCode),
)
closeResponse(response.Body, logger)
return true
}
closeResponse(response.Body, logger)
logger.Debug(
"[Consumer] app is not ready yet to consume events, continue to wait",
zap.String("app url", appHost+appHealthCheckURI),
zap.Int("response.StatusCode", response.StatusCode),
)
}
}
}
func closeResponse(responseBody io.Closer, logger *zap.Logger) {
if err := responseBody.Close(); err != nil {
logger.Error("[Consumer] can not close the response")
}
}
@timakin Any feedback on the above? This pattern is quite common in many projects. In fact, the following code that is demonstrated in this links closes the body inside another function:
https://blog.golang.org/context
The tool would raise a flag against go func() { c <- f(http.DefaultClient.Do(req)) }() because the response body doesn't appear to be closed by f function but in fact is.
I have creative ways to work around this but I think this tool ignores certain practices I see from the community.
@renta Did you come up with a way around this? I have some creative ways, like in my example above, I would have the resp object return and I handle the close in the goroutine instead of the closure I use above.
@justinpage I've not found a proper way to fix this issue, so I've used //nolint:bodyclose before isAppReady function to suppress this check.
Another example:
resp, err := http.DefaultClient.Do(req) //nolint:bodyclose // linters bug
if err != nil {
return rc, err
}
defer func(Body io.ReadCloser) {
err := Body.Close()
if err != nil {
logger.GetLogger().Errorf("can not to close body: %s", err.Error())
}
}(resp.Body)
I think the same happened here. I opened https://github.com/ory/kratos/issues/1727 for the same reason, I think.
Any news, @timakin?
IntelliJ/Goland right now auto generates
defer func(Body io.ReadCloser) {
err := Body.Close()
if err != nil {
// here most obvious is to put log.Error(err) or similar
}
}(resp.Body)
and this linter doesn't accept that I think above is the most, yet verbose, solution as we don't ignore error.
this linter doesn't accept that
It works for me.
This issue is for another reason.
And chance in a config file you could allow us to provide a regexp for matching our closers? Using //nolint:bodyclose means it may miss catching other non-closed Body's, especially ones added later.
In my case I have a package called must and a package function called Close() that I call like so:
defer must.Close(resp.Body)
It would be nice if I could specify that anything matching "must\.Close\(\s*[a-zA-Z0-9_]+\.Body\s*\)" would satisfy body closure.
I have to wonder how the false positive / actual positive ratio is with this bug unresolved for over 2 years :thinking:
For some reason I get new warnings after upgrading to golangci-lint 1.52.0 from 1.50.1, is it expected to find more issues now?
I think the generic solution is adding the ignore rules, like if a response or body is passed to a defined in the rules function, then ignore that case
@timakin would you be open for somebody to contribute here? So making sure that a structure like shown by @arvenil is passing?
