timakin/bodyclose

Analyzer: checks whether HTTP response body is closed and a re-use of TCP connection is not blocked.

bodyclose

bodyclose is a static analysis tool which checks whether res.Body is correctly closed.

Install

You can get bodyclose by go get command.

$ go get -u github.com/timakin/bodyclose

How to use

bodyclose run with go vet as below when Go is 1.12 and higher.

$ go vet -vettool=$(which bodyclose) github.com/timakin/go_api/...
# github.com/timakin/go_api
internal/httpclient/httpclient.go:13:13: response body must be closed

When Go is lower than 1.12, just run bodyclose command with the package name (import path).

But it cannot accept some options such as --tags.

$ bodyclose github.com/timakin/go_api/...
~/go/src/github.com/timakin/api/internal/httpclient/httpclient.go:13:13: response body must be closed

Analyzer

bodyclose validates whether *net/http.Response of HTTP request calls method Body.Close() such as below code.

resp, err := http.Get("http://example.com/") // Wrong case
if err != nil {
	// handle error
}
body, err := ioutil.ReadAll(resp.Body)

This code is wrong. You must call resp.Body.Close when finished reading resp.Body.

resp, err := http.Get("http://example.com/")
if err != nil {
	// handle error
}
defer resp.Body.Close() // OK
body, err := ioutil.ReadAll(resp.Body)

In the GoDoc of Client.Do this rule is clearly described.

If you forget this sentence, a HTTP client cannot re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.

bodyclose's Issues

Close in another function's defer function is not handled correctly

Here is the sample code to reproduce:

package main

import (
        "net/http"
)

func closeByCall(res *http.Response) {
        defer res.Body.Close()
}

func closeByCallDefer(res *http.Response) {
        defer func() {
                res.Body.Close()
        }()
}

func responseCall() {
        res, _ := http.Get("https://example.com")
        closeByCall(res)
}

func responseCallDefer() {
        res, _ := http.Get("https://example.com")
        closeByCallDefer(res)
}

func main() {
        responseCall()
        responseCallDefer()
}

And the result with go vet:

go vet -vettool=$(which bodyclose) .
# mod
./main.go:23:20: response body must be closed

false positive on first class function

summary

Similar to #18, when the http.Response.Body.Close method is extracted and called, bodyclose misses the call and incorrectly warns:

resp, err := http.Get("http://example.com/")
if err != nil {
        // handle error
}
f := resp.Body.Close
defer f()
body, err = ioutil.ReadAll(resp.Body)

background

While this example is so simple as to be meaningless, the core problem comes up when trying to handle the potential error that io.Closer.Close can return:

func pick(one *error, two func() error) {
        err := two()
        if one != nil && *one == nil {
                *one = err
        }
}

func call() (err error) {
        resp, err := http.Get("http://example.com/")
        if err != nil {
                // handle error
        }
        defer pick(&err, resp.Body.Close)
        _, err = ioutil.ReadAll(resp.Body)
        if err != nil {
                // handle error
        }
        return nil
}

This is resolved if you inline/duplicate the pick method, defer func() {if close := resp.Body.Close(); err == nil { err = close } }(), since now there is a direct call to resp.Body.Close(), but the polymorphism and convenience of making a function for this is paramount.

It is also worth noting that defer func(f func() error) { f() }(resp.Body.Close) also fails.

Closing HTTP Response Body Gotcha

Have you come across this article - http://devs.cloudimmunity.com/gotchas-and-common-mistakes-in-go-golang/index.html#close_http_resp_body ? The code examples could be improved as per the suggestions in the article.

Generated code by ent cause check panic

> tree
.
├── ent
│   └── schema
│       └── hello.go
├── go.mod
├── go.sum
└── main.go

// main.go
package main

// hello.go
package schema

import (
	"entgo.io/ent"
	"entgo.io/ent/schema/field"
)

type Hello struct {
	ent.Schema
}

func (Hello) Fields() []ent.Field {
	return []ent.Field{
		field.String("content"),
	}
}

Generate Code

go run -mod=mod entgo.io/ent/cmd/ent generate --feature sql/upsert --target ./ent ./ent/schema

Run check

ERRO [linters_context/goanalysis] buildssa: panic during analysis: in main/ent.withHooks$1: cannot convert *t0 (M) to PM, goroutine 3115 [running]:
runtime/debug.Stack()
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/runtime/debug/stack.go:24 +0x64
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func1()
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_action.go:105 +0x4c
panic({0x105dad320, 0x14002769470})
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/runtime/panic.go:884 +0x204
golang.org/x/tools/go/ssa.emitConv(0x14000858d80, {0x105f92d00, 0x14002785500}, {0x105f8a1a0?, 0x14003416120})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/emit.go:286 +0xb4c
golang.org/x/tools/go/ssa.emitStore(0x14000858d80, {0x105f92d00, 0x14002785440}, {0x105f92d00, 0x14002785500}, 0x2b27dc8)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/emit.go:377 +0x58
golang.org/x/tools/go/ssa.(*address).store(0x140027437d0, 0x14000858d80?, {0x105f92d00?, 0x14002785500?})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/lvalue.go:40 +0x4c
golang.org/x/tools/go/ssa.(*storebuf).emit(...)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:533
golang.org/x/tools/go/ssa.(*builder).assignStmt(0x14000858d80?, 0x14000858d80, {0x1400099e8d0, 0x1, 0x105f26260?}, {0x1400099e900, 0x1, 0x104fad5a0?}, 0x0)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:1207 +0x378
golang.org/x/tools/go/ssa.(*builder).stmt(0x14003ebe838?, 0x14000858d80, {0x105f8d080?, 0x14002d92440?})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2181 +0x38c
golang.org/x/tools/go/ssa.(*builder).stmtList(0x140027435c0?, 0x0?, {0x14002d92480?, 0x4, 0x30?})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:946 +0x48
golang.org/x/tools/go/ssa.(*builder).stmt(0x14000858d80?, 0x14000858d80, {0x105f8d1a0?, 0x140016ea9f0?})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2277 +0x730
golang.org/x/tools/go/ssa.(*builder).buildFunctionBody(0x0?, 0x14000858d80)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2391 +0x368
golang.org/x/tools/go/ssa.(*builder).expr0(0x14003ebf9f8, 0x14003e90000, {0x105f8d440?, 0x1400099f300?}, {0x7, {0x105f8a0b0, 0x14000e1e240}, {0x0, 0x0}})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:656 +0x4ac
golang.org/x/tools/go/ssa.(*builder).expr(0x105e1c6c0?, 0x14003e90000, {0x105f8d440, 0x1400099f300})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:625 +0x11c
golang.org/x/tools/go/ssa.(*builder).expr0(0x14003ebf9f8, 0x14003e90000, {0x105f8d200?, 0x14002d92500?}, {0x7, {0x105f8a038, 0x14002118cb0}, {0x0, 0x0}})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:676 +0x688
golang.org/x/tools/go/ssa.(*builder).expr(0x105f18480?, 0x14003e90000, {0x105f8d200, 0x14002d92500})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:625 +0x11c
golang.org/x/tools/go/ssa.(*builder).assign(0x14003e90000?, 0x14003e90000?, {0x105f8fba8?, 0x14002743530}, {0x105f8d200?, 0x14002d92500?}, 0x8?, 0x0)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:598 +0x314
golang.org/x/tools/go/ssa.(*builder).localValueSpec(0x14003e90000?, 0x14003e90000, 0x140037b7e50)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:1147 +0xb8
golang.org/x/tools/go/ssa.(*builder).stmt(0x14003ebf548?, 0x14003e90000, {0x105f8d2f0?, 0x1400099f340?})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2147 +0x1584
golang.org/x/tools/go/ssa.(*builder).stmtList(0x14003ebf5c8?, 0x104fea000?, {0x14003168980?, 0x8, 0x10?})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:946 +0x48
golang.org/x/tools/go/ssa.(*builder).stmt(0x14003e90000?, 0x14003e90000, {0x105f8d1a0?, 0x140016eac30?})
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2277 +0x730
golang.org/x/tools/go/ssa.(*builder).buildFunctionBody(0x14003e80d80?, 0x14003e90000)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2391 +0x368
golang.org/x/tools/go/ssa.(*builder).buildFunction(0x1054f6560?, 0x14003e90000)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2326 +0x30
golang.org/x/tools/go/ssa.(*builder).buildCreated(0x14003ebf9f8)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2413 +0x28
golang.org/x/tools/go/ssa.(*Package).build(0x14001d68c80)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2606 +0xae0
sync.(*Once).doSlow(0x140004b6180?, 0x140037fb9f0?)
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/sync/once.go:74 +0x104
sync.(*Once).Do(...)
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/sync/once.go:65
golang.org/x/tools/go/ssa.(*Package).Build(...)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2477
golang.org/x/tools/go/analysis/passes/buildssa.run(0x140032fe780)
	/Users/go/pkg/mod/golang.org/x/[email protected]/go/analysis/passes/buildssa/buildssa.go:72 +0x13c
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyze(0x140010be3f0)
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_action.go:195 +0x990
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func2()
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_action.go:113 +0x20
github.com/golangci/golangci-lint/pkg/timeutils.(*Stopwatch).TrackStage(0x140011b4050, {0x105a94d1e, 0x8}, 0x14001498730)
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/timeutils/stopwatch.go:111 +0x44
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe(0x1054b8170?)
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_action.go:112 +0x74
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze.func2(0x140010be3f0)
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_loadingpackage.go:80 +0xb0
created by github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_loadingpackage.go:75 +0x17c
ERRO [runner] Panic: bodyclose: package "ent" (isInitialPkg: true, needAnalyzeSource: true): interface conversion: interface {} is nil, not *buildssa.SSA: goroutine 3110 [running]:
runtime/debug.Stack()
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/runtime/debug/stack.go:24 +0x64
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func1()
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_action.go:109 +0x238
panic({0x105e3ef00, 0x14001830480})
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/runtime/panic.go:884 +0x204
github.com/timakin/bodyclose/passes/bodyclose.runner.run({0x140032fe870, {0x0, 0x0}, 0x0, {0x0, 0x0}, 0x0, 0x0}, 0x140032fe870)
	/Users/go/pkg/mod/github.com/timakin/[email protected]/passes/bodyclose/bodyclose.go:45 +0x5d0
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyze(0x140010be360)
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_action.go:195 +0x990
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func2()
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_action.go:113 +0x20
github.com/golangci/golangci-lint/pkg/timeutils.(*Stopwatch).TrackStage(0x140011b4050, {0x105aa3ad6, 0x9}, 0x14000ba3730)
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/timeutils/stopwatch.go:111 +0x44
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe(0x1054b8170?)
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_action.go:112 +0x74
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze.func2(0x140010be360)
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_loadingpackage.go:80 +0xb0
created by github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze
	/Users/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/goanalysis/runner_loadingpackage.go:75 +0x17c
WARN [runner] Can't run linter goanalysis_metalinter: goanalysis_metalinter: bodyclose: package "ent" (isInitialPkg: true, needAnalyzeSource: true): interface conversion: interface {} is nil, not *buildssa.SSA
ERRO Running error: 1 error occurred:
	* can't run linter goanalysis_metalinter: goanalysis_metalinter: bodyclose: package "ent" (isInitialPkg: true, needAnalyzeSource: true): interface conversion: interface {} is nil, not *buildssa.SSA

false positive when close is in another package

The following code triggers a false positive.

package util

import (
	"io"
	"log"
)

func Close(c io.Closer) {
	if err := c.Close(); err != nil {
		log.Printf("error closing io: %w", err)
	}
}

package main

import (
	"net/http"
	"util"
)

func main() {
	res, _ := http.Get("http://example.com/")
	defer util.Close(res.Body)
}

bodyclose fails unexpected

bodyclose will fails with following code:

        resp, err := client.Do(req)
	if err != nil {
		return
	}
	// lint will fail
	defer func(b io.ReadCloser) {
		_ = b.Close()
	}(resp.Body)

but it's ok with:

        resp, err := client.Do(req)
	if err != nil {
		return
	}
	// lint will fail
	 defer resp.Body.Close()

False positive with a retry for

The following code is fine, because it closes the body only when the error is nil, but gets response body must be closed errors on both the d.client.Do(req) lines.

func (d *Dispatcher) call(req *http.Request) {
	// keep retrying the request until the error is nil
	tries := 0
	wait := time.Second
	resp, err := d.client.Do(req)
	for err != nil && tries < d.retries {
		// wait a while
		time.Sleep(wait)
		wait *= 2
		// try again
		resp, err = d.client.Do(req)
		tries++
	}

	// maximum retries exceeded
	if err != nil {
		// log error here
		return
	}

	// now the request was successful; do some work

	// close the body now
	if err = resp.Body.Close(); err != nil {
		// log error
	}
}

normalize CLI

Can we please get a more typical linter command line interface?

Most Go linters don't have to be wrapped in which (breaking native Windows support) or $(...) (breaking csh support) or go vet. It's a lot of extraneous, fragile, nonportable typing compared to a more conventional bodyclose <directory> interface.

Is this project still maintained? I want to add some new features to pr

False negatives with io.Closer

Hello, the following code is good, but bodyclose finds it bad:

$ git diff
diff --git a/passes/bodyclose/testdata/src/a/a.go b/passes/bodyclose/testdata/src/a/a.go
index dcf9448..da0c7cd 100644
--- a/passes/bodyclose/testdata/src/a/a.go
+++ b/passes/bodyclose/testdata/src/a/a.go
@@ -2,6 +2,7 @@ package a
 
 import (
        "fmt"
+       "io"
        "net/http"
 )
 
@@ -83,6 +84,14 @@ func f7() {
        resCloser()
 }
 
+func f7c() {
+       res, _ := http.Get("http://example.com/") // OK
+       resCloser := func(c io.Closer) {
+               c.Close()
+       }
+       resCloser(res.Body)
+}
+
 func f8() {
        res, _ := http.Get("http://example.com/") // want "response body must be closed"
        _ = func() {

$ GO111MODULE=on go test -v github.com/timakin/bodyclose/passes/bodyclose
=== RUN   Test
--- FAIL: Test (1.51s)
    analysistest.go:251: a/a.go:88:20: unexpected diagnostic: response body must be closed
FAIL
FAIL    github.com/timakin/bodyclose/passes/bodyclose   1.529s

false positive: if statement confuses linter

This program

package main

import (
	"net/http"
)

func main() {
	var resp *http.Response

	var fizz bool
	if fizz {
		resp, _ = foo()
	} else {
		resp, _ = bar()
	}
	defer resp.Body.Close()

	_ = resp
}

func foo() (*http.Response, error) {
	return nil, nil
}

func bar() (*http.Response, error) {
	return nil, nil
}

results in this false positive

main.go:12:16: bodyclose: response body must be closed (bodyclose)
                resp, _ = foo()
                             ^
main.go:14:16: bodyclose: response body must be closed (bodyclose)
                resp, _ = bar()
                             ^

Returning *http.Response to another function is not handled correctly

Here is the sample code to reproduce:

package main

import (
	"net/http"
)

func getResponse(url string) *http.Response {
	res, _ := http.Get(url)
	return res
}

func main() {
	resp := getResponse("https://example.com")
	resp.Body.Close()
}

And the result of go vet:

go vet -vettool=$(which bodyclose) ./main.go
# command-line-arguments
./closer.go:8:20: response body must be closed
./closer.go:13:21: response body must be closed

Bump golang.org/x/tools to v0.7.0 (as minimum) and make new tag

Hi 🙂

Please, bump golang.org/x/tools to v0.7.0 (as minimum) and make new tag in your repo.

It's important to be consistent with new Go 1.20 golangci-lint.
Related to golangci/golangci-lint#3086

Thanks!

P.S. Optionally I recommend

• to move linter to 1.20 totally (go.mod, CI, etc.)
• add support of dependabot

Nil pointer deref in runner.run

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x50 pc=0x9a694f]

goroutine 18969 [running]:
github.com/timakin/bodyclose/passes/bodyclose.(*runner).run(0xc0000c6d40, 0xc150d96f00, 0x10, 0xd4f8e0, 0xdf189b62edaedd01, 0xc2d16e0ac0)
	/go/src/github.com/foo/bar/vendor/github.com/timakin/bodyclose/passes/bodyclose/bodyclose.go:57 +0x1af
github.com/golangci/golangci-lint/pkg/golinters/goanalysis/checker.(*action).execOnce(0xc06bf7d540)
	/go/src/github.com/foo/bar/vendor/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/checker/checker.go:382 +0x68a
sync.(*Once).Do(0xc06bf7d540, 0xc00189e790)
	/usr/local/go/src/sync/once.go:44 +0xb3
github.com/golangci/golangci-lint/pkg/golinters/goanalysis/checker.(*action).exec(0xc06bf7d540)
	/go/src/github.com/foo/bar/vendor/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/checker/checker.go:303 +0x50
github.com/golangci/golangci-lint/pkg/golinters/goanalysis/checker.execAll.func1(0xc06bf7d540)
	/go/src/github.com/foo/bar/vendor/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/checker/checker.go:291 +0x34
created by github.com/golangci/golangci-lint/pkg/golinters/goanalysis/checker.execAll
	/go/src/github.com/foo/bar/vendor/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/checker/checker.go:297 +0x11b

I encountered this when running bodyclose through golangci-lint. The stack trace seems to point the finger at this line. AFAICT r.resObj.Type() must be returning nil.

Unfortunately, the code that triggered this is private so I can't share it with you 🙁

golangci-lint v1.17.1
go version 1.12.6

The following program is not handled correctly

package main

import (
	"io"
	"net/http"
)

func closeBody(c io.Closer) {
	_ = c.Close()
}

func main() {
	resp, _ := http.Get("https://example.com")
	defer closeBody(resp.Body)
}

False Positive when http.Response from a function

The following code triggers a false positive.

package main

import (
	"net/http"
	"tmp"
)

type MyResp struct {
	resp *http.Response
}

func (r *MyResp) Response() *http.Response {
	return r.resp
}

func test() {
	tmp := &MyResp{}
	var err error
	tmp.resp, err = http.Get("http://example.com/")
	if err != nil {
		fmt.Println(err)
	}
	defer tmp.Response().Body.Close()
	body, _ := ioutil.ReadAll(tmp.Response().Body)
	fmt.Println(body)
}

func main() {
        test()
}

Use semver

Please use semantic versioning with tags, like this:

$ git tag v0.0.1
$ git push origin v0.0.1
# ...
 * [new tag]         v0.0.1 -> v0.0.1

So we can pin to specific version instead of using pseudo-versions :)
Note that it is OK to start with v0.0.X versions, because bumping major versions v1 to v2 will require imports update.

Thank you.

Cross package functions are not handled correctly

example:

package p

import "io"

// CloseSilently ...
func CloseSilently(c io.Closer) {
	_ = c.Close()
}

package main

import (
	"net/http"

	"p"
)

func main() {
	resp, _ := http.Get("https://example.com")
	defer p.CloseSilently(resp.Body)
}

Check both closing and consuming the response body

In the GoDoc of http.Response this rule is clearly described.

It is the caller's responsibility to close Body. The default HTTP client's Transport may not reuse HTTP/1.x "keep-alive" TCP connections if the Body is not read to completion and closed.

This is what reuseconn fork tries to address by verifying that the response body is consumed as well. It would be great if it was possible to incorporate such linting behaviour in bodyclose.

False Positives on Methods/Functions That Return *http.Response

For functions and methods that return a *http.Response and handle the closing of the response body by the function/method caller, this tool flags the response inside the function/method as needing to be closed, when it is being closed by the caller.

False positive when using `httptest.NewRecorder()`

This code triggers a false positive:

func issues() {
	w := httptest.NewRecorder()
	resp := w.Result()
	defer func() {
		_ = resp.Body.Close()
	}()
	_, _ = io.ReadAll(resp.Body)
}

what zbout joining revive?

revive is so hot now https://github.com/mgechev/revive

what about beinig a part of it?

Analyzer for http.Request

The current version of bodyclose only run analysis on http.Response. It's quite common that people use the same behaviour on http.Request (on server side) which is not really necessary. According to the documentation for http.Request, the Body field has the following comment:

// For server requests, the Request Body is always non-nil
// but will return EOF immediately when no body is present.
// The Server will close the request body. The ServeHTTP
// Handler does not need to.
Body io.ReadCloser

^{(Also confirmed by this SO post)}

The reason to close the http.Response body can be found in the documentation for http.Client.Do:

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close. If the Body is not both read to EOF and closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.

And documentation for http.Response Body field:

// The http Client and Transport guarantee that Body is always
// non-nil, even on responses without a body or responses with
// a zero-length body. It is the caller's responsibility to
// close Body. The default HTTP client's Transport may not
// reuse HTTP/1.x "keep-alive" TCP connections if the Body is
// not read to completion and closed.

Since that does not apply to the server side, this means that code like this is not needed:

func handle(w http.ResponseWriter, r *http.Request) {
    body, err := ioutl.ReadAll(r.Body)
    if err != nil {
        panic(err)
    }
    r.Body.Close() // <- Not needed
}

It would be nice if the linter was telling you to remove the line closing the http.Request body. If you agree, are you open to pull requests?

global resp cause check panic

var resp *http.Response

func testGlobal() {
	resp, _ = http.Get("https://example.com")
	resp.Body.Close()

}

It cause panic when check this code.

ERRO [runner] Panic: bodyclose: package "main" (isInitialPkg: true, needAnalyzeSource: true): runtime error: invalid memory address or nil pointer dereference: goroutine 4224 [running]:
runtime/debug.Stack()
	runtime/debug/stack.go:24 +0x65
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func1()
	github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:101 +0x155
panic({0x4aeda60, 0x557d2c0})
	runtime/panic.go:1038 +0x215
github.com/timakin/bodyclose/passes/bodyclose.(*runner).isopen(0xc003c2dd08, 0xc01d94f770, 0x4)
	github.com/timakin/[email protected]/passes/bodyclose/bodyclose.go:135 +0x18d
github.com/timakin/bodyclose/passes/bodyclose.runner.run({0xc014270f70, {0x4e5caf8, 0xc0024143c0}, 0xc01d94f770, {0x4e5cb98, 0xc0024145f0}, 0xc000da2640, 0xc01b888a20}, 0xc014270f70)
	github.com/timakin/[email protected]/passes/bodyclose/bodyclose.go:102 +0x5c5
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyze(0xc0003b4300)
	github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:187 +0x9c4
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func2()
	github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:105 +0x1d
github.com/golangci/golangci-lint/pkg/timeutils.(*Stopwatch).TrackStage(0xc001a0e8c0, {0x4c2232d, 0x9}, 0xc002c0d760)
	github.com/golangci/golangci-lint/pkg/timeutils/stopwatch.go:111 +0x4a
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe(0xc0003b4300)
	github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:104 +0x85
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze.func2(0x0)
	github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_loadingpackage.go:80 +0x67
created by github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze
	github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_loadingpackage.go:75 +0x1fd

panic: runtime error: invalid memory address or nil pointer dereference

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x11f5d44]

goroutine 49 [running]:
go/types.(*object).Name(...)
	/usr/local/Cellar/go/1.12.1/libexec/src/go/types/object.go:134
github.com/timakin/bodyclose/passes/bodyclose.(*runner).isCloseCall(0xc00004a380, 0x13fd500, 0xc0002eda40, 0xc0003045a0)
	/Users/dcuadrado/Projects/GoCode/src/github.com/timakin/bodyclose/passes/bodyclose/bodyclose.go:226 +0x294
github.com/timakin/bodyclose/passes/bodyclose.(*runner).isopen(0xc00004a380, 0xc000302790, 0x0, 0xc00033c4e0)
	/Users/dcuadrado/Projects/GoCode/src/github.com/timakin/bodyclose/passes/bodyclose/bodyclose.go:175 +0x2e9
github.com/timakin/bodyclose/passes/bodyclose.(*runner).run(0xc00004a380, 0xc0000a85a0, 0x10a5c26, 0x5ca8c59a, 0x50dd49c16cc1328, 0x74254feed292)
	/Users/dcuadrado/Projects/GoCode/src/github.com/timakin/bodyclose/passes/bodyclose/bodyclose.go:100 +0x641
golang.org/x/tools/go/analysis/unitchecker.run.func5.1()
	/Users/dcuadrado/Projects/GoCode/src/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go:342 +0x6c1
sync.(*Once).Do(0xc0002b93b0, 0xc000035728)
	/usr/local/Cellar/go/1.12.1/libexec/src/sync/once.go:44 +0xb3
golang.org/x/tools/go/analysis/unitchecker.run.func5(0x1630500, 0x0)
	/Users/dcuadrado/Projects/GoCode/src/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go:302 +0x195
golang.org/x/tools/go/analysis/unitchecker.run.func6.1(0xc00000fe70, 0xc00040ed70, 0x1630500)
	/Users/dcuadrado/Projects/GoCode/src/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go:354 +0x33
created by golang.org/x/tools/go/analysis/unitchecker.run.func6
	/Users/dcuadrado/Projects/GoCode/src/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go:353 +0xa3

SSA and generics (go1.18)

Currently, SSA is not working with generics.

So your linter produces a panic when it is used with generics.

There is an issue open about that in the Go repository: golang/go#48525

Inside golangci-lint, we have disabled your linters: golangci/golangci-lint#2649

You have 2 solutions:

• waiting for a version of SSA that will support generics
• dropping the SSA analyzers and using something else to analyze the code.

Related to golang/go#50558

custom interfaces are not handled

package main

import (
	"net/http"
)

type myCloser interface {
	Close() error
}

func closeBody(c myCloser) {
	_ = c.Close()
}

func main() {
	resp, _ := http.Get("https://example.com")
	defer closeBody(resp.Body)
}

False positive when using `http.NewResponseController(rw)`

Simply calling http.NewResponseController(rw) for example in an HTTP server (to then call .Flush() on it) causes a linter warning.

Improve bodyclose when no defer

Hello,

bodyclose doesn't detect body not closed when we don't use defer.

With this two case:

First one is not problematic (because we can easily use defer res.Body.Close():

func TestBodyClose(_ *testing.T) {
	res, err := http.Get("https://github.com/timakin/bodyclose/")
	if err != nil {
		return
	}

	if res.StatusCode != http.StatusOK {
		// here bodyclose must detect this branch
		return
	}

	res.Body.Close()
}

The second one is more problematic from my point of view, because HTTP request is done in loop (for example for managing retries) and we can't easily use defer (without use of anonymous function):

func TestLoopBodyClose(_ *testing.T) {
	for i := 0; i < 3; i++ {
		res, err := http.Get("https://github.com/timakin/bodyclose/")
		if err != nil {
			continue
		}

		if res.StatusCode != http.StatusOK {
			// here bodyclose must detect this branch
			continue
		}

		res.Body.Close()
		break
	}
}

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")
	}
}

bodyclose: invoking "go tool vet" directly is unsupported; use "go vet"

after running:

$ bodyclose ./...

False positive: if function returns io.ReadCloser

Example:

func download(url string) (io.ReadCloser, error) { // body should be closed by User
	r, err := http.DefaultClient.Get(url)
	if err != nil {
		return nil, err
	}
	return r.Body, nil
}

Add debug mode

To make development and adding new features easier

Request: tag a version

This helps engineers to keep CI/CD pipelines stable in the face of breaking changes.

While technically, go mod supports using raw git commit ID's, doing so creates even more maintenance problems.

conflicts with errcheck?

resp.Body.Close() return error, so

defer resp.Body.Close()

seems to violate the errcheck rule?

errcheck: https://github.com/kisielk/errcheck