Abstract redirects and pipes into inputStream and outputStream interface #206

juster · 2023-08-30T13:08:15Z

This creates the inputStream and outputStream interfaces which are satisfied by structs for all combinations of file/pipe and input/output:

Input File - struct inFileStream
Input Command - struct inCmdStream
Output File - struct outFileStream
Output Command - struct outCmdStream

The streams satisfy the io.ReadCloser and io.WriteCloser interfaces while the outputStream also has a Flush() error method. Both types of streams also have an ExitCode() int method which makes it trivial to determine the result of a close() on those streams.

This PR homogenizes behavior and fixes a number of minor issues related to command pipes. Gawk behavior is mimicked for the following:

The exit status of the child process is returned from close() when closing a pipe. Exit due to signal is 256 + signal and if both a signal and coredump occurred it is 512 + signal. (see close() on pipe command does not return exit status #203)
The same rules apply to the result of the system() builtin function. (see system() always returns -1 if process exits due to signal #205)

A separate bug was fixed so that the child process information is cleaned up when close() is called on an input pipe (i.e. via getline). (see #204)

benhoyt · 2023-08-30T21:52:01Z

Thanks for this -- this is excellent work! I hope to take a closer look at the code changes this weekend, but at first glance is looks like a good approach.

FYI, I've just merged #207 to update to Go 1.16 so we can use io.Discard etc now. So if you merge the latest master into this branch, that should fix the test failures.

juster · 2023-08-31T01:06:28Z

That's great thanks! Do you mind if I rebase instead of merge? I rebase a lot it makes the commit history cleaner but spams up the issues & PRs... 😞

I noticed that the Linux test had a failure. It seemed that the signal may have been handled and the sh process itself did not exit due to signal. I'll have to dig in more there and research which sh implementation is used and how it handles signals, etc. Ofc that is just a guess for now.

benhoyt · 2023-08-31T01:19:20Z

@juster Yep, rebasing is fine. My development style is usually "push any old commit to the PR branch, then squash and merge when merging the PR", but if you keep a clean commit history in the PR, I'm happy to apply directly to master instead.

juster · 2023-08-31T11:57:03Z

I found the problem with the ExitSigPipe test on Linux runners:

--- FAIL: TestStreamInCmd (0.00s)
    --- FAIL: TestStreamInCmd/ExitSigPipe (0.52s)
        iostream_test.go:168: expected Close() result: 269, got: 141

This test is running the yes utility because it prints non-stop. When I close the stream it will cause a SIGPIPE (13) signal to be sent to yes because it tries to print to a closed pipe. The difference here is that sh is linked to bash and bash has a special exit code if one of its commands dies due to signal:

When a command terminates on a fatal signal whose number is N, Bash uses the value 128+N as the exit status.

Bash Reference Manual

On my old Macbook sh is linked to dash which does not trap the signal and provide its own exit status. Instead, it also dies with SIGPIPE.

This is an unfortunate design choice! Because now there is no way to be sure what the exit value of system for example will be if there is a signal. It varies depending on what is used for sh. GNU cleverness strikes again 😉 .

Oh well. As we've seen in #205 signal exits are kind of an overlooked gray area. I think I will try to avoid spawning a shell at all for this test and just run yes directly with an os/exec.Cmd.

In my testing I also found some signals are caught by default with shells. For example, I will avoid SIGQUIT (3) which is caught and just quits the shell. I used this command to try all the signals:

kill -l | ./goawk '{for(i=1;i<=NF;i++)printf "%2s %6s %s\n", ++j, $i, system("bash -c \047kill -"i" $$\047")}'

benhoyt

I haven't reviewed iostream.go or iostream_test.go yet (the bulk of the PR!), but here's a few early comments I had time for yesterday. I'll try to get to the rest over the weekend.

Thanks again for this. Note that I'm fairly thorough in my reviews as I like to get quality code merged and code that I can "own" from here on out. Hopefully we'll end up with something we're both proud of!

interp/vm.go

interp/iostream.go

juster · 2023-09-01T00:37:54Z

I haven't reviewed iostream.go or iostream_test.go yet (the bulk of the PR!), but here's a few early comments I had time for yesterday. I'll try to get to the rest over the weekend.

Thanks again for this. Note that I'm fairly thorough in my reviews as I like to get quality code merged and code that I can "own" from here on out. Hopefully we'll end up with something we're both proud of!

Not a problem and no rush. I think I want to [edit: squash] the "cleanup" commit into the first commit and pretend it was always cleaned up.

BTW, I found a simple way around the bash problem. I just need to use exec. What a lovely, strange command 😄.

benhoyt

Thanks @juster! I've done a thorough review now and left a lot of comments. They're mostly fairly minor refactoring requests, but the one major thing is I'd like to restructure the tests to be black-box tests that test external behaviour like the other interp tests, rather than testing implementation details. I'm happy to discuss further if that's not possible.

interp/interp_test.go

interp/vm.go

interp/iostream.go

interp/iostream_test.go

benhoyt · 2023-09-02T04:25:57Z

One other thing: it looks like there's also merge/rebase conflicts from the ioutil -> io change that I merged -- those will need to be resolved.

benhoyt

Excellent, thanks! Just a few very minor change requests now.

interp/interp_test.go

interp/iostream_test.go

interp/iostream.go

Creates the iostream.go file and uses I/O stream interfaces for all file redirects and command pipes. Defines inputStream and outputStream interfaces which satisfy the io.ReadCloser and io.WriteCloser interfaces, respectively. Adds an additional .ExitCode() accessor to return the exit code for goawk's close() builtin function. When the argument to close() is a pipe, this mimicks gawk's behavior: 1. The value passed to exit() when process exits normally 2. 256 + signal code when process exits due to signal 3. 512 + signal code when process exits due to signal with a core dump Fixes benhoyt#203 Fixes benhoyt#204

Old behavior always returned -1 on a signal exit. Fixes benhoyt#205

Enable gawk tests "status-close" and "sigpipe1". "sigpipe1" is skipped on Windows because it relies on the SIGPIPE POSIX signal. Modify TestShellCommand to check interpreter exit after missing shell Adds some interpTests entries to check signal exit from system(). Also adds entries to check signal exit and normal exit from a piped command close().

benhoyt

Thanks for the updates and the little flags parser! Just one more change requested (and a couple of nits): I was intending that !windows mean "skip on windows, regardless of interpreter" as I think that makes the most sense (and fits with the meaning of Go's own !windows build constraint).

interp/interp_test.go

interp/iostream.go

interp/iostream_test.go

interp/interp_test.go

There is not much to test. The only important one I could think of was consistent behavior in the event of a double-close.

The !windows flag will cause both the external awk tests and the goawk tests to be skipped if the tests are running on Windows. Signal-based test cases were failing on windows for goawk because Windows does not support POSIX signals. These are simulated on cygwin which is what MSYS2 appears to be using. MSYS2 is preinstalled on the Windows image which the CI runs. My best guess is that the MSYS2 kill command is forcing the child process to terminate with (signal << 8) as the exit status. Additionally, the gawk in the Windows CI image is using cmd.exe and not sh. Naturally, there are no exec or kill commands available from cmd.exe. Ironically, the signal exit status behaves predictibly for gawk on cygwin. Stricter flag parsing exposed some interpTest entries that were missing a comment char ('#').

benhoyt · 2023-09-24T23:34:05Z

Thanks for your efforts here -- merged! I intend to address a couple of other issues and then release a new GoAWK version that includes this change.

juster · 2023-09-24T23:58:11Z

Sweet thanks!

I noticed this failed test: https://github.com/benhoyt/goawk/actions/runs/6293036690/job/17083040381

It looks like there is a race and the shell exited before goawk could write to the pipe. I should probably change exit 9 to read; exit 9 to force the shell to read the empty line before exiting.

benhoyt · 2023-09-25T00:39:13Z

Sounds good -- please put up another PR with the fix. For the record, I can get it to fail a few times locally by running it 1000 times:

go test -v -count=1000 -run='^TestInterp/^BEGIN' ./interp

benhoyt reviewed Aug 31, 2023

View reviewed changes

interp/vm.go Show resolved Hide resolved

interp/vm.go Outdated Show resolved Hide resolved

interp/vm.go Outdated Show resolved Hide resolved

interp/vm.go Show resolved Hide resolved

interp/iostream.go Outdated Show resolved Hide resolved

juster force-pushed the pipe_close_exit_code branch 2 times, most recently from fc5297b to 7f1c845 Compare September 1, 2023 11:57

benhoyt requested changes Sep 2, 2023

View reviewed changes

juster force-pushed the pipe_close_exit_code branch 3 times, most recently from 258b10b to 143937b Compare September 10, 2023 14:15

benhoyt requested changes Sep 11, 2023

View reviewed changes

juster added 3 commits September 17, 2023 07:56

Make system() result identical to close(pipe) result

2255a37

Old behavior always returned -1 on a signal exit. Fixes benhoyt#205

juster force-pushed the pipe_close_exit_code branch 2 times, most recently from 25de3c6 to 07e8760 Compare September 17, 2023 13:24

benhoyt requested changes Sep 17, 2023

View reviewed changes

juster added 2 commits September 23, 2023 20:01

Adds iostream_test.go to test iostream in interp

159ce7b

There is not much to test. The only important one I could think of was consistent behavior in the event of a double-close.

juster force-pushed the pipe_close_exit_code branch from 07e8760 to 90259cf Compare September 24, 2023 19:38

benhoyt merged commit 17fbb8b into benhoyt:master Sep 24, 2023

Porkepix mentioned this pull request Sep 26, 2023

goawk 1.25.0 Homebrew/homebrew-core#146079

Merged

juster deleted the pipe_close_exit_code branch October 8, 2023 01:46

juster mentioned this pull request Oct 8, 2023

Fix rare race in pipe-to-command close() result test #213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract redirects and pipes into inputStream and outputStream interface #206

Abstract redirects and pipes into inputStream and outputStream interface #206

juster commented Aug 30, 2023

benhoyt commented Aug 30, 2023

juster commented Aug 31, 2023

benhoyt commented Aug 31, 2023

juster commented Aug 31, 2023 •

edited

Loading

benhoyt left a comment

juster commented Sep 1, 2023 •

edited

Loading

benhoyt left a comment

benhoyt commented Sep 2, 2023

benhoyt left a comment

benhoyt left a comment

benhoyt commented Sep 24, 2023

juster commented Sep 24, 2023

benhoyt commented Sep 25, 2023

Abstract redirects and pipes into inputStream and outputStream interface #206

Abstract redirects and pipes into inputStream and outputStream interface #206

Conversation

juster commented Aug 30, 2023

benhoyt commented Aug 30, 2023

juster commented Aug 31, 2023

benhoyt commented Aug 31, 2023

juster commented Aug 31, 2023 • edited Loading

benhoyt left a comment

Choose a reason for hiding this comment

juster commented Sep 1, 2023 • edited Loading

benhoyt left a comment

Choose a reason for hiding this comment

benhoyt commented Sep 2, 2023

benhoyt left a comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

benhoyt commented Sep 24, 2023

juster commented Sep 24, 2023

benhoyt commented Sep 25, 2023

juster commented Aug 31, 2023 •

edited

Loading

juster commented Sep 1, 2023 •

edited

Loading