masterzen / winrm

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

panic: close of closed channel #121

Open kke opened 3 years ago

kke commented 3 years ago
goroutine 1709 [running]:
github.com/masterzen/winrm.(*Command).Close(0xc00017e700, 0x0, 0x0)
    github.com/masterzen/winrm@v0.0.0-20201030141608-56ca5c5f2380/command.go:114

https://github.com/masterzen/winrm/blob/master/command.go#L111-L115

    select { // close cancel channel if it's still open
    case <-c.cancel:
    default:
        close(c.cancel)
    }
kke commented 3 years ago

@masterzen I made a PR that attempts to fix this via sync.Once in #122 - PTAL.

masterzen commented 3 years ago

@kke , do you by any chance have client code or a unit test that could reproduce the issue so that I understand how the double close was triggered?

kke commented 3 years ago

It goes something like this:

    shell, err := c.client.CreateShell()
    if err != nil {
        return err
    }
    defer shell.Close()

    command, err := shell.Execute(cmd)
    if err != nil {
        return err
    }

    var wg sync.WaitGroup

    wg.Add(1)
    go func() {
        defer wg.Done()
        defer command.Stdin.Close()
        command.Stdin.Write([]byte(stdin))
    }()

    wg.Add(1)
    go func() {
        defer wg.Done()
        defer command.Stdout.Close()
        outputScanner := bufio.NewScanner(command.Stdout)

        for outputScanner.Scan() {
            fmt.Print(outputScanner.Text()+"\n", "")
        }
    }()

    command.Wait()
    wg.Wait()
    command.Close()

It seems to occur in situations where a command is periodically retried.

Anyway, to me it seems the select statement does not always do what it is supposed to. If it manages to read from the cancel channel, the channel remains open. Also I'm not sure if it's guaranteed to always hit that branch first.

Also I don't know if it should even proceed to send the signal to endpoint if the channel is already closed.