Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Avoid closing the Command cancel channel twice #122

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62c321b
Avoid closing the Command cancel channel twice
kke Jul 29, 2021
a087201
Clear c.id on Close to avoid closing twice
kke Dec 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,16 +117,16 @@ func (c *Command) Close() error {
return err
}

select { // close cancel channel if it's still open
case <-c.cancel:
default:
close(c.cancel)
}
close(c.cancel)
Copy link
Owner

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.

Copy link
Contributor Author

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.


id := c.id
c.id = ""
Copy link
Owner

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. 👍


request := NewSignalRequest(c.client.url, c.shell.id, c.id, &c.client.Parameters)
request := NewSignalRequest(c.client.url, c.shell.id, id, &c.client.Parameters)
defer request.Free()

_, err := c.client.sendRequest(request)

return err
}

Expand Down