Cancellable reads; EOF on "read" sets var to ""

mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt

https://pkg.go.dev/mvdan.cc/sh/v3

BSD 3-Clause "New" or "Revised" License

6.97k stars 332 forks source link

Cancellable reads; EOF on "read" sets var to "" #1066

Closed theclapp closed 2 months ago

theclapp commented 3 months ago

Make reading cancellable. Not all input from stdin, just that done directly by the shell (i.e. the "read" builtin). Exec'ed programs still read directly from stdin's os.File and are not cancellable.
If you press ^D (EOF) when reading into a shell variable, set the variable to "". This is consistent with bash & zsh.

Fixes https://github.com/mvdan/sh/issues/856. See also https://github.com/mvdan/sh/pull/857 for my first try at this.

mvdan commented 3 months ago

Overall nothing against merging this feature though - it is one added dependency, but it seems like a light one, and I don't think we can possibly implement cancelling reads any way other than OS-specific code.

theclapp commented 3 months ago

I assume this is one of those that is hard to test properly, but I'd still like us to try, because the logic is non-trivial and feels fragile.

Sure, no worries.

theclapp commented 3 months ago

only newline (empty input)

only ^D (empty input)

some string and newline (non-empty input)

some string and ^D (non-empty input)

It turns out the current code already works with the first, third, and fourth. I've added a test case with the second.

None of these cases actually test having a read in-progress and cancelling its context, so I wrote a separate test for that. It turns out it wasn't that difficult.

theclapp commented 3 months ago

As noted in the commit: TestCancelreader failing intermittently under Linux (on a Debian VM), even though the read context is definitely cancelled. Not sure what's going on. Committing what I've got, for posterity.

theclapp commented 3 months ago

TestCancelreader failing intermittently under Linux

At a guess, this is because of, or at least related to, the data race reported in the CI tests. Will look more tomorrow.

theclapp commented 3 months ago

@ghostsquad Wanna take a look?

mvdan commented 3 months ago

@theclapp do you want to squash your commits into one and write a single commit message? I could squash myself, only keeping the original commit message, but I suspect you might want to keep some of the text from the other commit messages, e.g. about the test.

theclapp commented 3 months ago

Squashed 'em all.