Repository: opentofu/opentofu
File: internal/command/command_test.go (Go)

Code before:

	if runtime.GOOS == "windows" {
		lockBin = lockBin + ".exe"
	}

	cmd := exec.Command("go", "build", "-o", lockBin, source)

	cmd.Dir = filepath.Dir(sourceDir)

	out, err := cmd.CombinedOutput()
	if err != nil {
		return nil, fmt.Errorf("%w %s", err, out)
	}

	locker := exec.Command(lockBin, path)
	pr, pw, err := os.Pipe()
	if err != nil {
		return nil, err
	}
	tr, tw, err := os.Pipe()
	if err != nil {
		return nil, err
	}
	locker.Stderr = pw
	locker.Stdout = pw
	locker.Stdin = tr

	if err := locker.Start(); err != nil {
		return nil, err
	}

	reader := bufio.NewReader(pr)

	// callback function to unlock the state file
	cbFunc := func() {
		defer tr.Close()
		defer tw.Close()
		defer pr.Close()
		defer pw.Close()

		_, err = io.WriteString(tw, "UNLOCK\n")
		if err != nil {
			t.Fatalf("write to statelocker returned: %v", err)
		}

		// wait for the process to unlock
		buf, err := reader.ReadString('\n')
		if err != nil {
			t.Fatalf("read from statelocker returned: %v", err)
		}


Review comment (bug):
Having two processes do bidirectional communication with pipes is notoriously tricky because a write to a pipe blocks until the other party reads it, and so it's easy to accidentally deadlock if one of the processes doesn't behave exactly as expected.

While I don't think there's anything actually wrong with what you wrote here -- the statelocker program is reading from its stdin in a goroutine so blocking there is asynchronous from everything else -- I worry that this will be hard to understand and maintain in future.

Although I concede it's not _that_ much easier to understand, I wonder if it would be sufficient to _just_ call `tw.Close()` in `cbFunc` below, without first writing `UNLOCK` to it. Then the statelocker program's `waitForInput` could call `os.Stdin.Read()` and expect it to return the `io.EOF` error once the write end of the pipe is closed. That subtle change means that the parent process never needs to _write_ to `tw` and so cannot get blocked waiting for the child to read from its `os.Stdin`.




Code after:

	if runtime.GOOS == "windows" {
		lockBin = lockBin + ".exe"
	}

	cmd := exec.Command("go", "build", "-o", lockBin, source)

	cmd.Dir = filepath.Dir(sourceDir)

	out, err := cmd.CombinedOutput()
	if err != nil {
		return nil, fmt.Errorf("%w %s", err, out)
	}

	locker := exec.Command(lockBin, path)
	stdin, err := locker.StdinPipe()
	if err != nil {
		return nil, err
	}
	stdout, err := locker.StdoutPipe()
	if err != nil {
		return nil, err
	}

	if err := locker.Start(); err != nil {
		return nil, err
	}

	reader := bufio.NewReader(stdout)

	// callback function to unlock the state file
	cbFunc := func() {
		stdin.Close()
		stdout.Close()

		_ = locker.Wait()

		t.Logf("closed statelocker stdin and finished.")
	}

	// wait for the process to lock
	buf, err := reader.ReadString('\n')
	if err != nil {
		return cbFunc, fmt.Errorf("read from statelocker returned: %w", err)
	}

	output := string(buf)
	if !strings.HasPrefix(output, "LOCKID") {
		return cbFunc, fmt.Errorf("statelocker wrote: %s", output)
	}
	t.Logf("statelocker locked %s", output)