From a4cea1b4f32f2c07d470ab47fbcb6a383731bfed Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 26 Oct 2022 23:51:07 +0200 Subject: [PATCH] go1.19: zfs.SendStream.Close() after EOF would return context cancellation error Before upgrading to Go 1.19, these platform tests would sproadically fail due to the reason outlined in the comment github.com/zrepl/zrepl/platformtest/tests.SendStreamMultipleCloseAfterEOF github.com/zrepl/zrepl/platformtest/tests.SendStreamCloseAfterEOFRead --- zfs/zfs.go | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/zfs/zfs.go b/zfs/zfs.go index 5d2aa1d..a8bf86d 100644 --- a/zfs/zfs.go +++ b/zfs/zfs.go @@ -343,7 +343,6 @@ const ( type SendStream struct { cmd *zfscmd.Cmd - kill context.CancelFunc stdoutReader io.ReadCloser // not *os.File for mocking during platformtest stderrBuf *circlog.CircularLog @@ -416,8 +415,29 @@ func (s *SendStream) killAndWait() error { debug("sendStream: killAndWait enter") defer debug("sendStream: killAndWait leave") + // ensure this function is only called once + if s.state != sendStreamOpen { + panic(s.state) + } + // send SIGKILL - s.kill() + // In an earlier version, we used the starting context.Context's cancel function + // for this, but in Go > 1.19, doing so will cause .Wait() to return the + // context cancel error instead of the *exec.ExitError. + err := s.cmd.Process().Kill() + if err != nil { + if err == os.ErrProcessDone { + // This can happen if + // (1) the process has already been .Wait()ed, or + // (2) some other goroutine cancels the context, likely further up + // the context tree. + // Case (1) can't happen to us because we only call + // this function in sendStreamOpen state. + // In Case (2), it's still our job to .Wait(), so, fallthrough. + } else { + return err + } + } // Close our read-end of the pipe. // @@ -963,10 +983,10 @@ func ZFSSend(ctx context.Context, sendArgs ZFSSendArgsValidated) (*SendStream, e stream := &SendStream{ cmd: cmd, - kill: cancel, stdoutReader: stdoutReader, stderrBuf: stderrBuf, } + _ = cancel // the SendStream.killAndWait() will kill the process return stream, nil }