masterzen / winrm

Command-line tool and library for Windows remote command execution in Go
Apache License 2.0
425 stars 129 forks source link

Avoid closing the Command cancel channel twice #122

Open kke opened 3 years ago

kke commented 3 years ago

Fixes #121

(possibly)

WDYT?

kke commented 1 year ago

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

kke commented 11 months ago

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 commented 11 months ago

🤔 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 commented 10 months ago

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.