Ensure Close() waits for goroutines when called concurrently
#331
Conversation
k8s-ci-robot
added
kind/bug
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ash2k The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
| } | ||
| } | ||
| }) | ||
| return nil |
There was a problem hiding this comment.
I think it doesn't make sense to return an error for double close. Either we return the actual underlying errors or no error at all. Any reason to create "artificial" errors? I.e. nothing bad happened because of the second call, why return an error?
There was a problem hiding this comment.
An alternative implementation here would still synchronize the shutdown of goroutines, but call Close on listeners as many times as this Close is called and then return the real errors, if any. I.e. pass the duplicate call through.
| for connErr := range ml.connCh { | ||
| if connErr.conn != nil { | ||
| _ = connErr.conn.Close() | ||
| } |
There was a problem hiding this comment.
I don't see why this loop is necessary. Channel is not buffered and we've told the publishing goroutines to stop just above via close(ml.stopCh) - they will each close the connection they are trying to push, if any.
What type of PR is this?
/kind bug
What this PR does / why we need it:
See the discussion at #329 (comment).
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: