Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

treefmt --stdin hangs if no filename is specified #406

Closed
jfly opened this issue Sep 14, 2024 · 0 comments · Fixed by #407 or #409
Closed

treefmt --stdin hangs if no filename is specified #406

jfly opened this issue Sep 14, 2024 · 0 comments · Fixed by #407 or #409
Labels
bug Something isn't working

Comments

@jfly
Copy link
Collaborator

jfly commented Sep 14, 2024

This command hangs indefinitely:

treefmt --stdin

SIGTERM doesn't stop it, I have to send a SIGKILL.

Expected behavior

treefmt should never hang indefinitely. In the specific case above, I'd expect to see this error message:

only one path should be specified when using the --stdin flag

System information

I'm using the latest version of treefmt (at time of writing):

$ treefmt --version
treefmt v2.0.5

Additional context

  • I dug into this, and we appear to be getting stuck here waiting for some goroutines (am I using that word correctly? sorry, total go noob here) to terminate: https://github.com/numtide/treefmt/blob/v2.0.5/cli/format.go#L196
  • Specifically, the applyFormatters goroutine is stuck forever in this for loop reading from f.filesCh
  • The contract seems to be that the walkFilesystem goroutine is responsible for closing f.filesCh, but it exited here before it sets up a defer to close the channel at exit.
  • IIUC, the fix here is as simple as moving the defer close(f.filesCh) to the very top of the walkFilesystem goroutine, so there's no chance for it to return without closing f.filesCh. This is a very simple change, so I'll send in a PR with the change.
@jfly jfly added the bug Something isn't working label Sep 14, 2024
jfly added a commit to jfly/treefmt that referenced this issue Sep 14, 2024
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 numtide#406
jfly added a commit to jfly/treefmt that referenced this issue Sep 14, 2024
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 numtide#406
brianmcgee added a commit that referenced this issue Sep 14, 2024
Closes #406

Signed-off-by: Brian McGee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
1 participant