From a4009ed6fad6a95e25d8fbc56f360e1b5ec0284d Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 13:14:29 +0100 Subject: [PATCH] use script to wait --- acceptance/bin/wait_pid | 4 +- acceptance/bundle/debug/out.stderr.txt | 2 +- .../daemon/child-detaches/out.parentchild.txt | 18 ++--- acceptance/daemon/child-detaches/output.txt | 4 +- acceptance/daemon/child-detaches/script | 5 +- .../daemon/child-discards-output/output.txt | 15 ++-- .../daemon/child-discards-output/script | 6 +- cmd/selftest/child.go | 21 ++---- cmd/selftest/parent.go | 10 +-- libs/process/wait.go | 15 ---- libs/process/wait_unix.go | 36 ---------- libs/process/wait_unix_test.go | 71 ------------------- libs/process/wait_windows.go | 44 ------------ 13 files changed, 32 insertions(+), 219 deletions(-) delete mode 100644 libs/process/wait.go delete mode 100644 libs/process/wait_unix.go delete mode 100644 libs/process/wait_unix_test.go delete mode 100644 libs/process/wait_windows.go diff --git a/acceptance/bin/wait_pid b/acceptance/bin/wait_pid index f1375bf51..6e28c144c 100755 --- a/acceptance/bin/wait_pid +++ b/acceptance/bin/wait_pid @@ -13,13 +13,13 @@ wait_pid() { if [[ "$OSTYPE" == "msys"* || "$OSTYPE" == "cygwin"* ]]; then # Windows approach if ! tasklist | grep -q $pid; then - echo "Process has ended" + echo "[wait_pid] process has ended" return 0 fi else # Linux/macOS approach if ! kill -0 $pid 2>/dev/null; then - echo "Process has ended" + echo "[wait_pid] process has ended" return 0 fi fi diff --git a/acceptance/bundle/debug/out.stderr.txt b/acceptance/bundle/debug/out.stderr.txt index e5867e008..031d58082 100644 --- a/acceptance/bundle/debug/out.stderr.txt +++ b/acceptance/bundle/debug/out.stderr.txt @@ -72,7 +72,7 @@ 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=metadata.AnnotatePipelines 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Using Terraform from DATABRICKS_TF_EXEC_PATH at [TERRAFORM] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize -10:07:59 Debug: Using Terraform CLI config from DATABRICKS_TF_CLI_CONFIG_FILE at [DATABRICKS_TF_CLI_CONFIG_FILE] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize +10:07:59 Debug: DATABRICKS_TF_PROVIDER_VERSION as 1.62.0 does not match the current version 1.65.1, ignoring DATABRICKS_TF_CLI_CONFIG_FILE pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Environment variables for Terraform: ...redacted... pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit 10:07:59 Debug: No script defined for postinit, skipping pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit diff --git a/acceptance/daemon/child-detaches/out.parentchild.txt b/acceptance/daemon/child-detaches/out.parentchild.txt index 36d2e9722..e4223edfd 100644 --- a/acceptance/daemon/child-detaches/out.parentchild.txt +++ b/acceptance/daemon/child-detaches/out.parentchild.txt @@ -1,13 +1,7 @@ -Parent process has started -Started the child process -Provided input: Hello from the other side -Released the child process -Parent process is exiting - -==================== - -All output from this point on is from the child process -Parent process has exited -Received input from parent process: -Hello from the other side +[parent] started child +[parent] input sent to child: Hello from the other side +[parent] exiting +[child][wait_pid] process has ended +[child] Parent process has exited +[child] input from parent: Hello from the other side diff --git a/acceptance/daemon/child-detaches/output.txt b/acceptance/daemon/child-detaches/output.txt index 30e9c5331..1e14c4514 100644 --- a/acceptance/daemon/child-detaches/output.txt +++ b/acceptance/daemon/child-detaches/output.txt @@ -1,2 +1,2 @@ -waiting for child process to finish -Process has ended +[script] waiting for child process to end... +[script][wait_pid] process has ended diff --git a/acceptance/daemon/child-detaches/script b/acceptance/daemon/child-detaches/script index 0e83daade..cd123b66b 100644 --- a/acceptance/daemon/child-detaches/script +++ b/acceptance/daemon/child-detaches/script @@ -1,8 +1,9 @@ export DATABRICKS_CLI_SELFTEST_CHILD_OUTPUT_FILE="out.parentchild.txt" -$CLI selftest parent &> out.parentchild.txt +$CLI selftest parent > "out.parentchild.txt" -echo "waiting for child process to finish" +echo "[script] waiting for child process to end..." +echo -n "[script]" wait_pid $(cat ./child.pid) rm ./child.pid diff --git a/acceptance/daemon/child-discards-output/output.txt b/acceptance/daemon/child-discards-output/output.txt index 5fe488ade..3e92a5cb3 100644 --- a/acceptance/daemon/child-discards-output/output.txt +++ b/acceptance/daemon/child-discards-output/output.txt @@ -1,9 +1,6 @@ -Parent process has started -Started the child process -Provided input: Hello from the other side -Released the child process -Parent process is exiting - -wait for child process to finish -Process has ended -the child process should not have written any output. +[parent] started child +[parent] input sent to child: Hello from the other side +[parent] exiting +[script] waiting for child process to end... +[script][wait_pid] process has ended +[script] child process should not have written any output. diff --git a/acceptance/daemon/child-discards-output/script b/acceptance/daemon/child-discards-output/script index 94ef19c70..c7833422f 100644 --- a/acceptance/daemon/child-discards-output/script +++ b/acceptance/daemon/child-discards-output/script @@ -1,9 +1,9 @@ $CLI selftest parent -echo " -wait for child process to finish" +echo "[script] waiting for child process to end..." +echo -n "[script]" wait_pid $(cat ./child.pid) -echo "the child process should not have written any output." +echo "[script] child process should not have written any output." rm ./child.pid diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go index b1584970b..129042dec 100644 --- a/cmd/selftest/child.go +++ b/cmd/selftest/child.go @@ -1,14 +1,12 @@ package selftest import ( - "errors" "fmt" "io" "os" - "strconv" + "os/exec" "github.com/databricks/cli/libs/daemon" - "github.com/databricks/cli/libs/process" "github.com/spf13/cobra" ) @@ -17,27 +15,20 @@ func newChildCommand() *cobra.Command { return &cobra.Command{ Use: "child", RunE: func(cmd *cobra.Command, args []string) error { - parentPid, err := strconv.Atoi(os.Getenv(daemon.DatabricksCliParentPid)) + waitCmd := exec.Command("bash", "-euo", "pipefail", "wait_pid", os.Getenv(daemon.DatabricksCliParentPid)) + b, err := waitCmd.Output() if err != nil { - return fmt.Errorf("failed to parse parent PID: %w", err) - } - - err = process.Wait(parentPid) - if err != nil && !errors.As(err, &process.ErrProcessNotFound{}) { return fmt.Errorf("failed to wait for parent process: %w", err) } - - fmt.Println("\n====================") - fmt.Println("\nAll output from this point on is from the child process") - fmt.Println("Parent process has exited") + fmt.Print("[child]" + string(b)) + fmt.Println("[child] Parent process has exited") in, err := io.ReadAll(os.Stdin) if err != nil { return fmt.Errorf("failed to read from stdin: %w", err) } - fmt.Println("Received input from parent process:") - fmt.Println(string(in)) + fmt.Println("[child] input from parent: " + string(in)) return nil }, } diff --git a/cmd/selftest/parent.go b/cmd/selftest/parent.go index 7198f3d91..77d1cfe64 100644 --- a/cmd/selftest/parent.go +++ b/cmd/selftest/parent.go @@ -14,8 +14,6 @@ func newParentCommand() *cobra.Command { return &cobra.Command{ Use: "parent", RunE: func(cmd *cobra.Command, args []string) error { - fmt.Println("Parent process has started") - d := daemon.Daemon{ Env: os.Environ(), Args: []string{"selftest", "child"}, @@ -27,21 +25,19 @@ func newParentCommand() *cobra.Command { if err != nil { return fmt.Errorf("failed to start child process: %w", err) } - fmt.Println("Started the child process") + fmt.Println("[parent] started child") err = d.WriteInput([]byte("Hello from the other side\n")) if err != nil { return fmt.Errorf("failed to write to child process: %w", err) } - fmt.Println("Provided input: Hello from the other side") + fmt.Println("[parent] input sent to child: Hello from the other side") err = d.Release() if err != nil { return fmt.Errorf("failed to release child process: %w", err) } - fmt.Println("Released the child process") - - fmt.Println("Parent process is exiting") + fmt.Println("[parent] exiting") return nil }, } diff --git a/libs/process/wait.go b/libs/process/wait.go deleted file mode 100644 index 251ef1db6..000000000 --- a/libs/process/wait.go +++ /dev/null @@ -1,15 +0,0 @@ -package process - -import "fmt" - -type ErrProcessNotFound struct { - Pid int -} - -func (e ErrProcessNotFound) Error() string { - return fmt.Sprintf("process with pid %d does not exist", e.Pid) -} - -func Wait(pid int) error { - return waitForPid(pid) -} diff --git a/libs/process/wait_unix.go b/libs/process/wait_unix.go deleted file mode 100644 index 03f548f97..000000000 --- a/libs/process/wait_unix.go +++ /dev/null @@ -1,36 +0,0 @@ -//go:build linux || darwin - -package process - -import ( - "errors" - "os" - "syscall" - "time" -) - -func waitForPid(pid int) error { - p, err := os.FindProcess(pid) - if err != nil { - return err - } - - // Initial existence check. - if err := p.Signal(syscall.Signal(0)); err != nil { - if errors.Is(err, os.ErrProcessDone) { - return ErrProcessNotFound{Pid: pid} - } - return err - } - - // Polling loop until process exits - for { - if err := p.Signal(syscall.Signal(0)); err != nil { - if errors.Is(err, os.ErrProcessDone) { - return nil - } - return err - } - time.Sleep(100 * time.Millisecond) - } -} diff --git a/libs/process/wait_unix_test.go b/libs/process/wait_unix_test.go deleted file mode 100644 index 680e8b5c3..000000000 --- a/libs/process/wait_unix_test.go +++ /dev/null @@ -1,71 +0,0 @@ -package process - -import ( - "os" - "os/exec" - "path/filepath" - "runtime" - "strconv" - "testing" - "time" - - "github.com/databricks/cli/internal/testutil" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestWaitUnix(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("Skipping test on Windows") - } - - t.Parallel() - tmpDir := t.TempDir() - - pidFile := filepath.Join(tmpDir, "child.pid") - outputFile := filepath.Join(tmpDir, "output.txt") - - // For this test, we cannot start the background process in this test itself - // and instead have to use parent.sh as an intermediary. - // - // This is because in Unix if we start the background process in this test itself, - // the background process will be a child of this test process and thus would - // need to be reaped by this test process (using the Wait function / syscall). - // Otherwise waitForPid will forever wait for the background process to finish. - // - // If we rely on an intermediate script to start the background process, the - // background process is reasigned to the init process (PID 1) once the parent - // exits and thus we can successfully wait for it in this test using waitForPid function. - cmd := exec.Command("./testdata/parent.sh", pidFile, outputFile) - err := cmd.Start() - require.NoError(t, err) - - // Wait 5 seconds for the parent bash script to write the child's PID to the file. - var childPid int - require.Eventually(t, func() bool { - b, err := os.ReadFile(pidFile) - if err != nil { - return false - } - - childPid, err = strconv.Atoi(string(b)) - require.NoError(t, err) - return true - }, 2*time.Second, 100*time.Millisecond) - - // The output file should not exist yet since the background process should - // still be running. - assert.NoFileExists(t, outputFile) - - // Wait for the background process to finish. - err = waitForPid(childPid) - assert.NoError(t, err) - - // The output file should exist now since the background process has finished. - testutil.AssertFileContents(t, outputFile, "abc\n") - - // Since the background process has finished, waiting for it again should - // return an error. - err = waitForPid(childPid) - assert.Regexp(t, "process with pid .* does not exist", err.Error()) -} diff --git a/libs/process/wait_windows.go b/libs/process/wait_windows.go deleted file mode 100644 index c7a8d9ac7..000000000 --- a/libs/process/wait_windows.go +++ /dev/null @@ -1,44 +0,0 @@ -//go:build windows - -package process - -import ( - "errors" - "fmt" - "time" - - "golang.org/x/sys/windows" -) - -func waitForPid(pid int) error { - handle, err := windows.OpenProcess( - windows.SYNCHRONIZE|windows.PROCESS_QUERY_INFORMATION, - false, - uint32(pid), - ) - if errors.Is(err, windows.ERROR_INVALID_PARAMETER) { - return ErrProcessNotFound{Pid: pid} - } - if err != nil { - return fmt.Errorf("OpenProcess failed: %v", err) - } - defer windows.CloseHandle(handle) - - // Wait forever for the process to exit. Wait for 5 minutes max. - ret, err := windows.WaitForSingleObject(handle, uint32(5*time.Minute.Milliseconds())) - if err != nil { - return fmt.Errorf("Wait failed: %v", err) - } - - switch ret { - case windows.WAIT_OBJECT_0: - return nil // Process exited - case 0x00000102: - // Standard library does not have have a constant defined for this - // so we use the hex value directly. This is the WAIT_TIMEOUT value. - // ref: https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobject#return-value - return fmt.Errorf("process wait timed out") - default: - return fmt.Errorf("unexpected process wait return value: %d", ret) - } -}