-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid closing the Command cancel channel twice #122
base: master
Are you sure you want to change the base?
Conversation
Still getting the close of a closed channel error quite often. |
Maybe I shouldn't be calling Close() at all. I've been doing command, err := shell.ExecuteWithContext(context.Background(), cmd)
if err != nil {
return fmt.Errorf("%w: execute command: %w", ErrCommandFailed, err)
}
...
command.Wait()
command.Close() That is what the client's But looking at the Is the code in |
Signed-off-by: Kimmo Lehto <[email protected]>
Signed-off-by: Kimmo Lehto <[email protected]>
4b3ea15
to
a087201
Compare
🤔 command.check() is called in the beginning of functions like func (c *Command) check() error {
if c.id == "" {
return errors.New("Command has already been closed")
}
if c.shell == nil {
return errors.New("Command has no associated shell")
}
if c.client == nil {
return errors.New("Command has no associated client")
}
return nil
} but nothing seems to set |
Yes that doesn't seem great :) Would you mind writing a test that exhibit the problem, because I fear we're going to have the same issue again and we might not catch the regression. |
default: | ||
close(c.cancel) | ||
} | ||
close(c.cancel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the select
here was made to prevent crashing when calling Close
twice.
I believe this should instead be protected by a sync.Once
instead.
Leaving the close(c.cancel)
as is, makes it at risk of double close and panic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sync.once around the close seems like a workaround without fixing the root cause, but it may be that a mutex is going to be needed somewhere, perhaps to guard access to c.id
.
close(c.cancel) | ||
|
||
id := c.id | ||
c.id = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed a command shouldn't be reused once closed. 👍
Fixes #121
(possibly)
WDYT?