[PATCH 0/5] Retry if fdopen() fails due to ENOMEM

[PATCH 0/5] Retry if fdopen() fails due to ENOMEM

[Michael Haggerty]

One likely reason for fdopen() to fail is the lack of memory for
allocating a FILE structure. When that happens, try freeing some
memory and calling fdopen() again in the hope that it will work the
second time.

This change was suggested by Jonathan Nieder [1]

In the first patch it is unsatisfying that try_to_free_routine() is
called with a magic number (1000) rather than sizeof(FILE). But the C
standard doesn't guarantee that FILE is a complete type, so I can't
think of a better approach. Suggestions, anybody?

Michael

[1] http://article.gmane.org/gmane.comp.version-control.git/260848

Michael Haggerty (5):
  xfdopen(): if first attempt fails, free memory and try again
  fdopen_lock_file(): use fdopen_with_retry()
  copy_to_log(): use fdopen_with_retry()
  update_info_file(): use fdopen_with_retry()
  buffer_fdinit(): use fdopen_with_retry()

 daemon.c              |  4 ++--
 git-compat-util.h     | 11 +++++++++++
 lockfile.c            |  2 +-
 server-info.c         |  2 +-
 vcs-svn/line_buffer.c |  2 +-
 wrapper.c             | 28 +++++++++++++++++++++++++---
 6 files changed, 41 insertions(+), 8 deletions(-)

-- 
2.1.4

[PATCH 1/5] xfdopen(): if first attempt fails, free memory and try again

[Michael Haggerty]

One likely reason for the failure of fdopen() is a lack of free
memory.

Also expose a new function, fdopen_with_retry(), which retries on
ENOMEM but doesn't die() on errors. In a moment this function will be
used elsewhere.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-compat-util.h | 11 +++++++++++
 wrapper.c         | 28 +++++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 3455c5e..a5652a7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -672,7 +672,18 @@ extern ssize_t xread(int fd, void *buf, size_t len);
 extern ssize_t xwrite(int fd, const void *buf, size_t len);
 extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
 extern int xdup(int fd);
+
+/*
+ * Like fdopen(), but if the first attempt fails with ENOMEM, try to
+ * free up some memory and try again.
+ */
+extern FILE *fdopen_with_retry(int fd, const char *mode);
+
+/*
+ * Like fdopen_with_retry(), but die on errors.
+ */
 extern FILE *xfdopen(int fd, const char *mode);
+
 extern int xmkstemp(char *template);
 extern int xmkstemp_mode(char *template, int mode);
 extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
diff --git a/wrapper.c b/wrapper.c
index d5a6cef..b60cc03 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -311,14 +311,36 @@ int xdup(int fd)
 	return ret;
 }
 
-FILE *xfdopen(int fd, const char *mode)
+FILE *fdopen_with_retry(int fd, const char *mode)
 {
 	FILE *stream = fdopen(fd, mode);
-	if (stream == NULL)
-		die_errno("Out of memory? fdopen failed");
+
+	if (!stream && errno == ENOMEM) {
+		/*
+		 * Try to free up some memory, then try again. We
+		 * would prefer to use sizeof(FILE) here, but that is
+		 * not guaranteed to be defined (e.g., FILE might be
+		 * an incomplete type).
+		 */
+		try_to_free_routine(1000);
+		stream = fdopen(fd, mode);
+	}
+
 	return stream;
 }
 
+FILE *xfdopen(int fd, const char *mode)
+{
+	FILE *stream = fdopen_with_retry(fd, mode);
+
+	if (stream)
+		return stream;
+	else if (errno == ENOMEM)
+		die_errno("Out of memory? fdopen failed");
+	else
+		die_errno("fdopen failed");
+}
+
 int xmkstemp(char *template)
 {
 	int fd;
-- 
2.1.4

[PATCH 2/5] fdopen_lock_file(): use fdopen_with_retry()

[Michael Haggerty]

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Jonathan's original suggestion was that this function should call
xfdopen(). But a couple of callers of fdopen_lock_file() try to
recover if it fails, so I decided to do it this way instead.

 lockfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lockfile.c b/lockfile.c
index 9889277..cad567b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -232,7 +232,7 @@ FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
 	if (lk->fp)
 		die("BUG: fdopen_lock_file() called twice for file '%s'", lk->filename.buf);
 
-	lk->fp = fdopen(lk->fd, mode);
+	lk->fp = fdopen_with_retry(lk->fd, mode);
 	return lk->fp;
 }
 
-- 
2.1.4

[PATCH 3/5] copy_to_log(): use fdopen_with_retry()

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 daemon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/daemon.c b/daemon.c
index c3edd96..1d76ccf 100644
--- a/daemon.c
+++ b/daemon.c
@@ -421,8 +421,8 @@ static void copy_to_log(int fd)
 	struct strbuf line = STRBUF_INIT;
 	FILE *fp;
 
-	fp = fdopen(fd, "r");
-	if (fp == NULL) {
+	fp = fdopen_with_retry(fd, "r");
+	if (!fp) {
 		logerror("fdopen of error channel failed");
 		close(fd);
 		return;
-- 
2.1.4

[PATCH 4/5] update_info_file(): use fdopen_with_retry()

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 server-info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server-info.c b/server-info.c
index 34b0253..8c6f662 100644
--- a/server-info.c
+++ b/server-info.c
@@ -20,7 +20,7 @@ static int update_info_file(char *path, int (*generate)(FILE *))
 	fd = git_mkstemp_mode(tmp, 0666);
 	if (fd < 0)
 		goto out;
-	fp = fdopen(fd, "w");
+	fp = fdopen_with_retry(fd, "w");
 	if (!fp)
 		goto out;
 	ret = generate(fp);
-- 
2.1.4

[PATCH 5/5] buffer_fdinit(): use fdopen_with_retry()

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 vcs-svn/line_buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 57cc1ce..10791cf 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -19,7 +19,7 @@ int buffer_init(struct line_buffer *buf, const char *filename)
 
 int buffer_fdinit(struct line_buffer *buf, int fd)
 {
-	buf->infile = fdopen(fd, "r");
+	buf->infile = fdopen_with_retry(fd, "r");
 	if (!buf->infile)
 		return -1;
 	return 0;
-- 
2.1.4

Re: [PATCH 1/5] xfdopen(): if first attempt fails, free memory and try again

[Stefan Beller]

On Thu, Mar 5, 2015 at 8:07 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> One likely reason for the failure of fdopen() is a lack of free
> memory.
>
> Also expose a new function, fdopen_with_retry(), which retries on
> ENOMEM but doesn't die() on errors. In a moment this function will be
> used elsewhere.
>
> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  git-compat-util.h | 11 +++++++++++
>  wrapper.c         | 28 +++++++++++++++++++++++++---
>  2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 3455c5e..a5652a7 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -672,7 +672,18 @@ extern ssize_t xread(int fd, void *buf, size_t len);
>  extern ssize_t xwrite(int fd, const void *buf, size_t len);
>  extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
>  extern int xdup(int fd);
> +
> +/*
> + * Like fdopen(), but if the first attempt fails with ENOMEM, try to
> + * free up some memory and try again.
> + */
> +extern FILE *fdopen_with_retry(int fd, const char *mode);
> +
> +/*
> + * Like fdopen_with_retry(), but die on errors.

"Rather like fdopen, but die on errors"?
When reading this before looking at the implementation below,
I thought this would behave like
FILE *xfdopen() {
       FILE *stream = fdopen(fd, mode);
       if (stream == NULL)
              die_errno("Out of memory? fdopen failed");
      return stream;
}
which has no retrying. I kind of read that comment that way.
Maybe
    This opens a FILE* handle, but tries
    to deal with some errors. If unsuccessful it dies,
    which guarantees to have a valid FILE* if returning.
?

I dunno.

> + */
>  extern FILE *xfdopen(int fd, const char *mode);
> +
>  extern int xmkstemp(char *template);
>  extern int xmkstemp_mode(char *template, int mode);
>  extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
> diff --git a/wrapper.c b/wrapper.c
> index d5a6cef..b60cc03 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -311,14 +311,36 @@ int xdup(int fd)
>         return ret;
>  }
>
> -FILE *xfdopen(int fd, const char *mode)
> +FILE *fdopen_with_retry(int fd, const char *mode)
>  {
>         FILE *stream = fdopen(fd, mode);
> -       if (stream == NULL)
> -               die_errno("Out of memory? fdopen failed");
> +
> +       if (!stream && errno == ENOMEM) {
> +               /*
> +                * Try to free up some memory, then try again. We
> +                * would prefer to use sizeof(FILE) here, but that is
> +                * not guaranteed to be defined (e.g., FILE might be
> +                * an incomplete type).
> +                */
> +               try_to_free_routine(1000);
> +               stream = fdopen(fd, mode);
> +       }
> +
>         return stream;
>  }
>
> +FILE *xfdopen(int fd, const char *mode)
> +{
> +       FILE *stream = fdopen_with_retry(fd, mode);
> +
> +       if (stream)
> +               return stream;
> +       else if (errno == ENOMEM)
> +               die_errno("Out of memory? fdopen failed");
> +       else
> +               die_errno("fdopen failed");

My gut feeling dislikes the distinction of different errors here just
to pass in a different string to die. The die_errno will already print
an appropriate message depending on the errno set. So I'd
rather have no distinction here.

If you want to have a different message for the case of ENOMEM,
I'd rather put it into fdopen_with_retry, which then uses a loop or
counted goto [ I've seen that in the refs code:
    if (nr_retries < MAX_RETRIES)
        goto retry_again;
]


> +}
> +
>  int xmkstemp(char *template)
>  {
>         int fd;
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] xfdopen(): if first attempt fails, free memory and try again

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> One likely reason for the failure of fdopen() is a lack of free
> memory.

Interesting.

Did you find this by code inspection?

Or did you actually hit this issue in real life, and applying this
patch helped?  The latter would indicate that this failure is rather
common with your workload, and that Git can continue working even
when the process is so memory starved to cause fdopen() to fail.

> Also expose a new function, fdopen_with_retry(), which retries on
> ENOMEM but doesn't die() on errors. In a moment this function will be
> used elsewhere.

Hmm, OK, I guess these three lines answers my question---asking that
question at this point in the series is moot ;-)

The code looks good.

> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  git-compat-util.h | 11 +++++++++++
>  wrapper.c         | 28 +++++++++++++++++++++++++---
>  2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 3455c5e..a5652a7 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -672,7 +672,18 @@ extern ssize_t xread(int fd, void *buf, size_t len);
>  extern ssize_t xwrite(int fd, const void *buf, size_t len);
>  extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
>  extern int xdup(int fd);
> +
> +/*
> + * Like fdopen(), but if the first attempt fails with ENOMEM, try to
> + * free up some memory and try again.
> + */
> +extern FILE *fdopen_with_retry(int fd, const char *mode);
> +
> +/*
> + * Like fdopen_with_retry(), but die on errors.
> + */
>  extern FILE *xfdopen(int fd, const char *mode);
> +
>  extern int xmkstemp(char *template);
>  extern int xmkstemp_mode(char *template, int mode);
>  extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
> diff --git a/wrapper.c b/wrapper.c
> index d5a6cef..b60cc03 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -311,14 +311,36 @@ int xdup(int fd)
>  	return ret;
>  }
>  
> -FILE *xfdopen(int fd, const char *mode)
> +FILE *fdopen_with_retry(int fd, const char *mode)
>  {
>  	FILE *stream = fdopen(fd, mode);
> -	if (stream == NULL)
> -		die_errno("Out of memory? fdopen failed");
> +
> +	if (!stream && errno == ENOMEM) {
> +		/*
> +		 * Try to free up some memory, then try again. We
> +		 * would prefer to use sizeof(FILE) here, but that is
> +		 * not guaranteed to be defined (e.g., FILE might be
> +		 * an incomplete type).
> +		 */
> +		try_to_free_routine(1000);
> +		stream = fdopen(fd, mode);
> +	}
> +
>  	return stream;
>  }
>  
> +FILE *xfdopen(int fd, const char *mode)
> +{
> +	FILE *stream = fdopen_with_retry(fd, mode);
> +
> +	if (stream)
> +		return stream;
> +	else if (errno == ENOMEM)
> +		die_errno("Out of memory? fdopen failed");
> +	else
> +		die_errno("fdopen failed");
> +}
> +
>  int xmkstemp(char *template)
>  {
>  	int fd;

Re: [PATCH 0/5] Retry if fdopen() fails due to ENOMEM

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> One likely reason for fdopen() to fail is the lack of memory for
> allocating a FILE structure. When that happens, try freeing some
> memory and calling fdopen() again in the hope that it will work the
> second time.

In codepaths where we are likely under memory pressure, the above
might help, but I have to wonder

    (1) if update-server-info and daemon fall into that category; and

    (2) if Git continues to work under such a memory pressure to
        cause even fdopen() to fail.

In other words, I do not see a reason not to do this change, but I
am not sure how much it would help us in practice.

We call fopen() from a lot more places than we call fdopen().  Do we
want to do the same, or is there a good reason why this does not
matter to callers of fopen(), and if so why doesn't the same reason
apply to callers of fdopen()?

> Michael Haggerty (5):
>   xfdopen(): if first attempt fails, free memory and try again
>   fdopen_lock_file(): use fdopen_with_retry()
>   copy_to_log(): use fdopen_with_retry()
>   update_info_file(): use fdopen_with_retry()
>   buffer_fdinit(): use fdopen_with_retry()
>
>  daemon.c              |  4 ++--
>  git-compat-util.h     | 11 +++++++++++
>  lockfile.c            |  2 +-
>  server-info.c         |  2 +-
>  vcs-svn/line_buffer.c |  2 +-
>  wrapper.c             | 28 +++++++++++++++++++++++++---
>  6 files changed, 41 insertions(+), 8 deletions(-)

Re: [PATCH 0/5] Retry if fdopen() fails due to ENOMEM

[Torsten Bögershausen]

On 03/05/2015 05:07 PM, Michael Haggerty wrote:
> One likely reason for fdopen() to fail is the lack of memory for
> allocating a FILE structure. When that happens, try freeing some
> memory and calling fdopen() again in the hope that it will work the
> second time.
>
> This change was suggested by Jonathan Nieder [1]
>
> In the first patch it is unsatisfying that try_to_free_routine() is
> called with a magic number (1000) rather than sizeof(FILE). But the C
> standard doesn't guarantee that FILE is a complete type, so I can't
> think of a better approach. Suggestions, anybody?
>
>
it's not the sizeof(FILE) which is critical, it is the size of the buffer
associated with a FILE

http://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdio.h.html

BUFSIZ may be  your friend, and if it is not defined, 4096 may be a 
useful default.

Re: [PATCH 0/5] Retry if fdopen() fails due to ENOMEM

[Michael Haggerty]

On 03/05/2015 08:19 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> One likely reason for fdopen() to fail is the lack of memory for
>> allocating a FILE structure. When that happens, try freeing some
>> memory and calling fdopen() again in the hope that it will work the
>> second time.
> 
> In codepaths where we are likely under memory pressure, the above
> might help, but I have to wonder
> 
>     (1) if update-server-info and daemon fall into that category; and
> 
>     (2) if Git continues to work under such a memory pressure to
>         cause even fdopen() to fail.
> 
> In other words, I do not see a reason not to do this change, but I
> am not sure how much it would help us in practice.
> 
> We call fopen() from a lot more places than we call fdopen().  Do we
> want to do the same, or is there a good reason why this does not
> matter to callers of fopen(), and if so why doesn't the same reason
> apply to callers of fdopen()?

To be honest, I was hoping that Jonathan would jump in and respond to
this thread, as he is the one who suggested this change.

I agree that it seems rather unlikely that a call to fdopen() is the
straw that breaks the camel's back. But I've never looked very closely
at Git's facility for freeing up memory when an allocation fails and
don't really have a mental model for how it is used.

If memory is allocated profligately under the expectation that it can be
freed if necessary (i.e., if calls to try_to_free_routine() are
relatively routine and tend to free a lot of memory), then it seems
important that as many memory allocation paths as possible call it and
retry when there is an error. This would be the garbage-collection
model, in which a failure to call the garbage collector at the right
time unnecessarily turns a routine event into a fatal error.

On the other hand, if calling try_to_free_routine() is an act of
desperation and typically results in little or no memory being freed,
then calling it in this code path is probably not very useful.

You also make a good point that if this rigmarole makes sense for
fdopen(), then it probably also makes sense for fopen().

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 0/5] Retry if fdopen() fails due to ENOMEM

[Michael Haggerty]

On 03/06/2015 06:08 AM, Torsten Bögershausen wrote:
> On 03/05/2015 05:07 PM, Michael Haggerty wrote:
>> One likely reason for fdopen() to fail is the lack of memory for
>> allocating a FILE structure. When that happens, try freeing some
>> memory and calling fdopen() again in the hope that it will work the
>> second time.
>>
>> This change was suggested by Jonathan Nieder [1]
>>
>> In the first patch it is unsatisfying that try_to_free_routine() is
>> called with a magic number (1000) rather than sizeof(FILE). But the C
>> standard doesn't guarantee that FILE is a complete type, so I can't
>> think of a better approach. Suggestions, anybody?
> 
> it's not the sizeof(FILE) which is critical, it is the size of the buffer
> associated with a FILE
> 
> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdio.h.html
> 
> BUFSIZ may be  your friend, and if it is not defined, 4096 may be a
> useful default.

Good point. If this patch series is not dropped as being useless, I will
make this change.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 0/5] Retry if fdopen() fails due to ENOMEM

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 03/06/2015 06:08 AM, Torsten Bögershausen wrote:
>> On 03/05/2015 05:07 PM, Michael Haggerty wrote:
>>> One likely reason for fdopen() to fail is the lack of memory for
>>> allocating a FILE structure. When that happens, try freeing some
>>> memory and calling fdopen() again in the hope that it will work the
>>> second time.
>>>
>>> This change was suggested by Jonathan Nieder [1]
>>>
>>> In the first patch it is unsatisfying that try_to_free_routine() is
>>> called with a magic number (1000) rather than sizeof(FILE). But the C
>>> standard doesn't guarantee that FILE is a complete type, so I can't
>>> think of a better approach. Suggestions, anybody?
>> 
>> it's not the sizeof(FILE) which is critical, it is the size of the buffer
>> associated with a FILE
>> 
>> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdio.h.html
>> 
>> BUFSIZ may be  your friend, and if it is not defined, 4096 may be a
>> useful default.
>
> Good point. If this patch series is not dropped as being useless, I will
> make this change.

OK, it has been a week since anybody mentioned this series.  What's
the verdict?  Taking what you said in $gmane/265228 into account, I
am taking the lack of reroll or follow-up as a sign of lost interest,
and if that is the case I'd drop the series before it hits 'next'.

Thanks.