Do not double close reader #29354
Do not double close reader #29354
Conversation
KN4CK3R
added
type/bug
backport/v1.21
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/S
modules/git/blob_nogogit.go
Outdated
| if b.rd == nil { | ||
| return fs.ErrClosed | ||
| } |
There was a problem hiding this comment.
Should it panic here instead? Calling Close multiple times is a coding error. I checked all calls to DataAsync and could not find another double close. A panic could prevent double closes in future while returning a (most of time) ignored error could hide it.
There was a problem hiding this comment.
No. None of Golang's other "Close" methods panics.
There was a problem hiding this comment.
Could it simply return "nil"? Most (all) errors for a Close method are meanless .... no caller cares about it.
There was a problem hiding this comment.
I think double Close should be supported. i.e. It's safe to invoke file.Close twice. So I think we should also allow invoking Close twice. It's convenient for some situations.
There was a problem hiding this comment.
Should we then add this rule to our coding guidelines?
If a type implements
io.Closer, callingClosemultiple times must not fail and return anilerror.
There was a problem hiding this comment.
I guess it depends, for most cases: yes, nil is good enough.
But I guess there could be some special cases that the caller might be interested in the result.
There was a problem hiding this comment.
Then maybe relax the nil but specify that calling Close multiple times must not be undefined behaviour and must not panic?
GiteaBot
added
lgtm/need 1
GiteaBot
added
lgtm/done
wxiaoguang
added
the
reviewed/wait-merge
Fixes go-gitea#29346 --------- Co-authored-by: wxiaoguang <[email protected]>
GiteaBot
added
backport/done
Backport #29354 by @KN4CK3R Fixes #29346 Co-authored-by: KN4CK3R <[email protected]> Co-authored-by: wxiaoguang <[email protected]>
Fixes #29346