From d7c106480ec309cf66b9dfdfd4cfc73679b2d22d Mon Sep 17 00:00:00 2001 From: Jeremy Fleischman Date: Sat, 14 Sep 2024 12:02:55 -0700 Subject: [PATCH] Avoid hanging: ensure we always close `f.filesCh` The contract seems to be that the `walkFilesystem` goroutine is responsible for closing `f.filesCh`, but before this change, there were codepaths that could result in the gorouting exiting without closing `f.filesCh`. That shouldn't be possible anymore, so long as we keep this statement at the top of the function =) This fixes https://github.com/numtide/treefmt/issues/406 --- cli/format.go | 6 +++--- cli/format_test.go | 12 ++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/cli/format.go b/cli/format.go index 93593bbc..edc8762f 100644 --- a/cli/format.go +++ b/cli/format.go @@ -201,6 +201,9 @@ func (f *Format) Run() (err error) { func (f *Format) walkFilesystem(ctx context.Context) func() error { return func() error { + // close the files channel when we're done walking the file system + defer close(f.filesCh) + eg, ctx := errgroup.WithContext(ctx) pathsCh := make(chan string, BatchSize) @@ -261,9 +264,6 @@ func (f *Format) walkFilesystem(ctx context.Context) func() error { return fmt.Errorf("failed to create walker: %w", err) } - // close the files channel when we're done walking the file system - defer close(f.filesCh) - // if no cache has been configured, or we are processing from stdin, we invoke the walker directly if f.NoCache || f.Stdin { return walker.Walk(ctx, func(file *walk.File, err error) error { diff --git a/cli/format_test.go b/cli/format_test.go index 303cbfcc..72540277 100644 --- a/cli/format_test.go +++ b/cli/format_test.go @@ -604,11 +604,19 @@ func TestStdIn(t *testing.T) { os.Stdin = prevStdIn }) - // + // omit the required filename parameter contents := `{ foo, ... }: "hello"` os.Stdin = test.TempFile(t, "", "stdin", &contents) + // we get an error about the missing filename parameter. + out, err := cmd(t, "-C", tempDir, "--allow-missing-formatter", "--stdin") + as.EqualError(err, "only one path should be specified when using the --stdin flag") + as.Equal("", string(out)) - out, err := cmd(t, "-C", tempDir, "--allow-missing-formatter", "--stdin", "test.nix") + // now pass along the filename parameter + contents = `{ foo, ... }: "hello"` + os.Stdin = test.TempFile(t, "", "stdin", &contents) + + out, err = cmd(t, "-C", tempDir, "--allow-missing-formatter", "--stdin", "test.nix") as.NoError(err) assertStats(t, as, 1, 1, 1, 1)