Cancellable reads; EOF on "read" sets var to "" #1066
Conversation
There was a problem hiding this comment.
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.
For example, with https://pkg.go.dev/os#Pipe to create a stdin file, and cancelling the runner context to mimic ^D, we could test a few edge cases with the read builtin:
- only newline (empty input)
- only ^D (empty input)
- some string and newline (non-empty input)
- some string and ^D (non-empty input)
|
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. |
Sure, no worries. |
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. |
|
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. |
At a guess, this is because of, or at least related to, the data race reported in the CI tests. Will look more tomorrow. |
|
@ghostsquad Wanna take a look? |
|
@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. |
- 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.
|
Squashed 'em all. |
Fixes #856.
See also #857 for my first try at this.