pkg/ioutils: Make subsequent Close calls a no-op #46419
Conversation
vvoland
added
status/2-code-review
kind/bugfix
vvoland
force-pushed
the
pkg-pools-close-noop
branch
from
4a3bc3f to
d5bc8dc
Compare
vvoland
force-pushed
the
pkg-pools-close-noop
branch
from
d5bc8dc to
415a910
Compare
pkg/pools/pools.go
Outdated
| return ioutils.NewReadCloserWrapper(r, func() error { | ||
| if !atomic.CompareAndSwapInt32(&closed, 0, 1) { | ||
| subsequentCloseWarn("ReadCloserWrapper") | ||
| return nil |
There was a problem hiding this comment.
The behavior of Close after the first call is undefined.
Code which calls Close more than once on an arbitrary io.Closer is buggy by definition. Logging a warning is a great first step, especially for code which may discard the error return, but it really should also return an error to maximize fail-loudly-ness without actually panicking.
There was a problem hiding this comment.
In this case, I would tend to agree, at least with not logging, nobody will ever see this log message message because there generally wouldn't be a reason to correlate that with anything.
If someone does see it it will have very little meaning to them.
If it is an error maybe we'll see it show up in CI?
In most cases if there is a bug I would imagine it would be something like passing a closer to another function and both the outer and the inner have close in a defer.
Not technically correct since the outer function should be delegating the close to the inner function.
That can also get hairy, what if the inner function returns an error? It would be difficult to tell if the closer was closed or not.
Anyway, I see your point that this would be a bug that we should catch, but also possibly it would just as easily be a non-issue.
Not to mention in most places we don't check error on close.
|
@vvoland ptal |
vvoland
force-pushed
the
pkg-pools-close-noop
branch
from
415a910 to
75b40fd
Compare
vvoland
changed the title
vvoland
force-pushed
the
pkg-pools-close-noop
branch
from
75b40fd to
d2cbdc2
Compare
Turn subsequent `Close` calls into a no-op and produce a warning with an optional stack trace (if debug mode is enabled). Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
vvoland
force-pushed
the
pkg-pools-close-noop
branch
from
d2cbdc2 to
585d74b
Compare
thaJeztah
dismissed
cpuguy83’s stale review
change request was addressed #46419 (review)
vvoland
added
process/cherry-picked
and removed
process/cherry-pick/23.0
process/cherry-pick/24.0
process/cherry-pick/25.0
labels
Turn subsequent
Closecalls into a no-op and produce a warning with an optional stack trace (if debug mode is enabled).- What I did
Turned subsequent
Closecalls on on (Read/Write/cancel)CloserWrappers into a no-op.- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)