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

The latest version returns an error: too many open files #208

Closed
screwyprof opened this issue Mar 17, 2022 · 10 comments · Fixed by #211
Closed

The latest version returns an error: too many open files #208

screwyprof opened this issue Mar 17, 2022 · 10 comments · Fixed by #211

Comments

@screwyprof
Copy link

screwyprof commented Mar 17, 2022

After updating to the latest version I get the following error:
open <file-name>: too many open files

I've got the following target in my Makefile:

LOCAL_PACKAGES="gitlab.com/cool-company/"
GO_FILES = $(shell find . -name "*.go" | grep -v vendor | uniq)

fmt: ## format code
	@gofumpt -l -w $(GO_FILES)
	@gci -w -l $(LOCAL_PACKAGES) $(GO_FILES)	

Interestingly enough gci doesn't have this problem

@mvdan
Copy link
Owner

mvdan commented Mar 17, 2022

What system are you on? If it is unix-like, can you show the output of ulimit -S -n and ulimit -H -n?

@screwyprof
Copy link
Author

screwyprof commented Mar 17, 2022

I'm on mac Big Sur. Don't know the previous limit as I temporary fixed it by setting the ulimit -n 10240.

I've tried to use find + xargs, but it doesn't help:

❯ find . -path ./vendor -prune -o -name \*.go -print0 | xargs -0 gofumpt -w -l
 ulimit -S -n
256
❯ ulimit -H -n
unlimited

@mvdan
Copy link
Owner

mvdan commented Mar 17, 2022

What if instead you use a directory, like gofumpt -l -w .? It could be that the problem is supplying all the files at once rather than letting gofumpt walk the directories to find Go files itself.

@mvdan
Copy link
Owner

mvdan commented Mar 17, 2022

Also, can you reproduce if you use gofmt from Go 1.18 instead of gofumpt?

@screwyprof
Copy link
Author

screwyprof commented Mar 17, 2022

gofmt doesn't have this issue. I'm using 'go 1.17'.

❯ gofumpt -l -w .
open handler/uuid.go: too many open files
open <file-name>: too many open files
...
fcntl <file-name>: too many open files
❯ sysctl -a | grep maxfiles
kern.maxfiles: 122880
kern.maxfilesperproc: 6144

@mvdan
Copy link
Owner

mvdan commented Mar 17, 2022

Please upgrade to Go 1.18 and compare gofumpt to gofmt from that version, as they should be opening the same amount of files with the same amount of parallelism. gofumpt is a modified version of gofmt from Go 1.18, and 1.18 is the version that made gofmt parallel. So if gofumpt is opening too many files, gofmt from 1.18 should be doing the same, and we should further investigate the issue with upstream.

@mvdan
Copy link
Owner

mvdan commented Mar 17, 2022

I should also note I tried to reproduce with ulimit -n 256; gofumpt **/*.go in $(go env GOROOT)/src and I couldn't get a failure.

@screwyprof
Copy link
Author

screwyprof commented Mar 17, 2022

I cannot simply update to go1.18 in my environment it maybe break some stuff related to deps management as they've deprecated some features. Not to mention that some colleagues of mine already complained about new version screwing up some old packages for some reason. Do you by any chance have a ready-to use Dockerfile?

I am not sure, but you may not have that much files in your root path which breaks it. But really it could be anything else. I've also noticed that macOS doesn't simply allow to set the ulimit to a higher value. The last comment on superuser.com suggest turning on a performance mode.

Update: mvdan.cc/gofumpt@v0.2.1 works great for me atm.

@screwyprof
Copy link
Author

screwyprof commented Mar 17, 2022

I've created a simple Dockerfile to test it with go1.18

❯ cat Dockerfile
FROM golang:1.18 AS build
RUN go install mvdan.cc/gofumpt@latest

FROM scratch
WORKDIR /root/
COPY --from=build /go/bin/gofumpt /gofumpt

# Set runner as a non existent user
USER 65534:65534
ENTRYPOINT [ "/gofumpt" ]
CMD ["--help"]

Then built the image:

docker build -t gofumpt .

Running it like in the example bellow works:

docker run -it --rm -v $(pwd):/root gofumpt -w -l .

Thanks for the trouble!

@mvdan
Copy link
Owner

mvdan commented Mar 18, 2022

I was finally able to reproduce by formatting 10k tiny Go files. The problem does not appear to be there with gofmt, so this is definitely something we're doing wrong.

Thanks for bearing with me, as this one was a bit tricky to repro.

mvdan added a commit that referenced this issue Mar 18, 2022
We would call "go mod edit -json" for each Go file we formatted,
as each file may be in a different directory,
and thus inside a different module.

This could cause us to run into open file limits,
because spawning a child process and grabbing its output opens files of
its own such as named pipes.
The added test shows this with a limit of 256 and 10k tiny Go files:

	--- FAIL: TestWithLowOpenFileLimit (0.30s)
		ulimit_unix_test.go:82: writing 10000 tiny Go files
		ulimit_unix_test.go:104: running with 1 paths
		ulimit_unix_test.go:104: running with 10000 paths
		ulimit_unix_test.go:112:
			error:
			  got non-nil error
			comment:
			  stderr:
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/014.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/017.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/000.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/019.go: too many open files

Instead, only call "go mod edit -json" once per directory,
and do it in the main thread to reduce its parallelism.
Also make it grab fdSem as well, for good measure.

This may not be a complete fix, as we're not sure how many files are
open by an exec.Command.Output call. However, we are no longer able to
reproduce a failure, so leave that as a TODO.

Fixes #208.
mvdan added a commit that referenced this issue Mar 18, 2022
We would call "go mod edit -json" for each Go file we formatted,
as each file may be in a different directory,
and thus inside a different module.

This could cause us to run into open file limits,
because spawning a child process and grabbing its output opens files of
its own such as named pipes.
The added test shows this with a limit of 256 and 10k tiny Go files:

	--- FAIL: TestWithLowOpenFileLimit (0.30s)
		ulimit_unix_test.go:82: writing 10000 tiny Go files
		ulimit_unix_test.go:104: running with 1 paths
		ulimit_unix_test.go:104: running with 10000 paths
		ulimit_unix_test.go:112:
			error:
			  got non-nil error
			comment:
			  stderr:
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/014.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/017.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/000.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/019.go: too many open files

Instead, only call "go mod edit -json" once per directory,
and do it in the main thread to reduce its parallelism.
Also make it grab fdSem as well, for good measure.

This may not be a complete fix, as we're not sure how many files are
open by an exec.Command.Output call. However, we are no longer able to
reproduce a failure, so leave that as a TODO.

Fixes #208.
mvdan added a commit that referenced this issue Mar 18, 2022
We would call "go mod edit -json" for each Go file we formatted,
as each file may be in a different directory,
and thus inside a different module.

However, the first mistake is that we always ran the command in the
directory where gofumpt is run, not the directory containing each Go
file. Our tests weren't strict enough to catch this; now
octal-literal.txt is, via its run of gofmt before it calls cd.

The second mistake, and a pretty embarrassing one, is that since v0.3.0
made gofumpt concurrent, it has been racy in how it writes to globals
like langVersion from multiple goroutines. octal-literals.txt now tests
for this by adding a nested Go module.

Finally, we could also run into open file limits,
because spawning a child process and grabbing its output opens files of
its own such as named pipes.
The added test shows this with a limit of 256 and 10k tiny Go files:

	--- FAIL: TestWithLowOpenFileLimit (0.30s)
		ulimit_unix_test.go:82: writing 10000 tiny Go files
		ulimit_unix_test.go:104: running with 1 paths
		ulimit_unix_test.go:104: running with 10000 paths
		ulimit_unix_test.go:112:
			error:
			  got non-nil error
			comment:
			  stderr:
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/014.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/017.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/000.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/019.go: too many open files

Instead, only call "go mod edit -json" once per directory,
and do it in the main thread to reduce its parallelism.
Also make it grab fdSem as well, for good measure.

This may not be a complete fix, as we're not sure how many files are
open by an exec.Command.Output call. However, we are no longer able to
reproduce a failure, so leave that as a TODO.

Fixes #208.
mvdan added a commit that referenced this issue Mar 18, 2022
We would call "go mod edit -json" for each Go file we formatted,
as each file may be in a different directory,
and thus inside a different module.

However, the first mistake is that we always ran the command in the
directory where gofumpt is run, not the directory containing each Go
file. Our tests weren't strict enough to catch this; now
octal-literal.txt is, via its run of gofmt before it calls cd.

The second mistake, and a pretty embarrassing one, is that since v0.3.0
made gofumpt concurrent, it has been racy in how it writes to globals
like langVersion from multiple goroutines. octal-literals.txt now tests
for this by adding a nested Go module.

Finally, we could also run into open file limits,
because spawning a child process and grabbing its output opens files of
its own such as named pipes.
The added test shows this with a limit of 256 and 10k tiny Go files:

	--- FAIL: TestWithLowOpenFileLimit (0.30s)
		ulimit_unix_test.go:82: writing 10000 tiny Go files
		ulimit_unix_test.go:104: running with 1 paths
		ulimit_unix_test.go:104: running with 10000 paths
		ulimit_unix_test.go:112:
			error:
			  got non-nil error
			comment:
			  stderr:
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/014.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/017.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/000.go: too many open files
			  open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/019.go: too many open files

Instead, only call "go mod edit -json" once per directory,
and do it in the main thread to reduce its parallelism.
Also make it grab fdSem as well, for good measure.

This may not be a complete fix, as we're not sure how many files are
open by an exec.Command.Output call. However, we are no longer able to
reproduce a failure, so leave that as a TODO.

Fixes #208.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants