Skip to content

Fix and refactor command stderr handling

As mentioned in issue #3525, the command.New() with leak 2 goroutines if passed an invalid command and nil stderr. We always create a stderrCloser in such case, and 2 goroutines(command.writeLines and logrus.(*Entry).writerScanner) are created.

But if the cmd failed to start, stderrCloser will never get closed and the goroutine leak happens.

	if stderr != nil {
		command.stderrCloser = &noopWriteCloser{stderr}
		close(command.stderrDone)
	} else {
		command.stderrCloser = escapeNewlineWriter(ctxlogrus.Extract(ctx).WriterLevel(logrus.ErrorLevel), command.stderrDone, MaxStderrBytes)
	}

	cmd.Stderr = command.stderrCloser

	if err := cmd.Start(); err != nil {
		return nil, fmt.Errorf("GitCommand: start %v: %v", cmd.Args, err)
	}

The stderr handling for command currently is kind of complicated and very specifically designed for logrus. So I refactored the stderr handling by replacing the old stderrCloser and stderrDone with a single stderrBuffer.

stderrBuffer has similar functionality than before and itself implements io.Writer, which can be passed directly to cmd.Stderr. We no longer need to take much care about whether or not the pipe is closed finally, whether or not any goroutines leak, whether or not the '\n' is escaped. And we are free to use any other logger that may not provide a writer interface as logrus, and we are able to attach more useful tracking information with the stderr output in the final logProcessComplete call.

And some tests are optimized to support the new implementation.

Merge request reports