interp: Cancelling `Runner.Run`'s context doesn't abort `read` builtin

[theclapp]

See title.

Is there some other way to signal a Runner (something analogous to SIGINT) that I'm missing?

[mvdan]

The problem is: how do you abort a read? It does not take a context: https://github.com/mvdan/sh/blob/43a160083ea11d3d02f66c6ac748ae4c64ab4bc3/interp/builtin.go#L828

I'm fully on board with fixing this, because I agree that it's a bug that cancelling the interpreter doesn't stop the read builtin, but I'm not sure how to fix it.

[mvdan]

One possible solution would be to have the interpreter constantly read from stdin in a background goroutine, and use a channel for builtins like read. We'd just have to worry about:

1) buffering stdin bytes until they are used 2) forwarding stdin to a command when it's executed

[mvdan]

Thinking about this idea some more: this does mean that the interpreter will aggressively read before it technically needs to, but that might be a reasonable tradeoff to prevent reads from blocking potentially forever, until stdin or the entire program are closed.

[theclapp]

Yeah, after some Googling the best I can find so far is https://benjamincongdon.me/blog/2020/04/23/Cancelable-Reads-in-Go/ , which presents a CancelableReader class. It basically implements what you described above.

FWIW I don't think the interpreter should fork a reader goroutine until and unless somebody actually calls read.

[mvdan]

FWIW I don't think the interpreter should fork a reader goroutine until and unless somebody actually calls read.

That's fair, though I guess "we start reading early once runner.Run is first called" and "we start reading early once the first read is triggered" are both reasonable to me. I guess the former might be less inconsistent or surprising, but the latter is better in terms of not spawning the goroutine and doing work that might not be used at all - like when interpreting a small script that does not use stdin at all.

[theclapp]

Building on the code from the blog I linked, this seems like it'd work. (Lightly tested in the playground.)

package ctxreader

import (
    "context"
    "io"
    "sync"
)

type CancelableReader struct {
    ctx  context.Context
    data chan []byte
    err  error
    r    io.Reader
    once sync.Once
}

func (c *CancelableReader) begin() {
    buf := make([]byte, 1024)
    for c.ctx.Err() == nil {
        n, err := c.r.Read(buf)
        if n > 0 {
            tmp := make([]byte, n)
            copy(tmp, buf[:n])
            select {
            case c.data <- tmp:
            case <-c.ctx.Done():
                return
            }
        }
        if err != nil {
            c.err = err
            close(c.data)
            return
        }
    }
}

func (c *CancelableReader) Read(p []byte) (int, error) {
    c.once.Do(func() { go c.begin() })
    select {
    case <-c.ctx.Done():
        return 0, c.ctx.Err()
    case d, ok := <-c.data:
        if !ok {
            return 0, c.err
        }
        copy(p, d)
        return len(d), nil
    }
}

func New(ctx context.Context, r io.Reader) *CancelableReader {
    c := &CancelableReader{
        r:    r,
        ctx:  ctx,
        data: make(chan []byte),
    }
    return c
}

[mvdan]

That sounds about right. If you want to send a PR, we can continue there. Otherwise we can leave it up for grabs for a bit.

[theclapp]

I'll see if I can look at that in the next few days. Thanks!

[theclapp]

I suppose I really should extend it to writes, too (like echo).

[mvdan]

Maybe? Unless we can think of a practical example where it would matter, I think we can probably omit writes for now. Reads are clearly more necessary because the user might be typing into stdin, or choose not to. I don't think blocking writes will be common.

[theclapp]

See https://github.com/mvdan/sh/pull/857.

TestRunnerRun tests 697 - 703 are failing for me. But on the other hand, they're failing in my copy of mvdan/sh@master, too, so it doesn't seem to be this new code.