Skip to content

Commit

Permalink
Avoid hanging: ensure we always close f.filesCh
Browse files Browse the repository at this point in the history
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 #406
  • Loading branch information
jfly committed Sep 14, 2024
1 parent f927a83 commit d7c1064
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 5 deletions.
6 changes: 3 additions & 3 deletions cli/format.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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 {
Expand Down
12 changes: 10 additions & 2 deletions cli/format_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down

0 comments on commit d7c1064

Please sign in to comment.