{"thread":{"id":"33519","subject":"[PATCH 0/3] avoid bogus \"recursion detected in die handler\" message","startedAt":"2013-04-15T23:06:51Z","lastAt":"2013-04-23T21:27:31Z","messageCount":23,"participants":["Jeff King","Eric Sunshine","Brandon Casey","Jonathan Nieder","Johannes Sixt","Junio C Hamano","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"214396","messageId":"20130415230651.GA16670@sigill.intra.peff.net","threadId":"33519","inReplyTo":null,"subject":"[PATCH 0/3] avoid bogus \"recursion detected in die handler\" message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-15T23:06:51Z","receivedAt":"2013-04-15T23:06:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If upload-pack fails while generating a pack, the receiving side of a\nfetch will print out a message like this:\n\n  fatal: git remote host hung up unexpectedly\n  fatal: recursion detected in die handler\n\nYou can see it by instrumenting upload-pack to fail like this:\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex bfa6279..721b416 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -777,6 +777,7 @@ static void upload_pack(void)\n \treceive_needs();\n \tif (want_obj.nr) {\n \t\tget_common_commits();\n+\t\texit(0);\n \t\tcreate_pack_file();\n \t}\n }\n\nand doing a clone or fetch. What's actually happening is that we have\ntwo threads running, each of which dies, fooling the recursion check.\nThis series attempts to fix it. The errors afterwards look like:\n\n  fatal: git remote host hung up unexpectedly\n  fatal: index-pack failed\n\nwhich is what was printed pre-cd163d4 (usage.c: detect recursion in die\nroutines and bail out immediately).\n\n-Peff\n"},{"id":"214397","messageId":"20130415230802.GA11267@sigill.intra.peff.net","threadId":"33519","inReplyTo":"20130415230651.GA16670@sigill.intra.peff.net","subject":"[PATCH 1/3] usage: refactor die-recursion checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-15T23:08:02Z","receivedAt":"2013-04-15T23:08:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When any git code calls die(), we chain to a custom\ndie_routine, which we expect to print a message and exit the\nprogram. To avoid infinite loops, we detect a recursive call\nto die() with a simple counter, and break out of the loop by\nprinting a message and exiting ourselves, without chaining\nto the die_routine.\n\nBut the user does not get to see the message that would have\nbeen fed to the die_routine, which makes debugging harder.\nThe user does not know if it was a true infinite loop, or\nsimply a single re-entrant call, since they cannot compare\nthe messages. Furthermore, if we are wrong about detecting\nthe recursion, we have blocked the user from seeing the\noriginal message, which is probably the more useful one.\n\nThis patch teaches die() to print the original die message\nto stderr before reporting the recursion. The custom\ndie_routine may or may not have put it the message to\nstderr, but this is the best we can do (it is what most\nhandlers will do anyway, and it is where our recursion error\nwill go).\n\nWhile we're at it, let's mark the \"recursion detected\"\nmessage as a \"BUG:\", since it should never happen in\npractice. And let's factor out the repeated code in die and\ndie_errno. This loses the information of which function was\ncalled to cause the recursion, but it's important; knowing\nthe actual message fed to the function (which we now do) is\nmuch more useful, as it can generally pin-point the actual\ncall-site that caused the recursion.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis helped me debug the current problem. And factoring it out helps\nwith patch 3. :)\n\n usage.c | 29 ++++++++++++++---------------\n 1 file changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/usage.c b/usage.c\nindex 40b3de5..c6b7ac5 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -6,8 +6,6 @@\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n \n-static int dying;\n-\n void vreportf(const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n@@ -80,17 +78,24 @@ void NORETURN die(const char *err, ...)\n \tusagef(\"%s\", err);\n }\n \n+static void check_die_recursion(const char *fmt, va_list ap)\n+{\n+\tstatic int dying;\n+\n+\tif (!dying++)\n+\t\treturn;\n+\n+\tvreportf(\"fatal: \", fmt, ap);\n+\tfputs(\"BUG: recursion detected in die handler\\n\", stderr);\n+\texit(128);\n+}\n+\n void NORETURN die(const char *err, ...)\n {\n \tva_list params;\n \n-\tif (dying) {\n-\t\tfputs(\"fatal: recursion detected in die handler\\n\", stderr);\n-\t\texit(128);\n-\t}\n-\tdying = 1;\n-\n \tva_start(params, err);\n+\tcheck_die_recursion(err, params);\n \tdie_routine(err, params);\n \tva_end(params);\n }\n@@ -102,13 +107,6 @@ void NORETURN die_errno(const char *fmt, ...)\n \tchar str_error[256], *err;\n \tint i, j;\n \n-\tif (dying) {\n-\t\tfputs(\"fatal: recursion detected in die_errno handler\\n\",\n-\t\t\tstderr);\n-\t\texit(128);\n-\t}\n-\tdying = 1;\n-\n \terr = strerror(errno);\n \tfor (i = j = 0; err[i] && j < sizeof(str_error) - 1; ) {\n \t\tif ((str_error[j++] = err[i++]) != '%')\n@@ -126,6 +124,7 @@ void NORETURN die_errno(const char *fmt, ...)\n \tsnprintf(fmt_with_err, sizeof(fmt_with_err), \"%s: %s\", fmt, str_error);\n \n \tva_start(params, fmt);\n+\tcheck_die_recursion(fmt_with_err, params);\n \tdie_routine(fmt_with_err, params);\n \tva_end(params);\n }\n-- \n1.8.2.8.g44e4c28\n"},{"id":"214398","messageId":"20130415230816.GB11267@sigill.intra.peff.net","threadId":"33519","inReplyTo":"20130415230651.GA16670@sigill.intra.peff.net","subject":"[PATCH 2/3] run-command: factor out running_main_thread function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-15T23:08:16Z","receivedAt":"2013-04-15T23:08:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the async subsystem is used, we may spawn off\nsub-threads (if the platform supports it), and consider the\noriginal thread to be the \"main\" thread of execution. We use\nthis information in a custom die_routine to decide whether\nto call pthread_exit or a regular full-process exit.\n\nLet's pull this decision out into its own function so that\nother parts of the system can use it to make decisions.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n run-command.c | 15 ++++++++++++++-\n run-command.h | 10 ++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 765c2ce..69ca052 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -603,7 +603,7 @@ static NORETURN void die_async(const char *err, va_list params)\n {\n \tvreportf(\"fatal: \", err, params);\n \n-\tif (!pthread_equal(main_thread, pthread_self())) {\n+\tif (!running_main_thread()) {\n \t\tstruct async *async = pthread_getspecific(async_key);\n \t\tif (async->proc_in >= 0)\n \t\t\tclose(async->proc_in);\n@@ -614,6 +614,19 @@ static NORETURN void die_async(const char *err, va_list params)\n \n \texit(128);\n }\n+\n+int running_main_thread(void)\n+{\n+\treturn pthread_equal(main_thread, pthread_self());\n+}\n+\n+#else /* NO_PTHREADS defined */\n+\n+int running_main_thread(void)\n+{\n+\treturn 1;\n+}\n+\n #endif\n \n int start_async(struct async *async)\ndiff --git a/run-command.h b/run-command.h\nindex 221ce33..981dd10 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -92,4 +92,14 @@ int finish_async(struct async *async);\n int start_async(struct async *async);\n int finish_async(struct async *async);\n \n+/*\n+ * If the platform supports threads, returns 1 if we are running the \"main\"\n+ * thread that spawned other async threads, and zero if we are executing one\n+ * of the async threads.\n+ *\n+ * If the platform does not support threads, returns 1 (we are always in the\n+ * main thread then).\n+ */\n+int running_main_thread(void);\n+\n #endif\n-- \n1.8.2.8.g44e4c28\n"},{"id":"214399","messageId":"20130415230954.GC11267@sigill.intra.peff.net","threadId":"33519","inReplyTo":"20130415230651.GA16670@sigill.intra.peff.net","subject":"[PATCH 3/3] usage: do not check die recursion outside main thread","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-15T23:09:54Z","receivedAt":"2013-04-15T23:09:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we have started threads with the async subsystem, both\nthe thread and the main program may call die(). The async\nsubsystem sets up a special die routine to call pthread_exit\nrather than regular exit, but the recursion check in die()\nhappens outside of this routine, and does not know that it's\nOK for two dies to run.\n\nAs a result, we may print \"recursion in die handler\" when an\nasync thread dies, even though there is no recursion. This\ncan be triggered, for example, by upload-pack errors; the\nreceiving side will die both in the async sideband\ndemultiplexer and in the main program.\n\nThis patch teaches the recursion check to use the same\nrunning_main_thread() check that the async code bases its\nexit decision on. That means we detect only recursion within\nthe main program itself, and assume that async handlers\ncorrectly die (which they do).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is the simplest solution. You could actually have a thread-local\ndying counter, which would detect die recursion within an async thread.\nBut I did not want to get into designing a lowest-common-denominator of\nthread-local storage that would work on all of our platforms.\n\n usage.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/usage.c b/usage.c\nindex c6b7ac5..33405c8 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -5,6 +5,7 @@\n  */\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n+#include \"run-command.h\"\n \n void vreportf(const char *prefix, const char *err, va_list params)\n {\n@@ -82,6 +83,9 @@ static void check_die_recursion(const char *fmt, va_list ap)\n {\n \tstatic int dying;\n \n+\tif (!running_main_thread())\n+\t\treturn;\n+\n \tif (!dying++)\n \t\treturn;\n \n-- \n1.8.2.8.g44e4c28\n"},{"id":"214403","messageId":"CAPig+cQtXO_jvymrT1Y69W+bhJ908o3HNABj1eVLN5zdJtpT+Q@mail.gmail.com","threadId":"33519","inReplyTo":"20130415230802.GA11267@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] usage: refactor die-recursion checks","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-04-15T23:45:03Z","receivedAt":"2013-04-15T23:45:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 15, 2013 at 7:08 PM, Jeff King <peff@peff.net> wrote:\n> This patch teaches die() to print the original die message\n> to stderr before reporting the recursion. The custom\n> die_routine may or may not have put it the message to\n\ns/put it the/emitted/ perhaps?\n\n> stderr, but this is the best we can do (it is what most\n> handlers will do anyway, and it is where our recursion error\n> will go).\n>\n> While we're at it, let's mark the \"recursion detected\"\n> message as a \"BUG:\", since it should never happen in\n> practice. And let's factor out the repeated code in die and\n> die_errno. This loses the information of which function was\n> called to cause the recursion, but it's important; knowing\n\nWas this supposed to be s/important/unimportant/?\n\n> the actual message fed to the function (which we now do) is\n> much more useful, as it can generally pin-point the actual\n> call-site that caused the recursion.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"214404","messageId":"20130415234725.GA8031@sigill.intra.peff.net","threadId":"33519","inReplyTo":"CAPig+cQtXO_jvymrT1Y69W+bhJ908o3HNABj1eVLN5zdJtpT+Q@mail.gmail.com","subject":"Re: [PATCH 1/3] usage: refactor die-recursion checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-15T23:47:25Z","receivedAt":"2013-04-15T23:47:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 15, 2013 at 07:45:03PM -0400, Eric Sunshine wrote:\n\n> On Mon, Apr 15, 2013 at 7:08 PM, Jeff King <peff@peff.net> wrote:\n> > This patch teaches die() to print the original die message\n> > to stderr before reporting the recursion. The custom\n> > die_routine may or may not have put it the message to\n> \n> s/put it the/emitted/ perhaps?\n\nI meant s/ it//, but I think \"sent the message to...\" is probably more\nclear.\n\n> > stderr, but this is the best we can do (it is what most\n> > handlers will do anyway, and it is where our recursion error\n> > will go).\n> >\n> > While we're at it, let's mark the \"recursion detected\"\n> > message as a \"BUG:\", since it should never happen in\n> > practice. And let's factor out the repeated code in die and\n> > die_errno. This loses the information of which function was\n> > called to cause the recursion, but it's important; knowing\n> \n> Was this supposed to be s/important/unimportant/?\n\nUrgh, yes, it was originally \"not important\" but I lost the \"not\" while\ntrying to clarify the wording.\n\nThanks.\n\n-Peff\n"},{"id":"214406","messageId":"CA+sFfMes99EepY4FCW32s1L3ywv_gyFb76=Y=35rvPbc2K1BWA@mail.gmail.com","threadId":"33519","inReplyTo":"20130415230802.GA11267@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] usage: refactor die-recursion checks","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-04-16T00:11:36Z","receivedAt":"2013-04-16T00:11:36Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Mon, Apr 15, 2013 at 4:08 PM, Jeff King <peff@peff.net> wrote:\n> When any git code calls die(), we chain to a custom\n> die_routine, which we expect to print a message and exit the\n> program. To avoid infinite loops, we detect a recursive call\n> to die() with a simple counter, and break out of the loop by\n> printing a message and exiting ourselves, without chaining\n> to the die_routine.\n>\n> But the user does not get to see the message that would have\n> been fed to the die_routine, which makes debugging harder.\n> The user does not know if it was a true infinite loop, or\n> simply a single re-entrant call, since they cannot compare\n> the messages. Furthermore, if we are wrong about detecting\n> the recursion, we have blocked the user from seeing the\n> original message, which is probably the more useful one.\n>\n> This patch teaches die() to print the original die message\n> to stderr before reporting the recursion. The custom\n> die_routine may or may not have put it the message to\n> stderr, but this is the best we can do (it is what most\n> handlers will do anyway, and it is where our recursion error\n> will go).\n>\n> While we're at it, let's mark the \"recursion detected\"\n> message as a \"BUG:\", since it should never happen in\n> practice. And let's factor out the repeated code in die and\n> die_errno. This loses the information of which function was\n> called to cause the recursion, but it's important; knowing\n> the actual message fed to the function (which we now do) is\n> much more useful, as it can generally pin-point the actual\n> call-site that caused the recursion.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This helped me debug the current problem. And factoring it out helps\n> with patch 3. :)\n>\n>  usage.c | 29 ++++++++++++++---------------\n>  1 file changed, 14 insertions(+), 15 deletions(-)\n>\n> diff --git a/usage.c b/usage.c\n> index 40b3de5..c6b7ac5 100644\n> --- a/usage.c\n> +++ b/usage.c\n\n> @@ -80,17 +78,24 @@ void NORETURN die(const char *err, ...)\n>         usagef(\"%s\", err);\n>  }\n>\n> +static void check_die_recursion(const char *fmt, va_list ap)\n> +{\n> +       static int dying;\n> +\n> +       if (!dying++)\n> +               return;\n> +\n> +       vreportf(\"fatal: \", fmt, ap);\n\nHow do you know it's safe to call vreportf() ?\n\nIf the bug is in the vreportf code path, we will recurse infinitely\n(at least until the stack is used up). An implementation of vsnprintf\nexists in compat/snprintf.c for example.\n\nIt's nice to print out the error message here, but I think doing so\ndefeats the purpose of this \"dying\" check.  Better to get the stack\ntrace from a core dump.\n\n> +       fputs(\"BUG: recursion detected in die handler\\n\", stderr);\n> +       exit(128);\n> +}\n> +\n\n-Brandon\n"},{"id":"214408","messageId":"20130416004228.GA14995@sigill.intra.peff.net","threadId":"33519","inReplyTo":"CA+sFfMes99EepY4FCW32s1L3ywv_gyFb76=Y=35rvPbc2K1BWA@mail.gmail.com","subject":"Re: [PATCH 1/3] usage: refactor die-recursion checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-16T00:42:28Z","receivedAt":"2013-04-16T00:42:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 15, 2013 at 05:11:36PM -0700, Brandon Casey wrote:\n\n> > +static void check_die_recursion(const char *fmt, va_list ap)\n> > +{\n> > +       static int dying;\n> > +\n> > +       if (!dying++)\n> > +               return;\n> > +\n> > +       vreportf(\"fatal: \", fmt, ap);\n> \n> How do you know it's safe to call vreportf() ?\n\nBecause I hand-audited it. But I think the more important question you\nare getting at is: how do I know that it will remain safe to call\nvreportf?\n\n> If the bug is in the vreportf code path, we will recurse infinitely\n> (at least until the stack is used up). An implementation of vsnprintf\n> exists in compat/snprintf.c for example.\n\nRight. My assumption was that we are primarily interested in protecting\nagainst the die_routine. Compat functions should never be calling die.\nOf course that is assuming nobody violates that rule, which is part of\nthe point of the check.\n\n> It's nice to print out the error message here, but I think doing so\n> defeats the purpose of this \"dying\" check.  Better to get the stack\n> trace from a core dump.\n\nEasier said than done, sometimes, if you are debugging somebody else's\nproblem from a dump of a terminal session. :)\n\nBut I can live with dropping this patch; my primary goal is the bug-fix\nin patch three.\n\nI was also tempted to suggest just dropping the recursion check\naltogether. While it is neat to detect such things, it's a \"should never\nhappen\" bug situation, and an infinite loop of printing out the same\nmessage is pretty easy to notice. Do you remember what spurred the\noriginal check? The message in cd163d4 doesn't say.\n\n-Peff\n"},{"id":"214413","messageId":"20130416014145.GC3262@elie.Belkin","threadId":"33519","inReplyTo":"20130416004228.GA14995@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] usage: refactor die-recursion checks","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-16T01:41:45Z","receivedAt":"2013-04-16T01:41:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I was also tempted to suggest just dropping the recursion check\n> altogether. While it is neat to detect such things, it's a \"should never\n> happen\" bug situation, and an infinite loop of printing out the same\n> message is pretty easy to notice.\n\nOn servers it might be useful to avoid accidentally filling up logs\nquickly.\n\nIIUC the context is in the following two threads:\n\n http://thread.gmane.org/gmane.comp.version-control.git/182982\n http://thread.gmane.org/gmane.comp.version-control.git/181421/focus=181443\n\nThanks,\nJonathan\n"},{"id":"214414","messageId":"20130416014504.GD3262@elie.Belkin","threadId":"33519","inReplyTo":"20130415230816.GB11267@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] run-command: factor out running_main_thread function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-16T01:45:04Z","receivedAt":"2013-04-16T01:45:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -603,7 +603,7 @@ static NORETURN void die_async(const char *err, va_list params)\n>  {\n>  \tvreportf(\"fatal: \", err, params);\n>  \n> -\tif (!pthread_equal(main_thread, pthread_self())) {\n> +\tif (!running_main_thread()) {\n>  \t\tstruct async *async = pthread_getspecific(async_key);\n>  \t\tif (async->proc_in >= 0)\n>  \t\t\tclose(async->proc_in);\n> @@ -614,6 +614,19 @@ static NORETURN void die_async(const char *err, va_list params)\n>  \n>  \texit(128);\n>  }\n> +\n> +int running_main_thread(void)\n> +{\n> +\treturn pthread_equal(main_thread, pthread_self());\n> +}\n\nWould it make sense to do something like\n\n\treturn !main_thread_set || pthread_equal(...);\n\nin case someone tries to call this before the first start_async call?\n"},{"id":"214417","messageId":"CA+sFfMdzTNjH10FKxhvJy+7hZg+0=1Wrqy9k8KaPoJg1DuDpmg@mail.gmail.com","threadId":"33519","inReplyTo":"20130416004228.GA14995@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] usage: refactor die-recursion checks","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-04-16T02:34:07Z","receivedAt":"2013-04-16T02:34:07Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Mon, Apr 15, 2013 at 5:42 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Apr 15, 2013 at 05:11:36PM -0700, Brandon Casey wrote:\n>\n>> > +static void check_die_recursion(const char *fmt, va_list ap)\n>> > +{\n>> > +       static int dying;\n>> > +\n>> > +       if (!dying++)\n>> > +               return;\n>> > +\n>> > +       vreportf(\"fatal: \", fmt, ap);\n>>\n>> How do you know it's safe to call vreportf() ?\n>\n> Because I hand-audited it.\n\n:)\n\n> But I think the more important question you\n> are getting at is: how do I know that it will remain safe to call\n> vreportf?\n\nRight.\n\n>> If the bug is in the vreportf code path, we will recurse infinitely\n>> (at least until the stack is used up). An implementation of vsnprintf\n>> exists in compat/snprintf.c for example.\n>\n> Right. My assumption was that we are primarily interested in protecting\n> against the die_routine. Compat functions should never be calling die.\n\nI think the rule we have been enforcing is less strict than that.  We\nhave only said that any compat function in a die handler path should\nnever call die.  But maybe that's what you meant.\n\n> Of course that is assuming nobody violates that rule, which is part of\n> the point of the check.\n>\n>> It's nice to print out the error message here, but I think doing so\n>> defeats the purpose of this \"dying\" check.  Better to get the stack\n>> trace from a core dump.\n>\n> Easier said than done, sometimes, if you are debugging somebody else's\n> problem from a dump of a terminal session. :)\n>\n> But I can live with dropping this patch; my primary goal is the bug-fix\n> in patch three.\n>\n> I was also tempted to suggest just dropping the recursion check\n> altogether. While it is neat to detect such things, it's a \"should never\n> happen\" bug situation, and an infinite loop of printing out the same\n> message is pretty easy to notice. Do you remember what spurred the\n> original check? The message in cd163d4 doesn't say.\n\nThat's a valid option.\n\nThe primary motivation was that Hannes Sixt had to step in and point\nout yet again that the high-level memory allocators should not be\ncalled in anything that could be in a die handler code path.  I was on\nthe thread, but I don't remember the topic (ah, Jonathan has stepped\nin with the answer).  I do remember that I was not the only one who\nhad forgotten about that rule though.\n\nWe didn't actually have someone report that they encountered infinite\nrecursion, but it seemed easy enough to add a check for it, so...\n\nUnfortunately, I didn't realize that the async functions installed\ntheir own die handler which may not exit the process, allowing die to\nbe called multiple times.\n\nTo implement this check correctly/completely (i.e. detect recursion in\nthe main thread as well as in any child threads), I think you really\ndo need to use thread-local storage as you mentioned in 3/3 which\ncould look something like:\n\n   static pthread_key_t dying;\n   static pthread_once_t dying_once = PTHREAD_ONCE_INIT;\n\n   void setup_die_counter(void)\n   {\n           pthread_key_create(&dying, NULL);\n   }\n\n   check_die_recursion(void)\n   {\n           pthread_once(&dying_once, setup_die_counter);\n           if (pthread(getspecific(dying)) {\n                   puts(\"BUG: recursion...\");\n                   exit(128);\n           }\n\n           pthread_setspecific(dying, &dying);\n   }\n\nor maybe the setup could be performed in set_die_routine(), but it\ndoes kinda seem like overkill for a \"nicety\" like this.  So maybe\nchecking for recursion in just the main thread as this series does is\nbetter than nothing.\n\n-Brandon\n"},{"id":"214419","messageId":"20130416025024.GA20932@sigill.intra.peff.net","threadId":"33519","inReplyTo":"CA+sFfMdzTNjH10FKxhvJy+7hZg+0=1Wrqy9k8KaPoJg1DuDpmg@mail.gmail.com","subject":"Re: [PATCH 1/3] usage: refactor die-recursion checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-16T02:50:24Z","receivedAt":"2013-04-16T02:50:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 15, 2013 at 07:34:07PM -0700, Brandon Casey wrote:\n\n> > Right. My assumption was that we are primarily interested in protecting\n> > against the die_routine. Compat functions should never be calling die.\n> \n> I think the rule we have been enforcing is less strict than that.  We\n> have only said that any compat function in a die handler path should\n> never call die.  But maybe that's what you meant.\n\nNo, I assumed we were following the stronger rule. If you are a compat\nfunction for a C library function, then you should never need to die.\nYou should be conforming to the existing interface, which will have some\nmechanism for passing back an error.\n\n> The primary motivation was that Hannes Sixt had to step in and point\n> out yet again that the high-level memory allocators should not be\n> called in anything that could be in a die handler code path.  I was on\n> the thread, but I don't remember the topic (ah, Jonathan has stepped\n> in with the answer).  I do remember that I was not the only one who\n> had forgotten about that rule though.\n\nYeah, it is subtle enough that it may be worth protecting against.\n\n> To implement this check correctly/completely (i.e. detect recursion in\n> the main thread as well as in any child threads), I think you really\n> do need to use thread-local storage as you mentioned in 3/3 which\n> could look something like:\n> \n>    static pthread_key_t dying;\n>    static pthread_once_t dying_once = PTHREAD_ONCE_INIT;\n> \n>    void setup_die_counter(void)\n>    {\n>            pthread_key_create(&dying, NULL);\n>    }\n> \n>    check_die_recursion(void)\n>    {\n>            pthread_once(&dying_once, setup_die_counter);\n>            if (pthread(getspecific(dying)) {\n>                    puts(\"BUG: recursion...\");\n>                    exit(128);\n>            }\n> \n>            pthread_setspecific(dying, &dying);\n>    }\n\nYeah, that seems sane; my biggest worry was that it would create\nheadaches for Windows folks, who would have to emulate pthread_key. But\nit seems like we already added support in 9ba604a.\n\nI'll try to re-work the series with thread-local storage, and I'll leave\noff the extra printing. This _should_ never happen, so if we are going\nto put in the check, it is probably better to be more thorough than to\nworry about what the error message looks like.\n\nThanks for looking it over.\n\n-Peff\n"},{"id":"214420","messageId":"20130416025340.GB20932@sigill.intra.peff.net","threadId":"33519","inReplyTo":"20130416014504.GD3262@elie.Belkin","subject":"Re: [PATCH 2/3] run-command: factor out running_main_thread function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-16T02:53:40Z","receivedAt":"2013-04-16T02:53:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 15, 2013 at 06:45:04PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -603,7 +603,7 @@ static NORETURN void die_async(const char *err, va_list params)\n> >  {\n> >  \tvreportf(\"fatal: \", err, params);\n> >  \n> > -\tif (!pthread_equal(main_thread, pthread_self())) {\n> > +\tif (!running_main_thread()) {\n> >  \t\tstruct async *async = pthread_getspecific(async_key);\n> >  \t\tif (async->proc_in >= 0)\n> >  \t\t\tclose(async->proc_in);\n> > @@ -614,6 +614,19 @@ static NORETURN void die_async(const char *err, va_list params)\n> >  \n> >  \texit(128);\n> >  }\n> > +\n> > +int running_main_thread(void)\n> > +{\n> > +\treturn pthread_equal(main_thread, pthread_self());\n> > +}\n> \n> Would it make sense to do something like\n> \n> \treturn !main_thread_set || pthread_equal(...);\n> \n> in case someone tries to call this before the first start_async call?\n\nI considered that, but assumed that pthread_equal would always return\nfalse against the zeroed out static main_thread. Mostly because we\nappeared not to be bothering with such a check already.  But actually,\nin the existing code, it is only checked from die_async, which is only\nput into place when we _do_ start a thread, so we would never hit this\ncode path without main_thread being set.\n\nSo yes, a check probably would make sense. However, I think I'm\nconvinced that we should drop this in favor of using thread-local\nstorage for the \"dying\" counter. So this patch can just go away.\n\nThanks for reviewing this version, though.\n\n-Peff\n"},{"id":"214442","messageId":"516CFB56.1090805@viscovery.net","threadId":"33519","inReplyTo":"20130416025024.GA20932@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] usage: refactor die-recursion checks","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-04-16T07:18:46Z","receivedAt":"2013-04-16T07:18:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/16/2013 4:50, schrieb Jeff King:\n> On Mon, Apr 15, 2013 at 07:34:07PM -0700, Brandon Casey wrote:\n> \n>>> Right. My assumption was that we are primarily interested in protecting\n>>> against the die_routine. Compat functions should never be calling die.\n>>\n>> I think the rule we have been enforcing is less strict than that.  We\n>> have only said that any compat function in a die handler path should\n>> never call die.  But maybe that's what you meant.\n> \n> No, I assumed we were following the stronger rule. If you are a compat\n> function for a C library function, then you should never need to die.\n> You should be conforming to the existing interface, which will have some\n> mechanism for passing back an error.\n\nThis rule has been violated LOOOONG ago, and not only in compat/mingw.c\n(see xmalloc in compat/qsort.c, for example).\n\n>> The primary motivation was that Hannes Sixt had to step in and point\n>> out yet again that the high-level memory allocators should not be\n>> called in anything that could be in a die handler code path.  I was on\n>> the thread, but I don't remember the topic (ah, Jonathan has stepped\n>> in with the answer).  I do remember that I was not the only one who\n>> had forgotten about that rule though.\n> \n> Yeah, it is subtle enough that it may be worth protecting against.\n\nToo late.\n\n>> To implement this check correctly/completely (i.e. detect recursion in\n>> the main thread as well as in any child threads), I think you really\n>> do need to use thread-local storage as you mentioned in 3/3 which\n>> could look something like:\n>>\n>>    static pthread_key_t dying;\n>>    static pthread_once_t dying_once = PTHREAD_ONCE_INIT;\n>>\n>>    void setup_die_counter(void)\n>>    {\n>>            pthread_key_create(&dying, NULL);\n>>    }\n>>\n>>    check_die_recursion(void)\n>>    {\n>>            pthread_once(&dying_once, setup_die_counter);\n>>            if (pthread(getspecific(dying)) {\n>>                    puts(\"BUG: recursion...\");\n>>                    exit(128);\n>>            }\n>>\n>>            pthread_setspecific(dying, &dying);\n>>    }\n> \n> Yeah, that seems sane; my biggest worry was that it would create\n> headaches for Windows folks, who would have to emulate pthread_key. But\n> it seems like we already added support in 9ba604a.\n\npthread_key is not a problem, but pthread_once is. It's certainly\nsolvable, but do we really have to?\n\n-- Hannes\n"},{"id":"214482","messageId":"20130416130154.GA17976@sigill.intra.peff.net","threadId":"33519","inReplyTo":"516CFB56.1090805@viscovery.net","subject":"Re: [PATCH 1/3] usage: refactor die-recursion checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-16T13:01:54Z","receivedAt":"2013-04-16T13:01:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 16, 2013 at 09:18:46AM +0200, Johannes Sixt wrote:\n\n> > Yeah, that seems sane; my biggest worry was that it would create\n> > headaches for Windows folks, who would have to emulate pthread_key. But\n> > it seems like we already added support in 9ba604a.\n> \n> pthread_key is not a problem, but pthread_once is. It's certainly\n> solvable, but do we really have to?\n\nI'm not clear on what you are suggesting. That we protect only the main\nthread from recursion, or that we drop the check entirely? Or that we\nimplement thread-local storage for this case without using pthread_once?\n\n-Peff\n"},{"id":"214486","messageId":"516D5CA4.7000500@viscovery.net","threadId":"33519","inReplyTo":"20130416130154.GA17976@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] usage: refactor die-recursion checks","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-04-16T14:13:56Z","receivedAt":"2013-04-16T14:13:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/16/2013 15:01, schrieb Jeff King:\n> On Tue, Apr 16, 2013 at 09:18:46AM +0200, Johannes Sixt wrote:\n> \n>>> Yeah, that seems sane; my biggest worry was that it would create\n>>> headaches for Windows folks, who would have to emulate pthread_key. But\n>>> it seems like we already added support in 9ba604a.\n>>\n>> pthread_key is not a problem, but pthread_once is. It's certainly\n>> solvable, but do we really have to?\n> \n> I'm not clear on what you are suggesting. That we protect only the main\n> thread from recursion, or that we drop the check entirely? Or that we\n> implement thread-local storage for this case without using pthread_once?\n\nAnything(*) that does not require pthread_once. A pthread_once\nimplementation on Windows would be tricky and voluminous and and on top of\nit very likely to be done differently for gcc and MSVC. I don't like to go\nthere if we can avoid it.\n\n(*) That includes doing nothing, but does not include ripping out the\nrecursion check, as it protects us from crashes.\n\n-- Hannes\n"},{"id":"214524","messageId":"20130416194418.GA7187@sigill.intra.peff.net","threadId":"33519","inReplyTo":"516D5CA4.7000500@viscovery.net","subject":"[PATCH v2 0/2] avoid bogus \"recursion detected in die handler\" message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-16T19:44:18Z","receivedAt":"2013-04-16T19:44:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 16, 2013 at 04:13:56PM +0200, Johannes Sixt wrote:\n\n> > I'm not clear on what you are suggesting. That we protect only the main\n> > thread from recursion, or that we drop the check entirely? Or that we\n> > implement thread-local storage for this case without using pthread_once?\n> \n> Anything(*) that does not require pthread_once. A pthread_once\n> implementation on Windows would be tricky and voluminous and and on top of\n> it very likely to be done differently for gcc and MSVC. I don't like to go\n> there if we can avoid it.\n\nWe don't need to use pthread_once, as we can just do the initialization\nof the thread-local storage along with starting the first thread (where\nwe already do similar initialization).  Patch series to follow:\n\n  [1/2]: usage: allow pluggable die-recursion checks\n  [2/2]: run-command: use thread-aware die_is_recursing routine\n\n> (*) That includes doing nothing, but does not include ripping out the\n> recursion check, as it protects us from crashes.\n\nI don't think doing nothing is a good idea. The recursion-detection is\ntriggering erroneously, blocking real error messages and replacing them\nwith the scary red-herring \"recursion detected in die handler\".\n\nThe absolute simplest thing I think we could do is basically:\n\ndiff --git a/run-command.c b/run-command.c\nindex 765c2ce..3b0ad44 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -599,11 +599,14 @@ static NORETURN void die_async(const char *err, va_list params)\n \treturn (void *)ret;\n }\n \n+extern int dying;\n+\n static NORETURN void die_async(const char *err, va_list params)\n {\n \tvreportf(\"fatal: \", err, params);\n \n \tif (!pthread_equal(main_thread, pthread_self())) {\n+\t\tdying = 0; /* undo counter */\n \t\tstruct async *async = pthread_getspecific(async_key);\n \t\tif (async->proc_in >= 0)\n \t\t\tclose(async->proc_in);\ndiff --git a/usage.c b/usage.c\nindex 40b3de5..cf8a968 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -6,7 +6,7 @@\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n \n-static int dying;\n+int dying;\n \n void vreportf(const char *prefix, const char *err, va_list params)\n {\n\nObviously it would help to wrap it in a \"clear_die_counter()\" function,\nbut it would still suffer from the problem that there is no\nsynchronization. In the moment between incrementing and resetting the\ndying counter, another thread (including the main program) could check\nit. In practice, this does not happen in the current code, because we\ndo not start many async threads (and we only die in the main thread once\nthe async thread dies). But it seems unnecessarily flaky and prone to\nfuture problems.\n\nIt would also be possible to use mutexes to make it work reliably, but\nI'd be very concerned about increasing the complexity of the die code\npath.  We would never want a hung thread to prevent the main program\nfrom successfully exiting, for example.\n\nSo I think the right solution is just a per-thread counter.\n\n-Peff\n"},{"id":"214526","messageId":"20130416194621.GA11185@sigill.intra.peff.net","threadId":"33519","inReplyTo":"20130416194418.GA7187@sigill.intra.peff.net","subject":"[PATCH v2 1/2] usage: allow pluggable die-recursion checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-16T19:46:22Z","receivedAt":"2013-04-16T19:46:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When any git code calls die or die_errno, we use a counter\nto detect recursion into the die functions from any of the\nhelper functions. However, such a simple counter is not good\nenough for threaded programs, which may call die from a\nsub-thread, killing only the sub-thread (but incrementing\nthe counter for everyone).\n\nRather than try to deal with threads ourselves here, let's\njust allow callers to plug in their own recursion-detection\nfunction. This is similar to how we handle the die routine\n(the caller plugs in a die routine which may kill only the\nsub-thread).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git-compat-util.h |  1 +\n usage.c           | 20 ++++++++++++++------\n 2 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cde442f..e955bb5 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -331,6 +331,7 @@ extern void set_error_routine(void (*routine)(const char *err, va_list params));\n \n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n extern void set_error_routine(void (*routine)(const char *err, va_list params));\n+extern void set_die_is_recursing_routine(int (*routine)(void));\n \n extern int prefixcmp(const char *str, const char *prefix);\n extern int suffixcmp(const char *str, const char *suffix);\ndiff --git a/usage.c b/usage.c\nindex 40b3de5..ed14645 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -6,8 +6,6 @@\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n \n-static int dying;\n-\n void vreportf(const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n@@ -49,12 +47,19 @@ static void (*warn_routine)(const char *err, va_list params) = warn_builtin;\n \tvreportf(\"warning: \", warn, params);\n }\n \n+static int die_is_recursing_builtin(void)\n+{\n+\tstatic int dying;\n+\treturn dying++;\n+}\n+\n /* If we are in a dlopen()ed .so write to a global variable would segfault\n  * (ugh), so keep things static. */\n static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;\n static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;\n static void (*error_routine)(const char *err, va_list params) = error_builtin;\n static void (*warn_routine)(const char *err, va_list params) = warn_builtin;\n+static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n \n void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))\n {\n@@ -66,6 +71,11 @@ void set_error_routine(void (*routine)(const char *err, va_list params))\n \terror_routine = routine;\n }\n \n+void set_die_is_recursing_routine(int (*routine)(void))\n+{\n+\tdie_is_recursing = routine;\n+}\n+\n void NORETURN usagef(const char *err, ...)\n {\n \tva_list params;\n@@ -84,11 +94,10 @@ void NORETURN die(const char *err, ...)\n {\n \tva_list params;\n \n-\tif (dying) {\n+\tif (die_is_recursing()) {\n \t\tfputs(\"fatal: recursion detected in die handler\\n\", stderr);\n \t\texit(128);\n \t}\n-\tdying = 1;\n \n \tva_start(params, err);\n \tdie_routine(err, params);\n@@ -102,12 +111,11 @@ void NORETURN die_errno(const char *fmt, ...)\n \tchar str_error[256], *err;\n \tint i, j;\n \n-\tif (dying) {\n+\tif (die_is_recursing()) {\n \t\tfputs(\"fatal: recursion detected in die_errno handler\\n\",\n \t\t\tstderr);\n \t\texit(128);\n \t}\n-\tdying = 1;\n \n \terr = strerror(errno);\n \tfor (i = j = 0; err[i] && j < sizeof(str_error) - 1; ) {\n-- \n1.8.2.8.g44e4c28\n"},{"id":"214528","messageId":"20130416195006.GB11185@sigill.intra.peff.net","threadId":"33519","inReplyTo":"20130416194418.GA7187@sigill.intra.peff.net","subject":"[PATCH v2 2/2] run-command: use thread-aware die_is_recursing routine","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-16T19:50:07Z","receivedAt":"2013-04-16T19:50:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we die from an async thread, we do not actually exit the\nprogram, but just kill the thread. This confuses the static\ncounter in usage.c's default die_is_recursing function; it\nupdates the counter once for the thread death, and then when\nthe main program calls die() itself, it erroneously thinks\nwe are recursing. The end result is that we print \"recursion\ndetected in die handler\" instead of the real error in such a\ncase (the easiest way to trigger this is having a remote\nconnection hang up while running a sideband demultiplexer).\n\nThis patch solves it by using a per-thread counter when the\nasync_die function is installed; we detect recursion in each\nthread (including the main one), but they do not step on\neach other's toes.\n\nOther threaded code does not need to worry about this, as\nthey do not install specialized die handlers; they just let\na die() from a sub-thread take down the whole program.\n\nSince we are overriding the default recursion-check\nfunction, there is an interesting corner case that is not a\nproblem, but bears some explanation. Imagine the main thread\ncalls die(), and then in the die_routine starts an async\ncall. We will switch to using thread-local storage, which\nstarts at 0, for the main thread's counter, even though\nthe original counter was actually at 1. That's OK, though,\nfor two reasons:\n\n  1. It would miss only the first level of recursion, and\n     would still find recursive failures inside the async\n     helper.\n\n  2. We do not currently and are not likely to start doing\n     anything as heavyweight as starting an async routine\n     from within a die routine or helper function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n run-command.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 765c2ce..1b32a12 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -588,6 +588,7 @@ static pthread_key_t async_key;\n static pthread_t main_thread;\n static int main_thread_set;\n static pthread_key_t async_key;\n+static pthread_key_t async_die_counter;\n \n static void *run_thread(void *data)\n {\n@@ -614,6 +615,14 @@ static NORETURN void die_async(const char *err, va_list params)\n \n \texit(128);\n }\n+\n+static int async_die_is_recursing(void)\n+{\n+\tvoid *ret = pthread_getspecific(async_die_counter);\n+\tpthread_setspecific(async_die_counter, (void *)1);\n+\treturn ret != NULL;\n+}\n+\n #endif\n \n int start_async(struct async *async)\n@@ -695,7 +704,9 @@ int start_async(struct async *async)\n \t\tmain_thread_set = 1;\n \t\tmain_thread = pthread_self();\n \t\tpthread_key_create(&async_key, NULL);\n+\t\tpthread_key_create(&async_die_counter, NULL);\n \t\tset_die_routine(die_async);\n+\t\tset_die_is_recursing_routine(async_die_is_recursing);\n \t}\n \n \tif (proc_in >= 0)\n-- \n1.8.2.8.g44e4c28\n"},{"id":"214550","messageId":"7v4nf6jfg7.fsf@alter.siamese.dyndns.org","threadId":"33519","inReplyTo":"20130416195006.GB11185@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] run-command: use thread-aware die_is_recursing routine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-16T22:09:12Z","receivedAt":"2013-04-16T22:09:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/run-command.c b/run-command.c\n> index 765c2ce..1b32a12 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -588,6 +588,7 @@ static pthread_key_t async_key;\n>  static pthread_t main_thread;\n>  static int main_thread_set;\n>  static pthread_key_t async_key;\n> +static pthread_key_t async_die_counter;\n>  \n>  static void *run_thread(void *data)\n>  {\n> @@ -614,6 +615,14 @@ static NORETURN void die_async(const char *err, va_list params)\n>  \n>  \texit(128);\n>  }\n> +\n> +static int async_die_is_recursing(void)\n> +{\n> +\tvoid *ret = pthread_getspecific(async_die_counter);\n> +\tpthread_setspecific(async_die_counter, (void *)1);\n> +\treturn ret != NULL;\n> +}\n> +\n>  #endif\n>  \n>  int start_async(struct async *async)\n> @@ -695,7 +704,9 @@ int start_async(struct async *async)\n>  \t\tmain_thread_set = 1;\n>  \t\tmain_thread = pthread_self();\n>  \t\tpthread_key_create(&async_key, NULL);\n> +\t\tpthread_key_create(&async_die_counter, NULL);\n>  \t\tset_die_routine(die_async);\n> +\t\tset_die_is_recursing_routine(async_die_is_recursing);\n>  \t}\n>  \n>  \tif (proc_in >= 0)\n\nWill queue; thanks.\n"},{"id":"214562","messageId":"20130417004941.GJ29773@google.com","threadId":"33519","inReplyTo":"20130416194418.GA7187@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/2] avoid bogus \"recursion detected in die handler\" message","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-17T00:49:41Z","receivedAt":"2013-04-17T00:49:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n>   [1/2]: usage: allow pluggable die-recursion checks\n>   [2/2]: run-command: use thread-aware die_is_recursing routine\n\nLovely.  This doesn't solve the analagous problem for grep,\nindex-pack, pack-objects, preload-index, or bidirectional_transfer_loop,\nbut it doesn't make them worse and even should make them easier to fix\nlater, so\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"214563","messageId":"20130417013708.GA27786@sigill.intra.peff.net","threadId":"33519","inReplyTo":"20130417004941.GJ29773@google.com","subject":"Re: [PATCH v2 0/2] avoid bogus \"recursion detected in die handler\" message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-17T01:37:08Z","receivedAt":"2013-04-17T01:37:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 16, 2013 at 05:49:41PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> >   [1/2]: usage: allow pluggable die-recursion checks\n> >   [2/2]: run-command: use thread-aware die_is_recursing routine\n> \n> Lovely.  This doesn't solve the analagous problem for grep,\n> index-pack, pack-objects, preload-index, or bidirectional_transfer_loop,\n> but it doesn't make them worse and even should make them easier to fix\n> later, so\n> \n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThe problem does exist in those other programs, but is much less\nlikely to be triggered. There it would be a race between two threads\ncalling die() at the same time, because as soon as one finishes, it\ntakes down the whole program. What made this one so easy to trigger was\nthe installation of die_async.\n\nSo it's probably not that big a deal to leave the race in those other\nplaces for the time being. Any refactoring of the \"dying\" flag should\naccompany an overall strategy for managing the clean death of\nsub-threads (which would entail a custom die routine, at which point the\nrecursion-check should just fall out naturally from whatever approach\nthey choose).\n\nThanks for reviewing.\n\n-Peff\n"},{"id":"215274","messageId":"CABPQNSbdYSbb5KrMDp8gKPTJcbjq2j91G3kJwrTMz=rJUydGuA@mail.gmail.com","threadId":"33519","inReplyTo":"20130416194418.GA7187@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/2] avoid bogus \"recursion detected in die handler\" message","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2013-04-23T21:27:31Z","receivedAt":"2013-04-23T21:27:31Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Apr 16, 2013 at 9:44 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Apr 16, 2013 at 04:13:56PM +0200, Johannes Sixt wrote:\n>\n>> > I'm not clear on what you are suggesting. That we protect only the main\n>> > thread from recursion, or that we drop the check entirely? Or that we\n>> > implement thread-local storage for this case without using pthread_once?\n>>\n>> Anything(*) that does not require pthread_once. A pthread_once\n>> implementation on Windows would be tricky and voluminous and and on top of\n>> it very likely to be done differently for gcc and MSVC. I don't like to go\n>> there if we can avoid it.\n\nThis seems to have been settled as a \"we don't need to go there\"\nalready, but just in case; here's an implementation of\nPTHREAD_MUTEX_INITIALIZER using double-checked locks, and it would\nprobably be repurposable to do pthread_once if needed:\n\nhttps://github.com/kusma/git/commit/c2d7190ed64652c3ac8ea1f2800ff8e7d0b6c01e\n\nYes, it's not pretty, it's error-prone; the best thing would be not to do it.\n"}]}