file descriptor leak? or expected behavior?

file descriptor leak? or expected behavior?

[Becky Bruce]

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

Re: file descriptor leak? or expected behavior?

[Becky Bruce]

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

[PATCH] Fix bunch of fd leaks in http-fetch

[Petr Baudis]

Dear diary, on Fri, Nov 11, 2005 at 11:58:39PM CET, I got a letter
where Becky Bruce <becky.bruce@freescale.com> 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 <becky.bruce@freescale.com>.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 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.

Re: [PATCH] Fix bunch of fd leaks in http-fetch

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> 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...

Thanks.  While I am sure a quick fix is better for the end user
than not doing anything at all, I am a bit reluctant.

It strikes me somewhat odd that these close() are not tied to
the lifetime rule of the transfer_request structure.  When the
program falls back from an individual object to alternates, the
same request structure is reused, but in that case ->local stays
the same.  Otherwise, the original request structure is released
so I wonder if would make things cleaner to close ->local inside
request_release()...

Nick?

Re: [PATCH] Fix bunch of fd leaks in http-fetch

[Nick Hengeveld]

On Fri, Nov 11, 2005 at 09:45:30PM -0800, Junio C Hamano wrote:

> It strikes me somewhat odd that these close() are not tied to
> the lifetime rule of the transfer_request structure.  When the
> program falls back from an individual object to alternates, the
> same request structure is reused, but in that case ->local stays
> the same.  Otherwise, the original request structure is released
> so I wonder if would make things cleaner to close ->local inside
> request_release()...

That is the intent of the fd close in finish_request() - but that isn't
called if the server returns a 404 and there are no alternates left to
try.

The following patch should fix it.




Added a call to finish_request to clean up resources if the server
returned a 404 and there are no alternates left to try.

Signed-off-by: Nick Hengeveld <nickh@reactrix.com>


---

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

applies-to: 8bae950cd42c1d615fafdf63f4c96f6b665f1e0e
fe26837d08627fbb2f5f57879ebb573474680c4a
diff --git a/http-fetch.c b/http-fetch.c
index cbb9690..78becce 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -632,6 +632,8 @@ static void process_curl_messages(void)
 						request->repo =
 							request->repo->next;
 						start_request(request);
+					} else {
+						finish_request(request);
 					}
 				} else {
 					finish_request(request);
---
0.99.9.GIT

Re: [PATCH] Fix bunch of fd leaks in http-fetch

[Petr Baudis]

Dear diary, on Sat, Nov 12, 2005 at 06:38:28PM CET, I got a letter
where Nick Hengeveld <nickh@reactrix.com> said that...
> On Fri, Nov 11, 2005 at 09:45:30PM -0800, Junio C Hamano wrote:
> 
> > It strikes me somewhat odd that these close() are not tied to
> > the lifetime rule of the transfer_request structure.  When the
> > program falls back from an individual object to alternates, the
> > same request structure is reused, but in that case ->local stays
> > the same.  Otherwise, the original request structure is released
> > so I wonder if would make things cleaner to close ->local inside
> > request_release()...
> 
> That is the intent of the fd close in finish_request() - but that isn't
> called if the server returns a 404 and there are no alternates left to
> try.
> 
> The following patch should fix it.

What about the rest of the leaks?

Specifically, the one around release_request(), and the one caused by
re-open()ing local in start_request() when re-calling it on existing
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.

Re: [PATCH] Fix bunch of fd leaks in http-fetch

[Nick Hengeveld]

On Sat, Nov 12, 2005 at 08:55:13PM +0100, Petr Baudis wrote:

> What about the rest of the leaks?
> 
> Specifically, the one around release_request(), and the one caused by
> re-open()ing local in start_request() when re-calling it on existing
> request.

There should be a close before calling start_request() with an
alternate - start doesn't currently check for an existing fd before it
opens a new one.  There should also be closes for the indexfile and packfile
fds.

So our patches combined should take care of those cases and report on
anything that may have been missed, which doesn't seem like a bad thing.
The extra close in fetch_object shouldn't cause any problems because
it only happens if there is still a valid fd in the request, which
should never happen...

I'm pretty sure that fds are either already closed or were never opened
prior to each call to release_request though.  release is called in the
following circumstances:

1) process_request_queue finds a WAITING request and the sha1 exists
   locally, no need to start() and the fd is never opened
2) fetch_object finds the sha1 exists locally, which means the object
   was already in the repo (same as #1) or prefetch got it loose
   and called finish
3) fetch_object finds an ABORTED request; requests abort when open fails
   or when the request didn't start (in which case the fd was closed
   after setting the request state)
4) fetch_object finds a request that isn't WAITING/ACTIVE/ABORTED
   (ie. COMPLETE);  finish is called when the request state is set
   to COMPLETE except in the aforementioned alternate restart case

-- 
For a successful technology, reality must take precedence over public
relations, for nature cannot be fooled.