{"thread":{"id":"38710","subject":"[PATCH 2/5] fdopen_lock_file(): use fdopen_with_retry()","startedAt":"2015-03-05T16:07:44Z","lastAt":"2015-03-17T22:32:18Z","messageCount":13,"participants":["Michael Haggerty","Stefan Beller","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"257085","messageId":"1425571669-22800-1-git-send-email-mhagger@alum.mit.edu","threadId":"38710","inReplyTo":null,"subject":"[PATCH 0/5] Retry if fdopen() fails due to ENOMEM","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-05T16:07:44Z","receivedAt":"2015-03-05T16:07:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"One likely reason for fdopen() to fail is the lack of memory for\nallocating a FILE structure. When that happens, try freeing some\nmemory and calling fdopen() again in the hope that it will work the\nsecond time.\n\nThis change was suggested by Jonathan Nieder [1]\n\nIn the first patch it is unsatisfying that try_to_free_routine() is\ncalled with a magic number (1000) rather than sizeof(FILE). But the C\nstandard doesn't guarantee that FILE is a complete type, so I can't\nthink of a better approach. Suggestions, anybody?\n\nMichael\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/260848\n\nMichael Haggerty (5):\n  xfdopen(): if first attempt fails, free memory and try again\n  fdopen_lock_file(): use fdopen_with_retry()\n  copy_to_log(): use fdopen_with_retry()\n  update_info_file(): use fdopen_with_retry()\n  buffer_fdinit(): use fdopen_with_retry()\n\n daemon.c              |  4 ++--\n git-compat-util.h     | 11 +++++++++++\n lockfile.c            |  2 +-\n server-info.c         |  2 +-\n vcs-svn/line_buffer.c |  2 +-\n wrapper.c             | 28 +++++++++++++++++++++++++---\n 6 files changed, 41 insertions(+), 8 deletions(-)\n\n-- \n2.1.4\n"},{"id":"257087","messageId":"1425571669-22800-2-git-send-email-mhagger@alum.mit.edu","threadId":"38710","inReplyTo":"1425571669-22800-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/5] xfdopen(): if first attempt fails, free memory and try again","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-05T16:07:45Z","receivedAt":"2015-03-05T16:07:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"One likely reason for the failure of fdopen() is a lack of free\nmemory.\n\nAlso expose a new function, fdopen_with_retry(), which retries on\nENOMEM but doesn't die() on errors. In a moment this function will be\nused elsewhere.\n\nSuggested-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n git-compat-util.h | 11 +++++++++++\n wrapper.c         | 28 +++++++++++++++++++++++++---\n 2 files changed, 36 insertions(+), 3 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 3455c5e..a5652a7 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -672,7 +672,18 @@ extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n extern int xdup(int fd);\n+\n+/*\n+ * Like fdopen(), but if the first attempt fails with ENOMEM, try to\n+ * free up some memory and try again.\n+ */\n+extern FILE *fdopen_with_retry(int fd, const char *mode);\n+\n+/*\n+ * Like fdopen_with_retry(), but die on errors.\n+ */\n extern FILE *xfdopen(int fd, const char *mode);\n+\n extern int xmkstemp(char *template);\n extern int xmkstemp_mode(char *template, int mode);\n extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\ndiff --git a/wrapper.c b/wrapper.c\nindex d5a6cef..b60cc03 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -311,14 +311,36 @@ int xdup(int fd)\n \treturn ret;\n }\n \n-FILE *xfdopen(int fd, const char *mode)\n+FILE *fdopen_with_retry(int fd, const char *mode)\n {\n \tFILE *stream = fdopen(fd, mode);\n-\tif (stream == NULL)\n-\t\tdie_errno(\"Out of memory? fdopen failed\");\n+\n+\tif (!stream && errno == ENOMEM) {\n+\t\t/*\n+\t\t * Try to free up some memory, then try again. We\n+\t\t * would prefer to use sizeof(FILE) here, but that is\n+\t\t * not guaranteed to be defined (e.g., FILE might be\n+\t\t * an incomplete type).\n+\t\t */\n+\t\ttry_to_free_routine(1000);\n+\t\tstream = fdopen(fd, mode);\n+\t}\n+\n \treturn stream;\n }\n \n+FILE *xfdopen(int fd, const char *mode)\n+{\n+\tFILE *stream = fdopen_with_retry(fd, mode);\n+\n+\tif (stream)\n+\t\treturn stream;\n+\telse if (errno == ENOMEM)\n+\t\tdie_errno(\"Out of memory? fdopen failed\");\n+\telse\n+\t\tdie_errno(\"fdopen failed\");\n+}\n+\n int xmkstemp(char *template)\n {\n \tint fd;\n-- \n2.1.4\n"},{"id":"257083","messageId":"1425571669-22800-3-git-send-email-mhagger@alum.mit.edu","threadId":"38710","inReplyTo":"1425571669-22800-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/5] fdopen_lock_file(): use fdopen_with_retry()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-05T16:07:46Z","receivedAt":"2015-03-05T16:07:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Suggested-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nJonathan's original suggestion was that this function should call\nxfdopen(). But a couple of callers of fdopen_lock_file() try to\nrecover if it fails, so I decided to do it this way instead.\n\n lockfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 9889277..cad567b 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -232,7 +232,7 @@ FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)\n \tif (lk->fp)\n \t\tdie(\"BUG: fdopen_lock_file() called twice for file '%s'\", lk->filename.buf);\n \n-\tlk->fp = fdopen(lk->fd, mode);\n+\tlk->fp = fdopen_with_retry(lk->fd, mode);\n \treturn lk->fp;\n }\n \n-- \n2.1.4\n"},{"id":"257088","messageId":"1425571669-22800-4-git-send-email-mhagger@alum.mit.edu","threadId":"38710","inReplyTo":"1425571669-22800-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/5] copy_to_log(): use fdopen_with_retry()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-05T16:07:47Z","receivedAt":"2015-03-05T16:07:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n daemon.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex c3edd96..1d76ccf 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -421,8 +421,8 @@ static void copy_to_log(int fd)\n \tstruct strbuf line = STRBUF_INIT;\n \tFILE *fp;\n \n-\tfp = fdopen(fd, \"r\");\n-\tif (fp == NULL) {\n+\tfp = fdopen_with_retry(fd, \"r\");\n+\tif (!fp) {\n \t\tlogerror(\"fdopen of error channel failed\");\n \t\tclose(fd);\n \t\treturn;\n-- \n2.1.4\n"},{"id":"257084","messageId":"1425571669-22800-5-git-send-email-mhagger@alum.mit.edu","threadId":"38710","inReplyTo":"1425571669-22800-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/5] update_info_file(): use fdopen_with_retry()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-05T16:07:48Z","receivedAt":"2015-03-05T16:07:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n server-info.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex 34b0253..8c6f662 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -20,7 +20,7 @@ static int update_info_file(char *path, int (*generate)(FILE *))\n \tfd = git_mkstemp_mode(tmp, 0666);\n \tif (fd < 0)\n \t\tgoto out;\n-\tfp = fdopen(fd, \"w\");\n+\tfp = fdopen_with_retry(fd, \"w\");\n \tif (!fp)\n \t\tgoto out;\n \tret = generate(fp);\n-- \n2.1.4\n"},{"id":"257086","messageId":"1425571669-22800-6-git-send-email-mhagger@alum.mit.edu","threadId":"38710","inReplyTo":"1425571669-22800-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/5] buffer_fdinit(): use fdopen_with_retry()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-05T16:07:49Z","receivedAt":"2015-03-05T16:07:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n vcs-svn/line_buffer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c\nindex 57cc1ce..10791cf 100644\n--- a/vcs-svn/line_buffer.c\n+++ b/vcs-svn/line_buffer.c\n@@ -19,7 +19,7 @@ int buffer_init(struct line_buffer *buf, const char *filename)\n \n int buffer_fdinit(struct line_buffer *buf, int fd)\n {\n-\tbuf->infile = fdopen(fd, \"r\");\n+\tbuf->infile = fdopen_with_retry(fd, \"r\");\n \tif (!buf->infile)\n \t\treturn -1;\n \treturn 0;\n-- \n2.1.4\n"},{"id":"257090","messageId":"CAGZ79kZx5HgQyzkGQ6ggdVwGZKkkLNn5YLQHFbTR-i4QafyurA@mail.gmail.com","threadId":"38710","inReplyTo":"1425571669-22800-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/5] xfdopen(): if first attempt fails, free memory and try again","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-05T16:59:03Z","receivedAt":"2015-03-05T16:59:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 5, 2015 at 8:07 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> One likely reason for the failure of fdopen() is a lack of free\n> memory.\n>\n> Also expose a new function, fdopen_with_retry(), which retries on\n> ENOMEM but doesn't die() on errors. In a moment this function will be\n> used elsewhere.\n>\n> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  git-compat-util.h | 11 +++++++++++\n>  wrapper.c         | 28 +++++++++++++++++++++++++---\n>  2 files changed, 36 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 3455c5e..a5652a7 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -672,7 +672,18 @@ extern ssize_t xread(int fd, void *buf, size_t len);\n>  extern ssize_t xwrite(int fd, const void *buf, size_t len);\n>  extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n>  extern int xdup(int fd);\n> +\n> +/*\n> + * Like fdopen(), but if the first attempt fails with ENOMEM, try to\n> + * free up some memory and try again.\n> + */\n> +extern FILE *fdopen_with_retry(int fd, const char *mode);\n> +\n> +/*\n> + * Like fdopen_with_retry(), but die on errors.\n\n\"Rather like fdopen, but die on errors\"?\nWhen reading this before looking at the implementation below,\nI thought this would behave like\nFILE *xfdopen() {\n       FILE *stream = fdopen(fd, mode);\n       if (stream == NULL)\n              die_errno(\"Out of memory? fdopen failed\");\n      return stream;\n}\nwhich has no retrying. I kind of read that comment that way.\nMaybe\n    This opens a FILE* handle, but tries\n    to deal with some errors. If unsuccessful it dies,\n    which guarantees to have a valid FILE* if returning.\n?\n\nI dunno.\n\n> + */\n>  extern FILE *xfdopen(int fd, const char *mode);\n> +\n>  extern int xmkstemp(char *template);\n>  extern int xmkstemp_mode(char *template, int mode);\n>  extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n> diff --git a/wrapper.c b/wrapper.c\n> index d5a6cef..b60cc03 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -311,14 +311,36 @@ int xdup(int fd)\n>         return ret;\n>  }\n>\n> -FILE *xfdopen(int fd, const char *mode)\n> +FILE *fdopen_with_retry(int fd, const char *mode)\n>  {\n>         FILE *stream = fdopen(fd, mode);\n> -       if (stream == NULL)\n> -               die_errno(\"Out of memory? fdopen failed\");\n> +\n> +       if (!stream && errno == ENOMEM) {\n> +               /*\n> +                * Try to free up some memory, then try again. We\n> +                * would prefer to use sizeof(FILE) here, but that is\n> +                * not guaranteed to be defined (e.g., FILE might be\n> +                * an incomplete type).\n> +                */\n> +               try_to_free_routine(1000);\n> +               stream = fdopen(fd, mode);\n> +       }\n> +\n>         return stream;\n>  }\n>\n> +FILE *xfdopen(int fd, const char *mode)\n> +{\n> +       FILE *stream = fdopen_with_retry(fd, mode);\n> +\n> +       if (stream)\n> +               return stream;\n> +       else if (errno == ENOMEM)\n> +               die_errno(\"Out of memory? fdopen failed\");\n> +       else\n> +               die_errno(\"fdopen failed\");\n\nMy gut feeling dislikes the distinction of different errors here just\nto pass in a different string to die. The die_errno will already print\nan appropriate message depending on the errno set. So I'd\nrather have no distinction here.\n\nIf you want to have a different message for the case of ENOMEM,\nI'd rather put it into fdopen_with_retry, which then uses a loop or\ncounted goto [ I've seen that in the refs code:\n    if (nr_retries < MAX_RETRIES)\n        goto retry_again;\n]\n\n\n> +}\n> +\n>  int xmkstemp(char *template)\n>  {\n>         int fd;\n> --\n> 2.1.4\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"257097","messageId":"xmqq4mpz463f.fsf@gitster.dls.corp.google.com","threadId":"38710","inReplyTo":"1425571669-22800-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/5] xfdopen(): if first attempt fails, free memory and try again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-05T19:06:12Z","receivedAt":"2015-03-05T19:06:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> One likely reason for the failure of fdopen() is a lack of free\n> memory.\n\nInteresting.\n\nDid you find this by code inspection?\n\nOr did you actually hit this issue in real life, and applying this\npatch helped?  The latter would indicate that this failure is rather\ncommon with your workload, and that Git can continue working even\nwhen the process is so memory starved to cause fdopen() to fail.\n\n> Also expose a new function, fdopen_with_retry(), which retries on\n> ENOMEM but doesn't die() on errors. In a moment this function will be\n> used elsewhere.\n\nHmm, OK, I guess these three lines answers my question---asking that\nquestion at this point in the series is moot ;-)\n\nThe code looks good.\n\n> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  git-compat-util.h | 11 +++++++++++\n>  wrapper.c         | 28 +++++++++++++++++++++++++---\n>  2 files changed, 36 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 3455c5e..a5652a7 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -672,7 +672,18 @@ extern ssize_t xread(int fd, void *buf, size_t len);\n>  extern ssize_t xwrite(int fd, const void *buf, size_t len);\n>  extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n>  extern int xdup(int fd);\n> +\n> +/*\n> + * Like fdopen(), but if the first attempt fails with ENOMEM, try to\n> + * free up some memory and try again.\n> + */\n> +extern FILE *fdopen_with_retry(int fd, const char *mode);\n> +\n> +/*\n> + * Like fdopen_with_retry(), but die on errors.\n> + */\n>  extern FILE *xfdopen(int fd, const char *mode);\n> +\n>  extern int xmkstemp(char *template);\n>  extern int xmkstemp_mode(char *template, int mode);\n>  extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n> diff --git a/wrapper.c b/wrapper.c\n> index d5a6cef..b60cc03 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -311,14 +311,36 @@ int xdup(int fd)\n>  \treturn ret;\n>  }\n>  \n> -FILE *xfdopen(int fd, const char *mode)\n> +FILE *fdopen_with_retry(int fd, const char *mode)\n>  {\n>  \tFILE *stream = fdopen(fd, mode);\n> -\tif (stream == NULL)\n> -\t\tdie_errno(\"Out of memory? fdopen failed\");\n> +\n> +\tif (!stream && errno == ENOMEM) {\n> +\t\t/*\n> +\t\t * Try to free up some memory, then try again. We\n> +\t\t * would prefer to use sizeof(FILE) here, but that is\n> +\t\t * not guaranteed to be defined (e.g., FILE might be\n> +\t\t * an incomplete type).\n> +\t\t */\n> +\t\ttry_to_free_routine(1000);\n> +\t\tstream = fdopen(fd, mode);\n> +\t}\n> +\n>  \treturn stream;\n>  }\n>  \n> +FILE *xfdopen(int fd, const char *mode)\n> +{\n> +\tFILE *stream = fdopen_with_retry(fd, mode);\n> +\n> +\tif (stream)\n> +\t\treturn stream;\n> +\telse if (errno == ENOMEM)\n> +\t\tdie_errno(\"Out of memory? fdopen failed\");\n> +\telse\n> +\t\tdie_errno(\"fdopen failed\");\n> +}\n> +\n>  int xmkstemp(char *template)\n>  {\n>  \tint fd;\n"},{"id":"257099","messageId":"xmqqy4nb2qwn.fsf@gitster.dls.corp.google.com","threadId":"38710","inReplyTo":"1425571669-22800-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/5] Retry if fdopen() fails due to ENOMEM","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-05T19:19:36Z","receivedAt":"2015-03-05T19:19:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> One likely reason for fdopen() to fail is the lack of memory for\n> allocating a FILE structure. When that happens, try freeing some\n> memory and calling fdopen() again in the hope that it will work the\n> second time.\n\nIn codepaths where we are likely under memory pressure, the above\nmight help, but I have to wonder\n\n    (1) if update-server-info and daemon fall into that category; and\n\n    (2) if Git continues to work under such a memory pressure to\n        cause even fdopen() to fail.\n\nIn other words, I do not see a reason not to do this change, but I\nam not sure how much it would help us in practice.\n\nWe call fopen() from a lot more places than we call fdopen().  Do we\nwant to do the same, or is there a good reason why this does not\nmatter to callers of fopen(), and if so why doesn't the same reason\napply to callers of fdopen()?\n\n> Michael Haggerty (5):\n>   xfdopen(): if first attempt fails, free memory and try again\n>   fdopen_lock_file(): use fdopen_with_retry()\n>   copy_to_log(): use fdopen_with_retry()\n>   update_info_file(): use fdopen_with_retry()\n>   buffer_fdinit(): use fdopen_with_retry()\n>\n>  daemon.c              |  4 ++--\n>  git-compat-util.h     | 11 +++++++++++\n>  lockfile.c            |  2 +-\n>  server-info.c         |  2 +-\n>  vcs-svn/line_buffer.c |  2 +-\n>  wrapper.c             | 28 +++++++++++++++++++++++++---\n>  6 files changed, 41 insertions(+), 8 deletions(-)\n"},{"id":"257127","messageId":"54F9365E.2000705@web.de","threadId":"38710","inReplyTo":"1425571669-22800-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/5] Retry if fdopen() fails due to ENOMEM","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-03-06T05:08:46Z","receivedAt":"2015-03-06T05:08:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 03/05/2015 05:07 PM, Michael Haggerty wrote:\n> One likely reason for fdopen() to fail is the lack of memory for\n> allocating a FILE structure. When that happens, try freeing some\n> memory and calling fdopen() again in the hope that it will work the\n> second time.\n>\n> This change was suggested by Jonathan Nieder [1]\n>\n> In the first patch it is unsatisfying that try_to_free_routine() is\n> called with a magic number (1000) rather than sizeof(FILE). But the C\n> standard doesn't guarantee that FILE is a complete type, so I can't\n> think of a better approach. Suggestions, anybody?\n>\n>\nit's not the sizeof(FILE) which is critical, it is the size of the buffer\nassociated with a FILE\n\nhttp://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdio.h.html\n\nBUFSIZ may be  your friend, and if it is not defined, 4096 may be a \nuseful default.\n"},{"id":"257462","messageId":"54FED8C0.7060104@alum.mit.edu","threadId":"38710","inReplyTo":"xmqqy4nb2qwn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/5] Retry if fdopen() fails due to ENOMEM","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-10T11:42:56Z","receivedAt":"2015-03-10T11:42:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/05/2015 08:19 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> One likely reason for fdopen() to fail is the lack of memory for\n>> allocating a FILE structure. When that happens, try freeing some\n>> memory and calling fdopen() again in the hope that it will work the\n>> second time.\n> \n> In codepaths where we are likely under memory pressure, the above\n> might help, but I have to wonder\n> \n>     (1) if update-server-info and daemon fall into that category; and\n> \n>     (2) if Git continues to work under such a memory pressure to\n>         cause even fdopen() to fail.\n> \n> In other words, I do not see a reason not to do this change, but I\n> am not sure how much it would help us in practice.\n> \n> We call fopen() from a lot more places than we call fdopen().  Do we\n> want to do the same, or is there a good reason why this does not\n> matter to callers of fopen(), and if so why doesn't the same reason\n> apply to callers of fdopen()?\n\nTo be honest, I was hoping that Jonathan would jump in and respond to\nthis thread, as he is the one who suggested this change.\n\nI agree that it seems rather unlikely that a call to fdopen() is the\nstraw that breaks the camel's back. But I've never looked very closely\nat Git's facility for freeing up memory when an allocation fails and\ndon't really have a mental model for how it is used.\n\nIf memory is allocated profligately under the expectation that it can be\nfreed if necessary (i.e., if calls to try_to_free_routine() are\nrelatively routine and tend to free a lot of memory), then it seems\nimportant that as many memory allocation paths as possible call it and\nretry when there is an error. This would be the garbage-collection\nmodel, in which a failure to call the garbage collector at the right\ntime unnecessarily turns a routine event into a fatal error.\n\nOn the other hand, if calling try_to_free_routine() is an act of\ndesperation and typically results in little or no memory being freed,\nthen calling it in this code path is probably not very useful.\n\nYou also make a good point that if this rigmarole makes sense for\nfdopen(), then it probably also makes sense for fopen().\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"257463","messageId":"54FED916.6020607@alum.mit.edu","threadId":"38710","inReplyTo":"54F9365E.2000705@web.de","subject":"Re: [PATCH 0/5] Retry if fdopen() fails due to ENOMEM","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-10T11:44:22Z","receivedAt":"2015-03-10T11:44:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/06/2015 06:08 AM, Torsten Bögershausen wrote:\n> On 03/05/2015 05:07 PM, Michael Haggerty wrote:\n>> One likely reason for fdopen() to fail is the lack of memory for\n>> allocating a FILE structure. When that happens, try freeing some\n>> memory and calling fdopen() again in the hope that it will work the\n>> second time.\n>>\n>> This change was suggested by Jonathan Nieder [1]\n>>\n>> In the first patch it is unsatisfying that try_to_free_routine() is\n>> called with a magic number (1000) rather than sizeof(FILE). But the C\n>> standard doesn't guarantee that FILE is a complete type, so I can't\n>> think of a better approach. Suggestions, anybody?\n> \n> it's not the sizeof(FILE) which is critical, it is the size of the buffer\n> associated with a FILE\n> \n> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdio.h.html\n> \n> BUFSIZ may be  your friend, and if it is not defined, 4096 may be a\n> useful default.\n\nGood point. If this patch series is not dropped as being useless, I will\nmake this change.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"257897","messageId":"xmqqlhivmf0t.fsf@gitster.dls.corp.google.com","threadId":"38710","inReplyTo":"54FED916.6020607@alum.mit.edu","subject":"Re: [PATCH 0/5] Retry if fdopen() fails due to ENOMEM","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T22:32:18Z","receivedAt":"2015-03-17T22:32:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> On 03/06/2015 06:08 AM, Torsten Bögershausen wrote:\n>> On 03/05/2015 05:07 PM, Michael Haggerty wrote:\n>>> One likely reason for fdopen() to fail is the lack of memory for\n>>> allocating a FILE structure. When that happens, try freeing some\n>>> memory and calling fdopen() again in the hope that it will work the\n>>> second time.\n>>>\n>>> This change was suggested by Jonathan Nieder [1]\n>>>\n>>> In the first patch it is unsatisfying that try_to_free_routine() is\n>>> called with a magic number (1000) rather than sizeof(FILE). But the C\n>>> standard doesn't guarantee that FILE is a complete type, so I can't\n>>> think of a better approach. Suggestions, anybody?\n>> \n>> it's not the sizeof(FILE) which is critical, it is the size of the buffer\n>> associated with a FILE\n>> \n>> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdio.h.html\n>> \n>> BUFSIZ may be  your friend, and if it is not defined, 4096 may be a\n>> useful default.\n>\n> Good point. If this patch series is not dropped as being useless, I will\n> make this change.\n\nOK, it has been a week since anybody mentioned this series.  What's\nthe verdict?  Taking what you said in $gmane/265228 into account, I\nam taking the lack of reroll or follow-up as a sign of lost interest,\nand if that is the case I'd drop the series before it hits 'next'.\n\nThanks.\n"}]}