git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Nick Hengeveld <nickh@reactrix.com>
To: Petr Baudis <pasky@suse.cz>
Cc: Junio C Hamano <junkio@cox.net>, git@vger.kernel.org
Subject: Re: [PATCH] Fix bunch of fd leaks in http-fetch
Date: Sat, 12 Nov 2005 19:37:13 -0800	[thread overview]
Message-ID: <20051113033713.GC24546@reactrix.com> (raw)
In-Reply-To: <20051112195513.GF30496@pasky.or.cz>

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.

      reply	other threads:[~2005-11-14 16:30 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-11-11 22:58 file descriptor leak? or expected behavior? Becky Bruce
2005-11-11 23:22 ` Becky Bruce
2005-11-11 23:55 ` [PATCH] Fix bunch of fd leaks in http-fetch Petr Baudis
2005-11-12  5:45   ` Junio C Hamano
2005-11-12 17:38     ` Nick Hengeveld
2005-11-12 19:55       ` Petr Baudis
2005-11-13  3:37         ` Nick Hengeveld [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20051113033713.GC24546@reactrix.com \
    --to=nickh@reactrix.com \
    --cc=git@vger.kernel.org \
    --cc=junkio@cox.net \
    --cc=pasky@suse.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).