-
Notifications
You must be signed in to change notification settings - Fork 380
TOOLS-4148 Add tests for SIGPIPE handling, remove equivalent JS tests, and fix handling for other signals #956
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| // Copyright (C) MongoDB, Inc. 2014-present. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
| // not use this file except in compliance with the License. You may obtain | ||
| // a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| package signals | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "os/exec" | ||
| "testing" | ||
|
|
||
| "github.com/mongodb/mongo-tools/common/testutil" | ||
| ) | ||
|
|
||
| // TestMain handles the subprocess mode used by TestBrokenPipeHandledAsWriteError. | ||
| func TestMain(m *testing.M) { | ||
| if os.Getenv("TEST_BROKEN_PIPE_SUBPROCESS") == "1" { | ||
| // Set up SIGPIPE handling as our tools do. | ||
| done := HandleWithInterrupt(nil) | ||
| defer close(done) | ||
|
|
||
| // Write repeatedly to stdout. When the reader closes the pipe, | ||
| // the write will return an EPIPE error. We exit 0 to signal that | ||
| // the error was surfaced and handled — not a SIGPIPE death. | ||
| for { | ||
| _, err := fmt.Println("data") | ||
| if err != nil { | ||
| os.Exit(0) | ||
| } | ||
| } | ||
| } | ||
| os.Exit(m.Run()) | ||
| } | ||
|
|
||
| // TestBrokenPipeHandledAsWriteError verifies that when the read end of a pipe | ||
| // is closed, our signal handler causes the write error to surface as an EPIPE | ||
| // (allowing the tool to exit cleanly) rather than the process being killed by | ||
| // SIGPIPE. | ||
| func TestBrokenPipeHandledAsWriteError(t *testing.T) { | ||
| cmd := exec.Command(os.Args[0], "-test.run=TestBrokenPipeHandledAsWriteError") | ||
| cmd.Env = append(os.Environ(), "TEST_BROKEN_PIPE_SUBPROCESS=1") | ||
| testutil.AssertBrokenPipeHandled(t, cmd) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| // Copyright (C) MongoDB, Inc. 2014-present. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
| // not use this file except in compliance with the License. You may obtain | ||
| // a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| //go:build !windows | ||
|
|
||
| package testutil | ||
|
|
||
| import ( | ||
| "os" | ||
| "os/exec" | ||
| "syscall" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // AssertBrokenPipeHandled starts cmd with its stdout connected to a pipe, | ||
| // reads a small amount of output to confirm the process has started writing, | ||
| // then breaks the pipe by closing the read end. It asserts the process was | ||
| // not killed by SIGPIPE — a broken pipe must surface as a write error that | ||
| // the process can handle cleanly. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This description seems a bit misleading with the function written as it is because—to me, at least—it implies that we expect a failure due to write error (albeit one that the program handles cleanly). (I can also read it as not implying that, but it’s not the first impression I got when reading this paragraph.)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we expect the command we run to fail due to a write error. That's what the code looks for. It checks that the process exited but not via a I wondered why we have the conditional on whether we got an exit error, but I think that makes sense. It's possible that a tool could handle a write error and exit 0. Maybe they shouldn't do that, but that's somewhat out of scope for these tests, so I think having the test pass with exit 0 is reasonable. |
||
| // | ||
| // cmd.Stdout must not be set before calling; this function sets it to the | ||
| // write end of a pipe. | ||
| func AssertBrokenPipeHandled(t *testing.T, cmd *exec.Cmd) { | ||
| t.Helper() | ||
|
|
||
| pr, pw, err := os.Pipe() | ||
| require.NoError(t, err) | ||
| cmd.Stdout = pw | ||
|
|
||
| require.NoError(t, cmd.Start()) | ||
| require.NoError(t, pw.Close()) | ||
|
|
||
| buf := make([]byte, 64) | ||
| _, err = pr.Read(buf) | ||
| require.NoError(t, err, "process should write at least some output before the pipe breaks") | ||
| require.NoError(t, pr.Close()) | ||
|
|
||
| err = cmd.Wait() | ||
| if err == nil { | ||
| return | ||
| } | ||
|
|
||
| var exitErr *exec.ExitError | ||
| require.ErrorAs(t, err, &exitErr, "non-zero exit from cmd.Wait() should always be an ExitError") | ||
| status, ok := exitErr.Sys().(syscall.WaitStatus) | ||
| require.True(t, ok, "exitErr.Sys() should return a syscall.WaitStatus") | ||
| if status.Signaled() { | ||
| require.NotEqual( | ||
| t, | ||
| syscall.SIGPIPE, | ||
| status.Signal(), | ||
| "process should not be killed by SIGPIPE: broken pipe should surface as a write error", | ||
| ) | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we retain the check for
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we should. In the JS code, this had been commented out for I don't want to try to figure that out right now, especially since I think this is all pointless and we don't need to suppress SIGPIPE any more (this is a legacy of very early Go versions being weird with |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // Copyright (C) MongoDB, Inc. 2014-present. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
| // not use this file except in compliance with the License. You may obtain | ||
| // a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| //go:build windows | ||
|
|
||
| package testutil | ||
|
|
||
| import ( | ||
| "os/exec" | ||
| "testing" | ||
| ) | ||
|
|
||
| // AssertBrokenPipeHandled is a no-op on Windows where SIGPIPE does not apply. | ||
| func AssertBrokenPipeHandled(t *testing.T, cmd *exec.Cmd) { | ||
| t.Skip("broken pipe handling via SIGPIPE is not applicable on Windows") | ||
| } |
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the race-prevention logic here also be applied to the handling of TERM & INT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Aside: I’m a bit surprised this compiles on Windows …)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question re: race. I have no idea why the code is written this way. I think that it's not really a big deal. The worst case is that we get a TERM or INT signal before our handlers are installed, but I don't think that would cause any breakage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for Windows, I think the Go stdlib & runtime handles the platform-specific details to make this work.