{"thread":{"id":"2460","subject":"file descriptor leak? or expected behavior?","startedAt":"2005-11-11T22:58:39Z","lastAt":"2005-11-13T03:37:13Z","messageCount":7,"participants":["Becky Bruce","Petr Baudis","Junio C Hamano","Nick Hengeveld"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"11634","messageId":"dd9dee136a573d72fc7332373cfd8ac1@freescale.com","threadId":"2460","inReplyTo":null,"subject":"file descriptor leak? or expected behavior?","fromName":"Becky Bruce","fromEmail":"becky.bruce@freescale.com","sentAt":"2005-11-11T22:58:39Z","receivedAt":"2005-11-11T22:58:39Z","isPatch":false,"sender":{"key":"becky.bruce@freescale.com","avatar":null},"body":"Folks,\n\nMy apologies if this is a known issue/question - I've searched the list \nand haven't found anything about this, but given the volume of traffic, \nit's easy to miss things.....\n\nI grabbed 0.99.9g this morning, and tried to clone Paul Mackerras' \nlinux merge tree. The clone failed and reported errors in http-fetch \nwith a bunch of messages of the form:\n\nerror: Couldn't create temporary file \n.git/objects/04/48fa7de8a416a48cd1977f29858be54e67c078.temp for .git\n/objects/04/48fa7de8a416a48cd1977f29858be54e67c078: Error 24: Too many \nopen files\n\nI did some experimenting, and it looks like this crops up somewhere \nbetween git versions 0.99.8f and 0.99.9a. My question is, is git \nexpected to try to open huge numbers of files, or is this a fd leak? I \ncranked up my ulimit, and am still unable to successfully clone this \ntree, although it fails differently (tail end of output....):\n\nprogress: 22 objects, 56146 bytes\nAlso look at \nhttp://www.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git/\nGetting pack list\nerror: The requested URL returned error: 404\nGetting pack list\nGetting index for pack e0d76ffe354ef5665028a6cb4506ea902f72e1d0\nGetting pack e0d76ffe354ef5665028a6cb4506ea902f72e1d0\nwhich contains 5014bfa48ac169e0748e1e9651897788feb306dc\nprogress: 1322 objects, 5736795 bytes\ncg-fetch: objects fetch failed\ncg-clone: fetch failed\n\n\nThe command I ran, and the tree I tried to clone are:\n\n > cg-clone \nhttp://www.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc-merge.git \nlinux-2.6.paul\n\nCheers,\n-B\n"},{"id":"11635","messageId":"e39c6bdc9e3f264e1248f937d9509c51@freescale.com","threadId":"2460","inReplyTo":"dd9dee136a573d72fc7332373cfd8ac1@freescale.com","subject":"Re: file descriptor leak? or expected behavior?","fromName":"Becky Bruce","fromEmail":"becky.bruce@freescale.com","sentAt":"2005-11-11T23:22:52Z","receivedAt":"2005-11-11T23:22:52Z","isPatch":false,"sender":{"key":"becky.bruce@freescale.com","avatar":null},"body":"\nOn Nov 11, 2005, at 4:58 PM, Becky Bruce wrote:\n>\n> error: Couldn't create temporary file\n> .git/objects/04/48fa7de8a416a48cd1977f29858be54e67c078.temp for .git\n> /objects/04/48fa7de8a416a48cd1977f29858be54e67c078: Error 24: Too many\n> open files\n\n\nBy the way, in case this looks funny to anyone, this isn't the default \nmessage - I added the \"Error 24\" part because I'm used to looking at \nerror numbers.\n\nThis is what the message looks like from an unmodified git:\n\nerror: Couldn't create temporary file \n.git/objects/a0/7e2c9307fa338896ecca300dc88033a8922885.temp for \n.git/objects/a0/7e2c9307fa338896ecca300dc88033a8922885: Too many open \nfiles\n\n\nCheers,\nB\n"},{"id":"11639","messageId":"20051111235516.GY30496@pasky.or.cz","threadId":"2460","inReplyTo":"dd9dee136a573d72fc7332373cfd8ac1@freescale.com","subject":"[PATCH] Fix bunch of fd leaks in http-fetch","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2005-11-11T23:55:16Z","receivedAt":"2005-11-11T23:55:16Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Fri, Nov 11, 2005 at 11:58:39PM CET, I got a letter\nwhere Becky Bruce said that...\n> I grabbed 0.99.9g this morning, and tried to clone Paul Mackerras' \n> linux merge tree. The clone failed and reported errors in http-fetch \n> with a bunch of messages of the form:\n> \n> error: Couldn't create temporary file \n> .git/objects/04/48fa7de8a416a48cd1977f29858be54e67c078.temp for .git\n> /objects/04/48fa7de8a416a48cd1977f29858be54e67c078: Error 24: Too many \n> open files\n\n---\n\nThe current http-fetch is rather careless about fd leakage, causing\nproblems while fetching large repositories. This patch does not reserve\nexhaustiveness, but I covered everything I spotted. I also left some\nsafeguards in place in case I missed something, so that we get to know,\nsooner or later.\n\nReported by Becky Bruce .\n\nSigned-off-by: Petr Baudis \n---\n\n http-fetch.c | 16 ++++++++++++++--\n 1 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex 99921cc..e7655d1 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -413,6 +413,8 @@ static void start_request(struct transfe\n \trename(request->tmpfile, prevfile);\n \tunlink(request->tmpfile);\n \n+\tif (request->local != -1)\n+\t\terror(\"fd leakage in start: %d\", request->local);\n \trequest->local = open(request->tmpfile,\n \t\t\t O_WRONLY | O_CREAT | O_EXCL, 0666);\n \t/* This could have failed due to the \"lazy directory creation\";\n@@ -511,7 +513,7 @@ static void start_request(struct transfe\n \t/* Try to get the request started, abort the request on error */\n \tif (!start_active_slot(slot)) {\n \t\trequest->state = ABORTED;\n-\t\tclose(request->local);\n+\t\tclose(request->local); request->local = -1;\n \t\tfree(request->url);\n \t\treturn;\n \t}\n@@ -525,7 +527,7 @@ static void finish_request(struct transf\n \tstruct stat st;\n \n \tfchmod(request->local, 0444);\n-\tclose(request->local);\n+\tclose(request->local); request->local = -1;\n \n \tif (request->http_code == 416) {\n \t\tfprintf(stderr, \"Warning: requested range invalid; we may already have all the data.\\n\");\n@@ -557,6 +559,8 @@ static void release_request(struct trans\n {\n \tstruct transfer_request *entry = request_queue_head;\n \n+\tif (request->local != -1)\n+\t\terror(\"fd leakage in release: %d\", request->local);\n \tif (request == request_queue_head) {\n \t\trequest_queue_head = request->next;\n \t} else {\n@@ -613,6 +617,8 @@ static void process_curl_messages(void)\n \t\t\t\t\tif (request->repo->next != NULL) {\n \t\t\t\t\t\trequest->repo =\n \t\t\t\t\t\t\trequest->repo->next;\n+\t\t\t\t\t\tclose(request->local);\n+\t\t\t\t\t\t\trequest->local = -1;\n \t\t\t\t\t\tstart_request(request);\n \t\t\t\t\t}\n \t\t\t\t} else {\n@@ -743,6 +749,7 @@ static int fetch_index(struct alt_base *\n \t\t\t\t curl_errorstr);\n \t\t}\n \t} else {\n+\t\tfclose(indexfile);\n \t\treturn error(\"Unable to start request\");\n \t}\n \n@@ -1025,6 +1032,7 @@ static int fetch_pack(struct alt_base *r\n \t\t\t\t curl_errorstr);\n \t\t}\n \t} else {\n+\t\tfclose(packfile);\n \t\treturn error(\"Unable to start request\");\n \t}\n \n@@ -1087,6 +1095,7 @@ static int fetch_object(struct alt_base \n \t\t\tfetch_alternates(alt->base);\n \t\t\tif (request->repo->next != NULL) {\n \t\t\t\trequest->repo = request->repo->next;\n+\t\t\t\tclose(request->local); request->local = -1;\n \t\t\t\tstart_request(request);\n \t\t\t}\n \t\t} else {\n@@ -1095,6 +1104,9 @@ static int fetch_object(struct alt_base \n \t\t}\n #endif\n \t}\n+\tif (request->local != -1) {\n+\t\tclose(request->local); request->local = -1;\n+\t}\n \n \tif (request->state == ABORTED) {\n \t\trelease_request(request);\n\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nVI has two modes: the one in which it beeps and the one in which\nit doesn't.\n"},{"id":"11649","messageId":"7vk6feiflx.fsf@assigned-by-dhcp.cox.net","threadId":"2460","inReplyTo":"20051111235516.GY30496@pasky.or.cz","subject":"Re: [PATCH] Fix bunch of fd leaks in http-fetch","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-11-12T05:45:30Z","receivedAt":"2005-11-12T05:45:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis writes:\n\n> The current http-fetch is rather careless about fd leakage, causing\n> problems while fetching large repositories. This patch does not reserve\n> exhaustiveness, but I covered everything I spotted...\n\nThanks. While I am sure a quick fix is better for the end user\nthan not doing anything at all, I am a bit reluctant.\n\nIt strikes me somewhat odd that these close() are not tied to\nthe lifetime rule of the transfer_request structure. When the\nprogram falls back from an individual object to alternates, the\nsame request structure is reused, but in that case ->local stays\nthe same. Otherwise, the original request structure is released\nso I wonder if would make things cleaner to close ->local inside\nrequest_release()...\n\nNick?\n"},{"id":"11687","messageId":"20051112173828.GG4051@reactrix.com","threadId":"2460","inReplyTo":"7vk6feiflx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix bunch of fd leaks in http-fetch","fromName":"Nick Hengeveld","fromEmail":"nickh@reactrix.com","sentAt":"2005-11-12T17:38:28Z","receivedAt":"2005-11-12T17:38:28Z","isPatch":true,"sender":{"key":"nickh@reactrix.com","avatar":null},"body":"On Fri, Nov 11, 2005 at 09:45:30PM -0800, Junio C Hamano wrote:\n\n> It strikes me somewhat odd that these close() are not tied to\n> the lifetime rule of the transfer_request structure. When the\n> program falls back from an individual object to alternates, the\n> same request structure is reused, but in that case ->local stays\n> the same. Otherwise, the original request structure is released\n> so I wonder if would make things cleaner to close ->local inside\n> request_release()...\n\nThat is the intent of the fd close in finish_request() - but that isn't\ncalled if the server returns a 404 and there are no alternates left to\ntry.\n\nThe following patch should fix it.\n\n\n\n\nAdded a call to finish_request to clean up resources if the server\nreturned a 404 and there are no alternates left to try.\n\nSigned-off-by: Nick Hengeveld \n\n\n---\n\n http-fetch.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\napplies-to: 8bae950cd42c1d615fafdf63f4c96f6b665f1e0e\nfe26837d08627fbb2f5f57879ebb573474680c4a\ndiff --git a/http-fetch.c b/http-fetch.c\nindex cbb9690..78becce 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -632,6 +632,8 @@ static void process_curl_messages(void)\n \t\t\t\t\t\trequest->repo =\n \t\t\t\t\t\t\trequest->repo->next;\n \t\t\t\t\t\tstart_request(request);\n+\t\t\t\t\t} else {\n+\t\t\t\t\t\tfinish_request(request);\n \t\t\t\t\t}\n \t\t\t\t} else {\n \t\t\t\t\tfinish_request(request);\n---\n0.99.9.GIT\n"},{"id":"11692","messageId":"20051112195513.GF30496@pasky.or.cz","threadId":"2460","inReplyTo":"20051112173828.GG4051@reactrix.com","subject":"Re: [PATCH] Fix bunch of fd leaks in http-fetch","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2005-11-12T19:55:13Z","receivedAt":"2005-11-12T19:55:13Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Sat, Nov 12, 2005 at 06:38:28PM CET, I got a letter\nwhere Nick Hengeveld said that...\n> On Fri, Nov 11, 2005 at 09:45:30PM -0800, Junio C Hamano wrote:\n> \n> > It strikes me somewhat odd that these close() are not tied to\n> > the lifetime rule of the transfer_request structure. When the\n> > program falls back from an individual object to alternates, the\n> > same request structure is reused, but in that case ->local stays\n> > the same. Otherwise, the original request structure is released\n> > so I wonder if would make things cleaner to close ->local inside\n> > request_release()...\n> \n> That is the intent of the fd close in finish_request() - but that isn't\n> called if the server returns a 404 and there are no alternates left to\n> try.\n> \n> The following patch should fix it.\n\nWhat about the rest of the leaks?\n\nSpecifically, the one around release_request(), and the one caused by\nre-open()ing local in start_request() when re-calling it on existing\nrequest.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nVI has two modes: the one in which it beeps and the one in which\nit doesn't.\n"},{"id":"11799","messageId":"20051113033713.GC24546@reactrix.com","threadId":"2460","inReplyTo":"20051112195513.GF30496@pasky.or.cz","subject":"Re: [PATCH] Fix bunch of fd leaks in http-fetch","fromName":"Nick Hengeveld","fromEmail":"nickh@reactrix.com","sentAt":"2005-11-13T03:37:13Z","receivedAt":"2005-11-13T03:37:13Z","isPatch":true,"sender":{"key":"nickh@reactrix.com","avatar":null},"body":"On Sat, Nov 12, 2005 at 08:55:13PM +0100, Petr Baudis wrote:\n\n> What about the rest of the leaks?\n> \n> Specifically, the one around release_request(), and the one caused by\n> re-open()ing local in start_request() when re-calling it on existing\n> request.\n\nThere should be a close before calling start_request() with an\nalternate - start doesn't currently check for an existing fd before it\nopens a new one. There should also be closes for the indexfile and packfile\nfds.\n\nSo our patches combined should take care of those cases and report on\nanything that may have been missed, which doesn't seem like a bad thing.\nThe extra close in fetch_object shouldn't cause any problems because\nit only happens if there is still a valid fd in the request, which\nshould never happen...\n\nI'm pretty sure that fds are either already closed or were never opened\nprior to each call to release_request though. release is called in the\nfollowing circumstances:\n\n1) process_request_queue finds a WAITING request and the sha1 exists\n locally, no need to start() and the fd is never opened\n2) fetch_object finds the sha1 exists locally, which means the object\n was already in the repo (same as #1) or prefetch got it loose\n and called finish\n3) fetch_object finds an ABORTED request; requests abort when open fails\n or when the request didn't start (in which case the fd was closed\n after setting the request state)\n4) fetch_object finds a request that isn't WAITING/ACTIVE/ABORTED\n (ie. COMPLETE); finish is called when the request state is set\n to COMPLETE except in the aforementioned alternate restart case\n\n-- \nFor a successful technology, reality must take precedence over public\nrelations, for nature cannot be fooled.\n"}]}