interp: reading from file redirection in while loops waits indefinitely

[andrewimeson]

In v3.9.0 of the gosh interpreter there is an issue with reading from file or file substitutions in while loops. The script just hangs when attempting to read from a file or file substitution.

This did work recently. I haven't narrowed down the exact version it broke in yet. I discovered this through it breaking in my Taskfile usage.

Examples

Works: Reading from pipe

#!/usr/bin/env gosh
set -x

echo "hi pipe" | while read -r line; do
    echo "$line"
done

echo "done"

Hangs: Reading from file

#!/usr/bin/env gosh
set -x

echo "hi file" > hi.txt

while read -r line; do
    echo "$line"
done < hi.txt

echo "done"

results in this hanging forever

$ ./while_issue.sh
+ echo 'hi file'
+ read '-r line'
+ echo 'hi file'
hi file
+ read '-r line'

Hangs: Reading from file substitution

#!/usr/bin/env gosh
set -x

while read -r line; do
    echo "$line"
done < <(echo "hi file substitution")

echo done

results in this hanging forever

$ ./while_issue.sh
+ read '-r line'
+ echo 'hi file substitution'
+ echo 'hi file substitution'
hi file substitution
+ read '-r line'

[andrewimeson]

Confirmed that it broke in 3.9.0. It works in 3.8.0.

[andrewimeson]

With git bisect I've determined that #1066 / a89b0be is the commit that broke it.

[andrewimeson]

I tried to write a test that would fail, but I can't seem to get a go test (or a CI run in my fork) to fail with my changes (still fails/hangs with the gosh interpreter). It does fail the CI run for Windows with "TODO: support process substitution on Windows," but I don't care about Windows 😀

diff --git a/interp/interp_test.go b/interp/interp_test.go
index 8b535ce3..4a4e1f2a 100644
--- a/interp/interp_test.go
+++ b/interp/interp_test.go
@@ -2905,6 +2905,10 @@ done <<< 2`,
 		"while read a; do echo $a; GOSH_CMD=exec_ok $GOSH_PROG; done <<EOF\na\nb\nc\nEOF",
 		"a\nexec ok\nb\nexec ok\nc\nexec ok\n",
 	},
+	{
+		"while read a; do echo $a; done < <(echo x)",
+		"x\n",
+	},
 	// TODO: our final exit status here isn't right.
 	// {
 	// 	"while read a; do echo $a; GOSH_CMD=exec_fail $GOSH_PROG; done <<< 'a\nb\nc'",

$ cd interp/
$ go test
PASS
ok  	mvdan.cc/sh/v3/interp	2.808s

[andrewimeson]

Here's a better test that won't run into Windows compatibility issues. I've based off of 3.8.0 and confirmed it passes in CI and it fails in CI for master.

diff --git a/interp/interp_test.go b/interp/interp_test.go
index 8b535ce3..877d8c7a 100644
--- a/interp/interp_test.go
+++ b/interp/interp_test.go
@@ -2905,6 +2905,10 @@ done <<< 2`,
 		"while read a; do echo $a; GOSH_CMD=exec_ok $GOSH_PROG; done <<EOF\na\nb\nc\nEOF",
 		"a\nexec ok\nb\nexec ok\nc\nexec ok\n",
 	},
+	{
+		"echo x > f; while read a; do echo $a; done < f",
+		"x\n",
+	},
 	// TODO: our final exit status here isn't right.
 	// {
 	// 	"while read a; do echo $a; GOSH_CMD=exec_fail $GOSH_PROG; done <<< 'a\nb\nc'",

No idea how to solve the actual bug.

[mvdan]

Thank you for filing this and for providing good test cases. I can reproduce with your latest test case, it seems to be due to an error coming from the cancelreader library. Very odd, but at least it's our fault that we hid this error.

[mvdan]

OK I figured it out; https://github.com/muesli/cancelreader uses epoll on Linux, which fails with an error on regular files, because polling on a regular file makes no sense - it always reads bytes immediately. So the cancelreader library worked OK for stdin being OS pipes, which it usually is... but when it is any sort of regular file, it would fail.

This page has a bit more context: https://stackoverflow.com/questions/5603642/epoll-ctl-operation-not-permitted-error-c-program

This will require a couple of changes upstream, and then the changes on our side will be pretty straightforward.

[mvdan]

On second thought, muesli/cancelreader#13 pretty much covers the changes I would have made, but we don't have to wait for that to be merged to resolve the problem.

[andrewimeson]

Thanks for the fix, @mvdan. How long do you think it will be until you cut the next release? It may help the Task project to decide whether to revert to an earlier release, or hang on a bit for the next release to fix the issue.

[mvdan]

The regression is pretty significant so I will do a release in the next day or so.