Fix memory leak in cmd parser #404
Merged
Conversation
This file contains 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
lib/iobroker.c
Outdated
| /* add sane max limit, if ulimit is set to unlimited or a very high value we | ||
| * don't want to waste memory for nothing */ | ||
| if (iobs->max_fds > 100000) | ||
| iobs->max_fds = 100000; |
There was a problem hiding this comment.
Could we make this configurable just in case someone wants a different value?
There was a problem hiding this comment.
i thought about it as well. It is usually limited by ulimit -n. So if you want a smaller value, set a sane ulimit.
Not sure if a config option is required, maybe a compile time constant?
There was a problem hiding this comment.
Yeah I guess it is mainly for larger values on would nee to change it.
There shouldn't be much need to increase it above this limit I suppose, but a configuration option somewhere (compiler/runtime) seems nicer than a "magic" constant.
Usually ulimit -n is has sane values somewhere between 1000 and 10000. Some container managers like docker set higher values, so naemon might end up with ulimit of millions which just wastes memory. If a single worker hits the open file limit, simply start more worker.
sni
force-pushed
the
fix_memory_leak_in_cmd_parser
branch
from
f41db3d to
a80eccf
Compare
|
i removed the magic... :-) |
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.
Fixing two issues here. If you like separate PRs, just tell me :-)
First one fixes a memory leak in the cmd parser which makes the worker processes grow which
then leads to more cpu usage during all the forking (along with the wasted memory)
The second came to my attention while searching the leak, those two allocations where by far the biggest ones on my test system and both simply depend on the maximum number of open files.
I added a high enough max value to be hopefully future proof without wasting useless memory.
I don't think any naemon worker will ever sanely use more than 100k open file handles. I
considered adding a log entry or another warning. But at that point, there is no access to the log and stderr/stdout is already closed.
With those patches, memory consumtion seems stable.