Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

interp: make read cancellable via Run's ctx #857

Closed
wants to merge 8 commits into from

Conversation

theclapp
Copy link
Collaborator

@theclapp theclapp commented May 5, 2022

  • Add a CancelableReader type, and wrap stdin with it at the top of Runner.Run.
  • Also include CancelableReaderTTY which preserves stdin looking like a tty, i.e. [[ -t 0 ]] is still true.

@theclapp
Copy link
Collaborator Author

theclapp commented May 5, 2022

So it turns out that using this CancelableReader triggers these two behaviors of os.Exec:

  1. from os.Exec, Cmd.Stdin, the "otherwise" clause:
    // Stdin specifies the process's standard input.
    //
    // If Stdin is nil, the process reads from the null device (os.DevNull).
    //
    // If Stdin is an *os.File, the process's standard input is connected
    // directly to that file.
    //
    // Otherwise, during the execution of the command a separate
    // goroutine reads from Stdin and delivers that data to the command
    // over a pipe. In this case, Wait does not complete until the goroutine
    // stops copying, either because it has reached the end of Stdin
    // (EOF or a read error) or because writing to the pipe returned an error.

and 2) from Cmd.Wait:

If any of c.Stdin, c.Stdout or c.Stderr are not an *os.File, Wait also waits for the respective
I/O loop copying to or from the process to complete.

Which all means that running any external program, such as "date", triggers a read from stdin.

Not sure how to get around that just yet.

@theclapp
Copy link
Collaborator Author

theclapp commented May 5, 2022

Also, the above problem aside, if I/we can get it to work, this feature still needs some unit tests.

@mvdan
Copy link
Owner

mvdan commented May 8, 2022

Which all means that running any external program, such as "date", triggers a read from stdin.

Good observation. I guess this then means that we only avoid spawning our background reader if an interpreted script does not call any program nor does it use stdin itself directly. I'm trying to think how likely that is; I imagine most shell scripts do call external programs. Perhaps it's best to do like os/exec does, and unilateraly spawn the background reader the first time that Runner.Run gets called.

@theclapp
Copy link
Collaborator Author

I've made a lot of progress on this. It got surprisingly complicated, though to be sure a lot of the problem was between my ears.

As for date (et al) triggering a read from stdin, I've solved this by using the naked *os.File in exec'd programs' stdin.

Other shells have the interesting property that you can say read x ; read x and reply ^D (eof) to the first one, and the second one will still run. So I've spent some time implementing that, too.

@ghostsquad
Copy link

maybe this will help? https://github.com/muesli/cancelreader

@ghostsquad
Copy link

I just gave this a shot with cancelreader and ran into some sort of deadlock in the tests. One problem I noticed is that cancelreader must be closed where-as io.Reader does not. It wasn't clear where that should actually be done, either in code or in the tests in many cases. Additionally, I think Reset is problematic. It doesn't make sense how to reset arbitrary interfaces (like io.Reader, cancelreader.CancelReader, etc).

https://github.com/mvdan/sh/compare/master...ghostsquad:feat/preemptible-reader?expand=1

I'm at a loss. But I'm certainly very interested in this. I too would like some way of essentially sending a shell command SIGINT but not from the user. Example is spawning multiple parallel processes, and letting them race to the finish line. First one to complete wins, the remainder get interrupted.

@ghostsquad
Copy link

ghostsquad commented May 14, 2022

I guess all I really need is SIGINT. I don't even care if technically I can't cancel a stdin read, as in my specific use case, I would be unlikely to actually need to. Any command that could be interruptible isn't going to be an interactive process. And if it was interactive, a normal ctrl+c would be sufficient.

I realized that last night, what I need actually for my own tool is to be able to differentiate a ctrl+c cancellation vs a cancellation by other means. Which is easily done from the SignalHandler github.com/oklog/run package.

errors.Is(err, run.SignalError) and use that to choose whether or not I pass the cancellation explicitly to interp.Runner.Run or let the standard signal propagation to sub processes occur.

@lollipopman
Copy link
Contributor

The elivish shell also has support for reading with a timeout, their code might be worth perusing for inspiration, https://github.com/elves/elvish/commits/master/pkg/cli/term/file_reader_unix.go

@mvdan
Copy link
Owner

mvdan commented May 18, 2022

I'm happy to defer to someone who has more experience in this area; I've never used Linux's epoll (what cancelreader uses) nor select (what elvish seems to use) so it's not clear to me which is a better approach.

I'm not too worried about portability, because some parts of the interpreter like FIFOs or checking for permissions in cd are currently not implemented on Windows. We could similarly start by only implementing reader cancellation on Linux, if that's significantly better than the "pure Go" approach of a background goroutine continuously reading.

I admit I'm hoping that we can avoid writing significant amounts of OS-specific code :)

@theclapp theclapp force-pushed the ctxreader branch 2 times, most recently from b081263 to bb7d2f9 Compare July 3, 2022 17:34
- Add a CancelableReader type, and wrap stdin with it at the top of
  Runner.Run.
- Also include CancelableReaderTTY which preserves stdin looking like a
  tty, i.e. [[ -t 0 ]] is still true.
Lots of work on this before realizing my approach was fundamentally
flawed, damn it.  Committing what I have before refactoring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants