timakin / bodyclose Goto Github PK
View Code? Open in Web Editor NEWAnalyzer: checks whether HTTP response body is closed and a re-use of TCP connection is not blocked.
License: MIT License
Analyzer: checks whether HTTP response body is closed and a re-use of TCP connection is not blocked.
License: MIT License
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)
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.
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.
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)
}
resp.Body.Close()
return error, so
defer resp.Body.Close()
seems to violate the errcheck rule?
errcheck: https://github.com/kisielk/errcheck
> 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
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
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)
}
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
To make development and adding new features easier
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?
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
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
}
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
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:
Related to golang/go#50558
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.
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
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.
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 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()
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()
^
Consider the program:
package main
import (
"fmt"
"io"
"net/http"
)
func main() {
req, err := http.NewRequest(http.MethodGet, "https://example.com", nil)
if err != nil {
panic(err)
}
resp, err := http.DefaultClient.Do(req)
if err != nil {
panic(err)
}
if resp.StatusCode >= 400 {
defer resp.Body.Close()
errResponse, err := io.ReadAll(resp.Body)
if err != nil {
panic(err)
}
fmt.Println("do something with error reponse", string(errResponse))
}
}
bodyclose is happy with the above program and won't show any errors. But in reality, the above program will cause a resource leak as resp.Body.Close()
is not being called in places outside the if
condition.
It is safe to generalize that, whenever a branch occurs in the code like an if
or switch
, then bodyclose
could look at all the branches to check if the resp.Body.Close()
is called.
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()
}
Simply calling http.NewResponseController(rw)
for example in an HTTP server (to then call .Flush()
on it) causes a linter warning.
after running:
$ bodyclose ./...
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
}
}
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
.
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
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
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.
This code triggers a false positive:
func issues() {
w := httptest.NewRecorder()
resp := w.Result()
defer func() {
_ = resp.Body.Close()
}()
_, _ = io.ReadAll(resp.Body)
}
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.
revive is so hot now https://github.com/mgechev/revive
what about beinig a part of it?
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)
}
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)
}
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
}
}
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.