Avoid closing the Command cancel channel twice

[kke]

Fixes #121

(possibly)

WDYT?

[kke]

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

[kke]

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]

🤔 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]

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.

[masterzen]

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.

[kke]

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.

[masterzen]

Indeed a command shouldn't be reused once closed. 👍