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

Fix system cmd memleak #921

Merged
merged 3 commits into from Oct 17, 2019
Merged

Fix system cmd memleak #921

merged 3 commits into from Oct 17, 2019

Conversation

@catfact
Copy link
Collaborator

@catfact catfact commented Oct 17, 2019

due to a subtlety of the pthreads API, the worker threads created in system_cmd.c were leaking all the memory of the child processes they spawned.

(the child process in latest version being a big ol' find that ate up a rather shocking 10MB a pop with a goodly number of scripts installed. [um, actually that's not quite right. it's mapped but unwired, or something.])

detaching the thread fixes this, it seems.

also uncrustified that file, because someone has their vim set on 2space again. :)

@catfact catfact requested a review from tehn Oct 17, 2019
@catfact
Copy link
Collaborator Author

@catfact catfact commented Oct 17, 2019

last commit is a bit silly, but it reduces mem usage of the subcommand by about 25% compared to find (AFAICT), and is probably faster

@tehn
tehn approved these changes Oct 17, 2019
Copy link
Member

@tehn tehn left a comment

thank you for spotting this. i clearly need to learn more about pthread.

@tehn tehn merged commit b9a809b into monome:master Oct 17, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tehn
Copy link
Member

@tehn tehn commented Oct 17, 2019

@catfact i suspect this is the problem:

#define CMD_BUFFER 8192

for super super long folder listings perhaps it's just running out of memory and plowing over

@catfact
Copy link
Collaborator Author

@catfact catfact commented Oct 18, 2019

oh, ha. indeed.

i'll do another round of cleanup on this. strcat is basically pure evil...

@tehn
Copy link
Member

@tehn tehn commented Oct 18, 2019

oy oy oy i apologize

@catfact catfact deleted the catfact:fix-system-cmd-memleak branch Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants