{"thread":{"id":"40628","subject":"[PATCH 1/2] run-command: factor out child_process_clear()","startedAt":"2015-10-24T12:11:27Z","lastAt":"2015-10-27T21:31:48Z","messageCount":7,"participants":["René Scharfe","Junio C Hamano","Stefan Beller","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"272163","messageId":"562B756F.1020305@web.de","threadId":"40628","inReplyTo":null,"subject":"[PATCH 1/2] run-command: factor out child_process_clear()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-24T12:11:27Z","receivedAt":"2015-10-24T12:11:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid duplication by moving the code to release allocated memory for\narguments and environment to its own function, child_process_clear().\nExport it to provide a counterpart to child_process_init().\n\nSigned-off-by: Rene Scharfe \n---\n Documentation/technical/api-run-command.txt | 7 +++++++\n run-command.c | 15 +++++++++------\n run-command.h | 1 +\n 3 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt\nindex a9fdb45..8bf3e37 100644\n--- a/Documentation/technical/api-run-command.txt\n+++ b/Documentation/technical/api-run-command.txt\n@@ -46,6 +46,13 @@ Functions\n \tThe argument dir corresponds the member .dir. The argument env\n \tcorresponds to the member .env.\n \n+`child_process_clear`::\n+\n+\tRelease the memory associated with the struct child_process.\n+\tMost users of the run-command API don't need to call this\n+\tfunction explicitly because `start_command` invokes it on\n+\tfailure and `finish_command` calls it automatically already.\n+\n The functions above do the following:\n \n . If a system call failed, errno is set and -1 is returned. A diagnostic\ndiff --git a/run-command.c b/run-command.c\nindex e17e456..13fa452 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -11,6 +11,12 @@ void child_process_init(struct child_process *child)\n \targv_array_init(&child->env_array);\n }\n \n+void child_process_clear(struct child_process *child)\n+{\n+\targv_array_clear(&child->args);\n+\targv_array_clear(&child->env_array);\n+}\n+\n struct child_to_clean {\n \tpid_t pid;\n \tstruct child_to_clean *next;\n@@ -327,8 +333,7 @@ int start_command(struct child_process *cmd)\n fail_pipe:\n \t\t\terror(\"cannot create %s pipe for %s: %s\",\n \t\t\t\tstr, cmd->argv[0], strerror(failed_errno));\n-\t\t\targv_array_clear(&cmd->args);\n-\t\t\targv_array_clear(&cmd->env_array);\n+\t\t\tchild_process_clear(cmd);\n \t\t\terrno = failed_errno;\n \t\t\treturn -1;\n \t\t}\n@@ -513,8 +518,7 @@ fail_pipe:\n \t\t\tclose_pair(fderr);\n \t\telse if (cmd->err)\n \t\t\tclose(cmd->err);\n-\t\targv_array_clear(&cmd->args);\n-\t\targv_array_clear(&cmd->env_array);\n+\t\tchild_process_clear(cmd);\n \t\terrno = failed_errno;\n \t\treturn -1;\n \t}\n@@ -540,8 +544,7 @@ fail_pipe:\n int finish_command(struct child_process *cmd)\n {\n \tint ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);\n-\targv_array_clear(&cmd->args);\n-\targv_array_clear(&cmd->env_array);\n+\tchild_process_clear(cmd);\n \treturn ret;\n }\n \ndiff --git a/run-command.h b/run-command.h\nindex 5428b04..12bb26c 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -47,6 +47,7 @@ struct child_process {\n \n #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }\n void child_process_init(struct child_process *);\n+void child_process_clear(struct child_process *);\n \n int start_command(struct child_process *);\n int finish_command(struct child_process *);\n-- \n2.6.2\n"},{"id":"272164","messageId":"562B7838.8050009@web.de","threadId":"40628","inReplyTo":"562B756F.1020305@web.de","subject":"[PATCH 2/2] daemon: plug memory leak","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-24T12:23:20Z","receivedAt":"2015-10-24T12:23:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Call child_process_clear() when a child ends to release the memory\nallocated for its environment. This is necessary because unlike all\nother users of start_command() we don't call finish_command(), which\nwould have taken care of that for us.\n\nThis leak was introduced by f063d38b (daemon: use cld->env_array\nwhen re-spawning).\n\nSigned-off-by: Rene Scharfe \n---\n daemon.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/daemon.c b/daemon.c\nindex 56679a1..be70cd4 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -802,6 +802,7 @@ static void check_dead_children(void)\n \t\t\t/* remove the child */\n \t\t\t*cradle = blanket->next;\n \t\t\tlive_children--;\n+\t\t\tchild_process_clear(&blanket->cld);\n \t\t\tfree(blanket);\n \t\t} else\n \t\t\tcradle = &blanket->next;\n-- \n2.6.2\n"},{"id":"272252","messageId":"xmqqziz5h3n5.fsf@gitster.mtv.corp.google.com","threadId":"40628","inReplyTo":"562B756F.1020305@web.de","subject":"Re: [PATCH 1/2] run-command: factor out child_process_clear()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-26T18:43:26Z","receivedAt":"2015-10-26T18:43:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Avoid duplication by moving the code to release allocated memory for\n> arguments and environment to its own function, child_process_clear().\n> Export it to provide a counterpart to child_process_init().\n>\n> Signed-off-by: Rene Scharfe \n> ---\n\nHmm, is this _deinit() Stefan added to his series recently?\n\n> Documentation/technical/api-run-command.txt | 7 +++++++\n> run-command.c | 15 +++++++++------\n> run-command.h | 1 +\n> 3 files changed, 17 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt\n> index a9fdb45..8bf3e37 100644\n> --- a/Documentation/technical/api-run-command.txt\n> +++ b/Documentation/technical/api-run-command.txt\n> @@ -46,6 +46,13 @@ Functions\n> \tThe argument dir corresponds the member .dir. The argument env\n> \tcorresponds to the member .env.\n> \n> +`child_process_clear`::\n> +\n> +\tRelease the memory associated with the struct child_process.\n> +\tMost users of the run-command API don't need to call this\n> +\tfunction explicitly because `start_command` invokes it on\n> +\tfailure and `finish_command` calls it automatically already.\n> +\n> The functions above do the following:\n> \n> . If a system call failed, errno is set and -1 is returned. A diagnostic\n> diff --git a/run-command.c b/run-command.c\n> index e17e456..13fa452 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -11,6 +11,12 @@ void child_process_init(struct child_process *child)\n> \targv_array_init(&child->env_array);\n> }\n> \n> +void child_process_clear(struct child_process *child)\n> +{\n> +\targv_array_clear(&child->args);\n> +\targv_array_clear(&child->env_array);\n> +}\n> +\n> struct child_to_clean {\n> \tpid_t pid;\n> \tstruct child_to_clean *next;\n> @@ -327,8 +333,7 @@ int start_command(struct child_process *cmd)\n> fail_pipe:\n> \t\t\terror(\"cannot create %s pipe for %s: %s\",\n> \t\t\t\tstr, cmd->argv[0], strerror(failed_errno));\n> -\t\t\targv_array_clear(&cmd->args);\n> -\t\t\targv_array_clear(&cmd->env_array);\n> +\t\t\tchild_process_clear(cmd);\n> \t\t\terrno = failed_errno;\n> \t\t\treturn -1;\n> \t\t}\n> @@ -513,8 +518,7 @@ fail_pipe:\n> \t\t\tclose_pair(fderr);\n> \t\telse if (cmd->err)\n> \t\t\tclose(cmd->err);\n> -\t\targv_array_clear(&cmd->args);\n> -\t\targv_array_clear(&cmd->env_array);\n> +\t\tchild_process_clear(cmd);\n> \t\terrno = failed_errno;\n> \t\treturn -1;\n> \t}\n> @@ -540,8 +544,7 @@ fail_pipe:\n> int finish_command(struct child_process *cmd)\n> {\n> \tint ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);\n> -\targv_array_clear(&cmd->args);\n> -\targv_array_clear(&cmd->env_array);\n> +\tchild_process_clear(cmd);\n> \treturn ret;\n> }\n> \n> diff --git a/run-command.h b/run-command.h\n> index 5428b04..12bb26c 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -47,6 +47,7 @@ struct child_process {\n> \n> #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }\n> void child_process_init(struct child_process *);\n> +void child_process_clear(struct child_process *);\n> \n> int start_command(struct child_process *);\n> int finish_command(struct child_process *);\n"},{"id":"272255","messageId":"CAGZ79kYopHL5D_7+bJV2P9GeKDHufPDBTo3a8bRL=4B3OmTGjg@mail.gmail.com","threadId":"40628","inReplyTo":"xmqqziz5h3n5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] run-command: factor out child_process_clear()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-26T19:23:23Z","receivedAt":"2015-10-26T19:23:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 26, 2015 at 11:43 AM, Junio C Hamano wrote:\n> René Scharfe writes:\n>\n>> Avoid duplication by moving the code to release allocated memory for\n>> arguments and environment to its own function, child_process_clear().\n>> Export it to provide a counterpart to child_process_init().\n>>\n>> Signed-off-by: Rene Scharfe \n>> ---\n>\n> Hmm, is this _deinit() Stefan added to his series recently?\n\nYes. Although you (Junio) take credit for actually using it in these\nplaces, too. :)\n"},{"id":"272257","messageId":"20151026194753.GA25534@sigill.intra.peff.net","threadId":"40628","inReplyTo":"562B7838.8050009@web.de","subject":"Re: [PATCH 2/2] daemon: plug memory leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-26T19:47:53Z","receivedAt":"2015-10-26T19:47:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 24, 2015 at 02:23:20PM +0200, René Scharfe wrote:\n\n> Call child_process_clear() when a child ends to release the memory\n> allocated for its environment. This is necessary because unlike all\n> other users of start_command() we don't call finish_command(), which\n> would have taken care of that for us.\n> \n> This leak was introduced by f063d38b (daemon: use cld->env_array\n> when re-spawning).\n\nI agree this is a leak we need to address, but I find the solution a\nlittle unsatisfactory (but tl;dr, it's probably the best we can do).\n\nIt would be nice if we could call finish_command() here to do the\nwait(). But it does not know about WNOHANG, so we would have to\nintroduce a new helper anyway.\n\nHowever, the whole check_dead_children function is accidentally\nquadratic, and should probably be refactored in general. It loops over\nthe list of children, calling `waitpid(pid, WNOHANG)` on each. So if `n`\nchildren connect at one time, we make `O(n^2)` waitpid calls.\n\nI have patches to instead loop over waitpid(-1, WNOHANG) until there are\nno dead children left (and use a hash to go from pid to each \"struct\nchild\").\n\nBut the reason I haven't posted them is that I'm somewhat of the opinion\nthat this child-list can go away altogether. The main use is in\nenforcing the max_connections variable. Just enforcing that can be done\nwith a counter, but we do something much weirder: when we get a new\nconnection, we try to kill an _existing_ child by finding the first\nclient with multiple connections, killing that, sleeping for a second\n(!), and then if that killed something, giving the slot to the new\nconnection.\n\nThis has a few problems:\n\n 1. Under non-malicious heavy load, you probably want to reject the new\n request rather than killing an existing one. If you kill a fetch\n that is 99% completed, that client is likely to come back and fetch\n again, and you've just thrown away all of the effort (both CPU and\n bandwidth) put into the failed clone. You haven't spent anything on\n the incoming connection, so you'd much rather they go away and come\n back in a moment.\n\n 2. I think the kill_some_child algorithm was meant to enforce\n fairness so that a single IP cannot monopolize all of your slots.\n I'm not sure that's realistic for a few reasons:\n\n a. Real bad guys will just hit you with a DDoS from many IPs.\n\n b. Real sites have load-balancing in front of git-daemon, and the\n client IPs may not be meaningful.\n\n c. The duplicate selection is pretty naive. A bad guy with tons\n\t of duplicate connections is no more likely to be killed than a\n\t normal client with exactly two connections (it actually\n\t depends solely on when the latest connection from either came\n\t in). So I suspect a determine attacker with a single IP could\n\t still present a problem.\n\n 3. Calling sleep() in the server parent process is a great way to kill\n performance. :)\n\nOf course my view is somewhat skewed by running a really big site with\nload balancers and such (and we have long set --max-connections high\nenough that it has never been hit). Maybe it's more realistic protection\nfor a \"normal\" site.\n\nBut my inclination would be to drop kill_some_child entirely in favor of\nsimply rejecting new connections that are over the limit, and getting\nrid of the child-list entirely (and calling waitpid only to reap the\nzombie PIDs).\n\nI guess we'd still need something like child_process_clear(), though. We\ncould just run it immediately after spawning the child, rather than when\nwe cleaned it up. So in that sense this series is the first incremental\nstep toward that future, anyway.\n\n-Peff\n"},{"id":"272347","messageId":"562FECCA.6080703@web.de","threadId":"40628","inReplyTo":"CAGZ79kYopHL5D_7+bJV2P9GeKDHufPDBTo3a8bRL=4B3OmTGjg@mail.gmail.com","subject":"Re: [PATCH 1/2] run-command: factor out child_process_clear()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-27T21:29:46Z","receivedAt":"2015-10-27T21:29:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.10.2015 um 20:23 schrieb Stefan Beller:\n> On Mon, Oct 26, 2015 at 11:43 AM, Junio C Hamano wrote:\n>> René Scharfe writes:\n>>\n>>> Avoid duplication by moving the code to release allocated memory for\n>>> arguments and environment to its own function, child_process_clear().\n>>> Export it to provide a counterpart to child_process_init().\n>>>\n>>> Signed-off-by: Rene Scharfe \n>>> ---\n>>\n>> Hmm, is this _deinit() Stefan added to his series recently?\n>\n> Yes. Although you (Junio) take credit for actually using it in these\n> places, too. :)\n\nOh, and it's already in next. I still like _clear() better (as in \n{argv,object,ref,sha1}_array_clear(), or string_list_clear(), or ...) \nthan _deinit(), which seems to have been introduced by vcs-svn and isn't \na real word. I'll rebase my patches on top of this series, but probably \nnot before the weekend.\n\nRené\n"},{"id":"272348","messageId":"CAGZ79kbCNnnWQapFjj=sA5+HPK2hpAu4=8LomNnOErM9+_QBjg@mail.gmail.com","threadId":"40628","inReplyTo":"562FECCA.6080703@web.de","subject":"Re: [PATCH 1/2] run-command: factor out child_process_clear()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-27T21:31:48Z","receivedAt":"2015-10-27T21:31:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 27, 2015 at 2:29 PM, René Scharfe wrote:\n> Am 26.10.2015 um 20:23 schrieb Stefan Beller:\n>>\n>> On Mon, Oct 26, 2015 at 11:43 AM, Junio C Hamano \n>> wrote:\n>>>\n>>> René Scharfe writes:\n>>>\n>>>> Avoid duplication by moving the code to release allocated memory for\n>>>> arguments and environment to its own function, child_process_clear().\n>>>> Export it to provide a counterpart to child_process_init().\n>>>>\n>>>> Signed-off-by: Rene Scharfe \n>>>> ---\n>>>\n>>>\n>>> Hmm, is this _deinit() Stefan added to his series recently?\n>>\n>>\n>> Yes. Although you (Junio) take credit for actually using it in these\n>> places, too. :)\n>\n>\n> Oh, and it's already in next. I still like _clear() better (as in\n> {argv,object,ref,sha1}_array_clear(), or string_list_clear(), or ...) than\n> _deinit(), which seems to have been introduced by vcs-svn and isn't a real\n> word. I'll rebase my patches on top of this series, but probably not before\n> the weekend.\n>\n\nThanks for the coming cleanup. :)\nAs you know naming things is hard.\n\n> René\n>\n"}]}