file descriptor leak? or expected behavior?

Folks,

My apologies if this is a known issue/question - I've searched the list 
and haven't found anything about this, but given the volume of traffic, 
it's easy to miss things.....

I grabbed 0.99.9g this morning, and tried to clone Paul Mackerras' 
linux merge tree. The clone failed and reported errors in http-fetch 
with a bunch of messages of the form:

error: Couldn't create temporary file 
.git/objects/04/48fa7de8a416a48cd1977f29858be54e67c078.temp for .git
/objects/04/48fa7de8a416a48cd1977f29858be54e67c078: Error 24: Too many 
open files

I did some experimenting, and it looks like this crops up somewhere 
between git versions  0.99.8f and 0.99.9a.  My question is, is git 
expected to try to open huge numbers of files, or is this a fd leak? I 
cranked up my ulimit, and am still unable to successfully clone this 
tree, although it fails differently (tail end of output....):

progress: 22 objects, 56146 bytes
Also look at 
http://www.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git/
Getting pack list
error: The requested URL returned error: 404
Getting pack list
Getting index for pack e0d76ffe354ef5665028a6cb4506ea902f72e1d0
Getting pack e0d76ffe354ef5665028a6cb4506ea902f72e1d0
which contains 5014bfa48ac169e0748e1e9651897788feb306dc
progress: 1322 objects, 5736795 bytes
cg-fetch: objects fetch failed
cg-clone: fetch failed


The command I ran, and the tree I tried to clone are:

 > cg-clone 
http://www.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc-merge.git 
linux-2.6.paul

Cheers,
-B

On Nov 11, 2005, at 4:58 PM, Becky Bruce wrote:

error: Couldn't create temporary file
.git/objects/04/48fa7de8a416a48cd1977f29858be54e67c078.temp for .git
/objects/04/48fa7de8a416a48cd1977f29858be54e67c078: Error 24: Too many
open files

By the way, in case this looks funny to anyone, this isn't the default 
message - I added the "Error 24" part because I'm used to looking at 
error numbers.

This is what the message looks like from an unmodified git:

error: Couldn't create temporary file 
.git/objects/a0/7e2c9307fa338896ecca300dc88033a8922885.temp for 
.git/objects/a0/7e2c9307fa338896ecca300dc88033a8922885: Too many open 
files


Cheers,
B

Dear diary, on Fri, Nov 11, 2005 at 11:58:39PM CET, I got a letter
where Becky Bruce [off-list ref] said that...

I grabbed 0.99.9g this morning, and tried to clone Paul Mackerras' 
linux merge tree. The clone failed and reported errors in http-fetch 
with a bunch of messages of the form:

error: Couldn't create temporary file 
.git/objects/04/48fa7de8a416a48cd1977f29858be54e67c078.temp for .git
/objects/04/48fa7de8a416a48cd1977f29858be54e67c078: Error 24: Too many 
open files

---

The current http-fetch is rather careless about fd leakage, causing
problems while fetching large repositories. This patch does not reserve
exhaustiveness, but I covered everything I spotted. I also left some
safeguards in place in case I missed something, so that we get to know,
sooner or later.

Reported by Becky Bruce [off-list ref].

Signed-off-by: Petr Baudis <redacted>
---

 http-fetch.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/http-fetch.c b/http-fetch.c
index 99921cc..e7655d1 100644
--- a/http-fetch.c
+++ b/http-fetch.c

@@ -413,6 +413,8 @@ static void start_request(struct transfe
 	rename(request->tmpfile, prevfile);
 	unlink(request->tmpfile);
 
+	if (request->local != -1)
+		error("fd leakage in start: %d", request->local);
 	request->local = open(request->tmpfile,
 			      O_WRONLY | O_CREAT | O_EXCL, 0666);
 	/* This could have failed due to the "lazy directory creation";

@@ -511,7 +513,7 @@ static void start_request(struct transfe
 	/* Try to get the request started, abort the request on error */
 	if (!start_active_slot(slot)) {
 		request->state = ABORTED;
-		close(request->local);
+		close(request->local); request->local = -1;
 		free(request->url);
 		return;
 	}

@@ -525,7 +527,7 @@ static void finish_request(struct transf
 	struct stat st;
 
 	fchmod(request->local, 0444);
-	close(request->local);
+	close(request->local); request->local = -1;
 
 	if (request->http_code == 416) {
 		fprintf(stderr, "Warning: requested range invalid; we may already have all the data.\n");

@@ -557,6 +559,8 @@ static void release_request(struct trans
 {
 	struct transfer_request *entry = request_queue_head;
 
+	if (request->local != -1)
+		error("fd leakage in release: %d", request->local);
 	if (request == request_queue_head) {
 		request_queue_head = request->next;
 	} else {

@@ -613,6 +617,8 @@ static void process_curl_messages(void)
 					if (request->repo->next != NULL) {
 						request->repo =
 							request->repo->next;
+						close(request->local);
+							request->local = -1;
 						start_request(request);
 					}
 				} else {

@@ -743,6 +749,7 @@ static int fetch_index(struct alt_base *
 				     curl_errorstr);
 		}
 	} else {
+		fclose(indexfile);
 		return error("Unable to start request");
 	}
 

@@ -1025,6 +1032,7 @@ static int fetch_pack(struct alt_base *r
 				     curl_errorstr);
 		}
 	} else {
+		fclose(packfile);
 		return error("Unable to start request");
 	}
 

@@ -1087,6 +1095,7 @@ static int fetch_object(struct alt_base 
 			fetch_alternates(alt->base);
 			if (request->repo->next != NULL) {
 				request->repo = request->repo->next;
+				close(request->local); request->local = -1;
 				start_request(request);
 			}
 		} else {

@@ -1095,6 +1104,9 @@ static int fetch_object(struct alt_base 
 		}
 #endif
 	}
+	if (request->local != -1) {
+		close(request->local); request->local = -1;
+	}
 
 	if (request->state == ABORTED) {
 		release_request(request);

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.