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.

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

Conversation

kke
Copy link
Contributor

@kke kke commented Jul 29, 2021

Fixes #121

(possibly)

WDYT?

@kke
Copy link
Contributor Author

kke commented Dec 14, 2022

Still getting the close of a closed channel error quite often.

@kke
Copy link
Contributor Author

kke commented Nov 27, 2023

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 Run, RunWithInput etc are doing too though.

But looking at the Close(), it seems like its intention is to terminate a running process, shouldn't it be terminated already unless I'm using Close() to cancel a running process before it's finished?

Is the code in client.go sending unnecessary termination signals for every command?

@kke kke force-pushed the double-close-panic branch from 4b3ea15 to a087201 Compare December 22, 2023 08:00
@kke
Copy link
Contributor Author

kke commented Dec 22, 2023

🤔 command.check() is called in the beginning of functions like Close():

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 c.id="".

@masterzen
Copy link
Owner

but nothing seems the set c.id="".

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

close(c.cancel)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic: close of closed channel
2 participants