fix hang with ginkgo -p (#1192)

* integration: build interceptor binary automatically

Trying to run this locally the interceptor binary did not exists.
Lets just build this in SynchronizedBeforeSuite() automatically so
users do not have to figure this out.

Signed-off-by: Paul Holzinger <[email protected]>

* integration: make interceptor test parallel safe

Using the same file name for all test cause conflicts when run in
parallel, this causes the tests to fail for me. To fix this use a
filename suffix with GinkgoParallelProcess() which prevents conflicts
in the file name.

Signed-off-by: Paul Holzinger <[email protected]>

* fix hang with ginkgo -p

When you run any child process that stay around longer than the test
suite ginkgo currently hangs. This is because the dup stdout/err fds are
leaked into the child thus read() will block on it as there is at least
one process still having the write pipe open.

From ginkgos POV it looks like it is done, you see the ginkgo result
output printed but then it just hangs and doe snot exit because of it.

To fix it we set FD_CLOEXEC on the dup-ed fds, this prevents them from
ever being leaking into other processes that could outlive the suite.
There is a dup3() call the could be uses to set the CLOEXEC option
directly but this seem linux only and thus is less portable.
The fcntl call should be good enough here, we do not need to be worried
about the race conditions described in the man page.
Ideally we should do some error handling in that function for both the
fcntl calls and the existing dup() above, however this seems like a
rather big change.

I am not so sure about how to test it properly, I added a test which
just executes `ginkgo run -p` and a test which only starts `sleep 60`.
Ginkgo then should exit right way and keep this process running. Then I
just make sure the gingo exits in under 15 seconds. As long as it is
below 60s it should fulfil the purpose.

Fixes #1191

Signed-off-by: Paul Holzinger <[email protected]>

* Rearrange new interceptor hang tests and confirm they cover the issue in question

---------

Signed-off-by: Paul Holzinger <[email protected]>
Co-authored-by: Onsi Fakhouri <[email protected]>

TODO

@@ -0,0 +1,60 @@
+# Backlog
+[] @G fix hang with ginkgo -p 
+    https://github.com/onsi/ginkgo/issues/1192
+[] @B biloba needs to support "McDonald's"
+[] @G Document order of execution for setup nodes on the same level
+    https://github.com/onsi/ginkgo/issues/1172
+[] @G fail fast may cause Serial spec or cleanup Node interrupted
+    https://github.com/onsi/ginkgo/issues/1178
+
+# Needs-Prioritization
+[] @G Pull JUnit config out of code and into flags/config files
+    https://github.com/onsi/ginkgo/issues/1115
+[] @G -exec support for `ginkgo run`
+    https://github.com/onsi/ginkgo/issues/869
+[] @G Add `indent` to gcustom
+[] @G HaveField should not panic in FailureMessage if the field is non-existant.  It should return a meaningful failure message.
+[] @G allow NodeTimeout and GracePeriod to live on containers and propagate down
+[] @G Clean up ReportEntry decoding:
+    func (entry ReportEntry) DecodeValue(p interface{}) error {
+        // if raw is assignable to p, assign
+        // else - parse AsJSON
+    }
+[] @B Biloba lets you get all innerHTML and emit it on failure
+[] @B equivalent of puppeteer text selector?
+[] @B how do we invoke async functions?  what does await look like for those?  maybe time to actually read?  goal: remove the separate muhasibPostLogin function.
+[] @B https://github.com/onsi/biloba/issues/2
+[] @B right now polling an element fails if the browser if ollowing redirects.  so i'm using Eventually(b.Location) - instead of just Eventually("#el").Should(b.Exist()).  I think we need a more robust way to ensure biloba.
+[] @B biloba support for gettign "SelectedOption" instead of just "Value" for select inputs (e.g. b.OptionByName(...) instead of value?)
+[] @B ginkgo interrupt should not show the whole stacktrace. it's just too much!
+[] @B add cookie support
+    chromedp.Run(b.Context, chromedp.ActionFunc(func(ctx context.Context) error {
+        return storage.ClearDataForOrigin("*", "all").Do(ctx)
+    }))
+
+    chromedp.Run(b.Context, chromedp.ActionFunc(func(ctx context.Context) error {
+        expr := cdp.TimeSinceEpoch(time.Date(2091, 28, 3, 1, 40, 45, 0, time.UTC))
+        return storage.SetCookies([]*network.CookieParam{{
+            Name:  "rallly-session",
+            Value: "Fe26.2*1*66a5cae1dd8728fc7be37a1a3c485557606e526b16b472329be78168ad4d48c2*Yb_O9pN2K3APF6LXt9S3zg*IZQ_c5aukJzt-AIW__lL19igVhpFMGH9cK0PyFenF-2ti94BgBsLDf325DB2rsKE*3825906104968*a06f3cfe6ef65db1a30b5177cb767c914ca38c8fc3e2456de89d5bea5641611e*6HNCfDeEzgfeQO88IRJ8TfdG5IDzDQtt6WaoGAg5i98~2", Expires: &expr,
+            Domain:   "rallly.co",
+            Path:     "/",
+            HTTPOnly: true,
+            Secure:   true,
+            SameSite: network.CookieSameSiteLax,
+        }}).WithBrowserContextID(b.BrowserContextID()).Do(ctx)
+    }))
+[] @Ω Gomega should have an error returning mode, then tell pohly
+[] @Ω Gomega submatcher description interface and bare-element interface (the former for any sort of matcher that takes a submatcher; the latter specifically for matchers like Consistently etc. that would replace equalMatchersToElements.
+
+
+[] @B JSSelector (is a function that returns one or many nodes)
+[] @B ScrollTo etc.
+[] @B support for esbuild?  or something?  consider the auth_login_scripts tests - what might make them better?
+
+# Long-term Backlog
+- VSCode Extension
+- Ginkgo WebView
+- Suite configuration
+- Suite parallelization
+	- With the special subcase of supporting shared singleton resources

integration/_fixtures/interceptor_sleep_fixture/interceptor_fixture_suite_test.go

@@ -0,0 +1,25 @@
+package main_test
+
+import (
+	"fmt"
+	"os"
+	"os/exec"
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestInterceptorSleepFixture(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "TestInterceptorSleepFixture Suite")
+}
+
+var _ = Describe("Ensuring that ginkgo -p does not hang when output is intercepted", func() {
+	It("ginkgo -p should not hang on this spec", func() {
+		fmt.Fprintln(os.Stdout, "Some STDOUT output")
+		fmt.Fprintln(os.Stderr, "Some STDERR output")
+		cmd := exec.Command("sleep", "60")
+		Ω(cmd.Start()).Should(Succeed())
+	})
+})

integration/output_interceptor_test.go

@@ -2,9 +2,11 @@ package integration_test
 
 import (
 	"os/exec"
+	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/onsi/gomega/gbytes"
 	"github.com/onsi/gomega/gexec"
 )
 
@@ -47,4 +49,20 @@ var _ = Describe("OutputInterceptor", func() {
 			Ω(report.SpecReports[0].CapturedStdOutErr).Should(Equal("CAPTURED OUTPUT A\nCAPTURED OUTPUT B\n"))
 		})
 	})
+
+	Context("ensuring Ginkgo does not hang when a child process does not exit: https://github.com/onsi/ginkgo/issues/1191", func() {
+		BeforeEach(func() {
+			fm.MountFixture("interceptor_sleep")
+		})
+
+		It("exits without hanging", func() {
+			sess := startGinkgo(fm.PathTo("interceptor_sleep"), "--no-color", "--procs=2")
+			Eventually(sess).WithTimeout(time.Second * 15).Should(gexec.Exit(0))
+
+			Ω(sess).Should(gbytes.Say("Captured StdOut/StdErr Output >>"))
+			Ω(sess).Should(gbytes.Say("Some STDOUT output"))
+			Ω(sess).Should(gbytes.Say("Some STDERR output"))
+			Ω(sess).Should(gbytes.Say("<< Captured StdOut/StdErr Output"))
+		})
+	})
 })

internal/output_interceptor_unix.go

@@ -26,6 +26,17 @@ func (impl *dupSyscallOutputInterceptorImpl) CreateStdoutStderrClones() (*os.Fil
 	stdoutCloneFD, _ := unix.Dup(1)
 	stderrCloneFD, _ := unix.Dup(2)
 
+	// Important, set the fds to FD_CLOEXEC to prevent them leaking into childs
+	// https://github.com/onsi/ginkgo/issues/1191
+	flags, err := unix.FcntlInt(uintptr(stdoutCloneFD), unix.F_GETFD, 0)
+	if err == nil {
+		unix.FcntlInt(uintptr(stdoutCloneFD), unix.F_SETFD, flags|unix.FD_CLOEXEC)
+	}
+	flags, err = unix.FcntlInt(uintptr(stderrCloneFD), unix.F_GETFD, 0)
+	if err == nil {
+		unix.FcntlInt(uintptr(stderrCloneFD), unix.F_SETFD, flags|unix.FD_CLOEXEC)
+	}
+
 	// And then wrap the clone file descriptors in files.
 	// One benefit of this (that we don't use yet) is that we can actually write
 	// to these files to emit output to the console even though we're intercepting output