Set the rlimit for NOFILE to 1024 to reduce the subprocess cleanup #3101
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We should never open more than 1024 file descriptors anyway, and under some situations, namely running as root or in docker, would give us huge allowances. This then results in a huge, unneeded, cleanup for subprocesses, which we use a lot. Fixes ElementsProject#2977
We were compiling them from source, but then not adding them in the final docker image. This just drops the source based ones in favor of system provided ones. Fixes ElementsProject#3074
|
cool thanks, will release a new version of current release with this patch today, |
ZmnSCPxj
reviewed
Sep 26, 2019
|
|
||
| /*~ Make sure that we limit ourselves to something reasonable. Modesty | ||
| * is a virtue. */ | ||
| setrlimit(RLIMIT_NOFILE, &nofile); |
There was a problem hiding this comment.
Would it be moderately useful to have this as a configurable option? Or do we need to do this setrlimit ASAP before any command-line processing?
There was a problem hiding this comment.
No we can make it configurable, but for now this limits us to ~1000 peers, which should be an ok guess for now.
For non-root users we're unlikely to even have a higher limit since it requires system configuration changes. If this becomes an issue we can definitely introduce the option.
Closed
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should limit the overhead introduced by 25ddb80, specifically the loop calling syscalls here:
lightning/ccan/ccan/pipecmd/pipecmd.c
Lines 141 to 145 in 25ddb80
In some environments this would be way larger (e.g., running as root in docker), resulting in way more
close()calls than intended. This PR limits the number of concurrent open fds, so we have to check and close a smaller range.Ping @NicolasDorier
Fixes #2977
Fixes #3074