Add a new test for our SIGPIPE handling in the common code

This test makes sure that we ignore SIGPIPE. Instead, writes to a broken pipe will cause the write call to return an error. The tools should always check errors on write calls, so these will be surfaced as normal "failure to write to an fd" errors, as opposed to the signal aborting the process.

Author: autarch

common/signals/signals.go

@@ -21,24 +21,23 @@ func Handle() chan struct{} {
 	return HandleWithInterrupt(nil)
 }
 
-// HandleWithInterrupt starts a goroutine which listens for SIGTERM, SIGINT, and
-// SIGKILL and explicitly ignores SIGPIPE. It calls the finalizer function when
-// the first signal is received and forcibly terminates the program after the
-// second. If a nil function is provided, the program will exit after the first
-// signal.
+// HandleWithInterrupt starts a goroutine which listens for SIGTERM, SIGINT, and SIGKILL. It also
+// calles signal.Ignore to explicitly ignore SIGPIPE. It calls the finalizer function when the first
+// signal is received and forcibly terminates the program after the second. If a nil function is
+// provided, the program will exit after the first signal.
 func HandleWithInterrupt(finalizer func()) chan struct{} {
+	// Ignore SIGPIPE synchronously so there is no race between this call and the first write: a
+	// broken pipe must surface as an EPIPE write error, not kill the process.
+	signal.Ignore(syscall.SIGPIPE)
+
 	finishedChan := make(chan struct{})
 	go handleSignals(finalizer, finishedChan)
 	return finishedChan
 }
 
 func handleSignals(finalizer func(), finishedChan chan struct{}) {
-	// explicitly ignore SIGPIPE; the tools should deal with write errors
-	noopChan := make(chan os.Signal)
-	signal.Notify(noopChan, syscall.SIGPIPE)
-
 	log.Logv(log.DebugLow, "will listen for SIGTERM, SIGINT, and SIGKILL")
-	sigChan := make(chan os.Signal, 2)
+	sigChan := make(chan os.Signal, 1)
 	signal.Notify(sigChan, syscall.SIGTERM, syscall.SIGINT)
 	defer signal.Stop(sigChan)
 	if finalizer != nil {

common/signals/signals_test.go

@@ -0,0 +1,82 @@
+// 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 (
+	"errors"
+	"fmt"
+	"os"
+	"os/exec"
+	"syscall"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// 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 (from all the '*_broken_pipe.js' JS tests).
+func TestBrokenPipeHandledAsWriteError(t *testing.T) {
+	cmd := exec.Command(os.Args[0], "-test.run=TestBrokenPipeHandledAsWriteError")
+	cmd.Env = append(os.Environ(), "TEST_BROKEN_PIPE_SUBPROCESS=1")
+
+	pr, pw, err := os.Pipe()
+	require.NoError(t, err)
+	cmd.Stdout = pw
+
+	require.NoError(t, cmd.Start())
+	require.NoError(t, pw.Close()) // parent closes its copy so reads see EOF
+
+	// Let the subprocess write a little before breaking the pipe.
+	buf := make([]byte, 64)
+	_, err = pr.Read(buf)
+	require.NoError(t, err)
+	require.NoError(t, pr.Close()) // break the pipe
+
+	err = cmd.Wait()
+
+	// Exit 0 means the subprocess caught EPIPE and handled it cleanly.
+	if err == nil {
+		return
+	}
+
+	var exitErr *exec.ExitError
+	if errors.As(err, &exitErr) {
+		status, ok := exitErr.Sys().(syscall.WaitStatus)
+		require.True(t, ok)
+		if status.Signaled() {
+			assert.NotEqual(
+				t,
+				syscall.SIGPIPE,
+				status.Signal(),
+				"process should not be killed by SIGPIPE: broken pipe should surface as a write error",
+			)
+		}
+	}
+}