winrm icon indicating copy to clipboard operation
winrm copied to clipboard

Avoid closing the Command cancel channel twice

Open kke opened this issue 3 years ago • 4 comments

Fixes #121

(possibly)

WDYT?

kke avatar Jul 29 '21 12:07 kke

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

kke avatar Dec 14 '22 09:12 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 avatar Nov 27 '23 07:11 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="".

kke avatar Dec 22 '23 09:12 kke

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 avatar Dec 22 '23 14:12 masterzen