{"thread":{"id":"26948","subject":"[PATCH 0/4] fix hang in git push when pack-objects fails","startedAt":"2011-03-31T18:42:44Z","lastAt":"2011-04-26T08:23:02Z","messageCount":48,"participants":["Jeff King","Johannes Sixt","Erik Faye-Lund","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"164812","messageId":"20110331184243.GA12027@sigill.intra.peff.net","threadId":"26948","inReplyTo":null,"subject":"[PATCH 0/4] fix hang in git push when pack-objects fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-31T18:42:44Z","receivedAt":"2011-03-31T18:42:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we push over the git protocol, we spawn a pack-object process. If\npack-objects dies prematurely (for example, because there is some repo\ncorruption), we are careful to clean up the sideband demuxer (if it is\nbeing used) with finish_async. However, for an async implementation\nwhich forks (i.e., when we have no pthreads), that means we will\nwaitpid() for the async process.\n\nMeanwhile, the async sideband demuxer will continue trying to stream\ndata from the remote repo until it gets EOF. Depending on what data\npack-objects actually sent, the remote repo may not actually send\nanything (e.g., if we sent nothing and it is simply waiting for the\npack). This leads to deadlock cycle in which send-pack waits on the\ndemuxer, the demuxer waits on the remote receive-pack, and the remote\nreceive-pack waits on send-pack to send the pack data.\n\nYou can test this by compiling with NO_PTHREADS=1 and running the\nfollowing script:\n\n-- >8 --\n#!/bin/sh\n\nrm -rf parent child\n\ngit init --bare parent &&\ngit init child &&\ncd child &&\ngit remote add origin ../parent &&\necho content >file &&\ngit add file &&\ngit commit -m one &&\ngit push origin HEAD &&\necho content >>file &&\ngit add file &&\ngit commit -m two\n\nsha1=`git rev-parse HEAD:file`\nfile=`echo $sha1 | sed 's,..,&/,'`\nrm -fv .git/objects/$file\n\ngit push\n-- 8< --\n\nThe problem bisects to 38a81b4 (receive-pack: Wrap status reports inside\nside-band-64k, 2010-02-05). In fact, at that point in time we didn't use\npthreads at all for async calls on non-win32 platforms, so even people\nwith pthreads are affected. For example, you can trigger it with\nv1.7.0.2 even with pthreads, since we didn't use them for async code\nback then. That state continued until f6b6098 (Enable threaded async\nprocedures whenever pthreads is available, 2010-03-09), at which point\nthe problem went away for pthreads users.\n\nSo what I did was build a maint series straight on top of 38a81b4,\nthe source of the bug:\n\n [1/4]: teach wait_or_whine a \"quiet\" mode\n [2/4]: finish_async: be quiet when waiting for async process\n [3/4]: run-command: allow aborting async code prematurely\n [4/4]: send-pack: abort sideband demuxer on pack-objects error\n\nThe first two are refactoring so that aborting async code does not\nproduce a stray \"child process died with signal 15\" message. If they're\ntoo invasive, they can go away and we can live with the extra message.\n\nThe third one introduces abort_async, which just kill()s the\nforked process for the non-win32 case. For the win32 case, we need to\neither:\n\n 1. do nothing. I'm not 100% sure why, but the bug does not manifest\n itself with pthreads. I don't know how it behaves on win32.\n\n 2. do the equivalent of pthread_cancel. This makes more sense to me.\n Even if this particular case doesn't have an issue in the threaded\n case, having a primitive like abort_async actually kill the thread\n is sensible.\n\nSo that fixes old versions. To fix newer ones, it needs to be merged\nwith f6b6098, which has a few conflicts. And then you can apply on top\nof that merge:\n\n [5/4]: run-command: implement abort_async for pthreads\n\nwhich will again break win32, as the compat wrapper doesn't implement\npthread_cancel.\n\nIf it's easier to pull than to recreate the merge, you can find the\nfirst series based on 38a81b4 here:\n\n git://github.com/peff/git.git jk/maint-push-async-hang\n\nand then the merge, with conflict resolution, and 5/4 on top here:\n\n git://github.com/peff/git.git jk/maint-push-async-hang-threads\n\nIf it's easier, I can also build the fix on top of v1.7.2, which was the\nfirst release with async pthreads, and then we can either call that \"old\nenough\", or I can backport it via cherry-pick to the source.\n\n-Peff\n"},{"id":"164814","messageId":"20110331184342.GA16906@sigill.intra.peff.net","threadId":"26948","inReplyTo":"20110331184243.GA12027@sigill.intra.peff.net","subject":"[PATCH 1/4] teach wait_or_whine a \"quiet\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-31T18:43:42Z","receivedAt":"2011-03-31T18:43:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The wait_or_whine function will complain to stderr in a few\ncases:\n\n 1. We fail to actually waitpid() correctly.\n\n 2. The child died of a signal.\n\n 3. The child returned exit code 127, indicating a missing\n command to exec after forking.\n\nWe already have a silent_exec_failure flag to silence (3).\nLet's convert that into a \"quiet\" flag to also silence (2).\nThis shouldn't result in signal failure being silent for\nexisting users of silent_exec_failure, since they already\nwill need to be checking the return code and complaining for\nthe case of a non-zero exit code.\n\nFor (1), it probably makes sense to always complain about a\nfailure to correctly wait, so let's not quiet that.\n\nSigned-off-by: Jeff King \n---\n run-command.c | 7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 0d95340..0d5626a 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -240,7 +240,7 @@ fail_pipe:\n \treturn 0;\n }\n \n-static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n+static int wait_or_whine(pid_t pid, const char *argv0, int quiet)\n {\n \tint status, code = -1;\n \tpid_t waiting;\n@@ -256,7 +256,8 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \t\terror(\"waitpid is confused (%s)\", argv0);\n \t} else if (WIFSIGNALED(status)) {\n \t\tcode = WTERMSIG(status);\n-\t\terror(\"%s died of signal %d\", argv0, code);\n+\t\tif (!quiet)\n+\t\t\terror(\"%s died of signal %d\", argv0, code);\n \t\t/*\n \t\t * This return value is chosen so that code & 0xff\n \t\t * mimics the exit code that a POSIX shell would report for\n@@ -271,7 +272,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \t\tif (code == 127) {\n \t\t\tcode = -1;\n \t\t\tfailed_errno = ENOENT;\n-\t\t\tif (!silent_exec_failure)\n+\t\t\tif (!quiet)\n \t\t\t\terror(\"cannot run %s: %s\", argv0,\n \t\t\t\t\tstrerror(ENOENT));\n \t\t}\n-- \n1.7.4.13.g8566c\n"},{"id":"164815","messageId":"20110331184400.GB16906@sigill.intra.peff.net","threadId":"26948","inReplyTo":"20110331184243.GA12027@sigill.intra.peff.net","subject":"[PATCH 2/4] finish_async: be quiet when waiting for async process","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-31T18:44:00Z","receivedAt":"2011-03-31T18:44:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we ask to finish_async, we should be quiet about things\nlike signal death in the async process, for two reasons:\n\n 1. This better matches what happens on the Windows side,\n where threads are used (and what will eventually happen\n when pthreads are used on unix).\n\n 2. We pass along the error code to the caller, who needs\n to check and produce an error message anyway for the\n case of a non-zero exit code.\n\nSigned-off-by: Jeff King \n---\n run-command.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 0d5626a..258c880 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -427,7 +427,7 @@ error:\n int finish_async(struct async *async)\n {\n #ifndef WIN32\n-\tint ret = wait_or_whine(async->pid, \"child process\", 0);\n+\tint ret = wait_or_whine(async->pid, \"child process\", 1);\n #else\n \tDWORD ret = 0;\n \tif (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0)\n-- \n1.7.4.13.g8566c\n"},{"id":"164816","messageId":"20110331184421.GC16906@sigill.intra.peff.net","threadId":"26948","inReplyTo":"20110331184243.GA12027@sigill.intra.peff.net","subject":"[PATCH 3/4] run-command: allow aborting async code prematurely","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-31T18:44:21Z","receivedAt":"2011-03-31T18:44:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We provide only two abstract promitives for async code:\nstart and finish. Where \"finish\" means to wait until the\nasync code tells us it is done. However, it may also be\nuseful for us to to tell the async code to abort right away,\nbecause whatever it was doing is no longer useful.\n\nFor a separate process, we just kill() it. For Windows, we\nneed to do whatever the equivalent to pthread_cancel is.\n\nSigned-off-by: Jeff King \n---\n run-command.c | 10 ++++++++++\n run-command.h | 1 +\n 2 files changed, 11 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 258c880..f179d2a 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -439,6 +439,16 @@ int finish_async(struct async *async)\n \treturn ret;\n }\n \n+void abort_async(struct async *async)\n+{\n+#ifndef WIN32\n+\tkill(async->pid, 15);\n+#else\n+\t/* no clue */\n+#endif\n+\tfinish_async(async);\n+}\n+\n int run_hook(const char *index_file, const char *name, ...)\n {\n \tstruct child_process hook;\ndiff --git a/run-command.h b/run-command.h\nindex 65ccb1c..df2c736 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -83,5 +83,6 @@ struct async {\n \n int start_async(struct async *async);\n int finish_async(struct async *async);\n+void abort_async(struct async *async);\n \n #endif\n-- \n1.7.4.13.g8566c\n"},{"id":"164817","messageId":"20110331184443.GD16906@sigill.intra.peff.net","threadId":"26948","inReplyTo":"20110331184243.GA12027@sigill.intra.peff.net","subject":"[PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-31T18:44:43Z","receivedAt":"2011-03-31T18:44:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If pack-objects dies prematurely (for example, because there\nis some repo corruption), we are careful to clean up the\nsideband demuxer (if it is being used) with finish_async.\nHowever, for an async implementation which forks (i.e., not\nWindows), that means we will waitpid() for the async\nprocess.\n\nMeanwhile, the async sideband demuxer will continue trying\nto stream data from the remote repo until it gets EOF.\nDepending on what data pack-objects actually sent, the\nremote repo may not actually send anything (e.g., if we sent\nnothing and it is simply waiting for the pack). This leads\nto deadlock cycle in which send-pack waits on the demuxer,\nthe demuxer waits on the remote receive-pack, and the remote\nreceive-pack waits on send-pack to send the pack data.\n\nTo break this, let's have send-pack abort the demuxer when\nit knows that it has failed to properly send the pack.\n\nSigned-off-by: Jeff King \n---\n builtin-send-pack.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 2478e18..cc068ff 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -516,7 +516,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n \t\t\t\tref->status = REF_STATUS_NONE;\n \t\t\tif (use_sideband)\n-\t\t\t\tfinish_async(&demux);\n+\t\t\t\tabort_async(&demux);\n \t\t\treturn -1;\n \t\t}\n \t}\n-- \n1.7.4.13.g8566c\n"},{"id":"164818","messageId":"20110331184546.GA16981@sigill.intra.peff.net","threadId":"26948","inReplyTo":"20110331184243.GA12027@sigill.intra.peff.net","subject":"[PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-31T18:45:46Z","receivedAt":"2011-03-31T18:45:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We just need to cancel the thread, instead of sending it a\nsignal as we do for fork()'d async sections.\n\nSigned-off-by: Jeff King \n---\nThis could also just be part of the merge resolution, but I thought it\nwould be easier to see what is going on if I put it here.\n\n run-command.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex e31c073..46ea07d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -607,7 +607,7 @@ void abort_async(struct async *async)\n #ifdef NO_PTHREADS\n \tkill(async->pid, 15);\n #else\n-\t/* no clue */\n+\tpthread_cancel(async->tid);\n #endif\n \tfinish_async(async);\n }\n-- \n1.7.4.13.g8566c\n"},{"id":"164831","messageId":"201103312245.38987.j6t@kdbg.org","threadId":"26948","inReplyTo":"20110331184243.GA12027@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] fix hang in git push when pack-objects fails","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-03-31T20:45:38Z","receivedAt":"2011-03-31T20:45:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 31. März 2011, Jeff King wrote:\n>   1. do nothing. I'm not 100% sure why, but the bug does not manifest\n>      itself with pthreads. I don't know how it behaves on win32.\n\nThe reason might be that with threads we carefully close file descriptors, so \nthat the other end sees EOF or broken pipe and terminates, while with a \nforked sideband demux one of the channels remains open in the forked process?\n\n-- Hannes\n"},{"id":"164832","messageId":"201103312256.39317.j6t@kdbg.org","threadId":"26948","inReplyTo":"20110331184342.GA16906@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] teach wait_or_whine a \"quiet\" mode","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-03-31T20:56:39Z","receivedAt":"2011-03-31T20:56:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 31. März 2011, Jeff King wrote:\n> The wait_or_whine function will complain to stderr in a few\n> cases:\n>\n> 1. We fail to actually waitpid() correctly.\n>\n> 2. The child died of a signal.\n>\n> 3. The child returned exit code 127, indicating a missing\n> command to exec after forking.\n>\n> We already have a silent_exec_failure flag to silence (3).\n> Let's convert that into a \"quiet\" flag to also silence (2).\n\nI'm rather negative on controlling these two error reports with the same flag \nbecause...\n\n> This shouldn't result in signal failure being silent for\n> existing users of silent_exec_failure, since they already\n> will need to be checking the return code and complaining for\n> the case of a non-zero exit code.\n\nThis reasoning is not correct. Error reporting in the routines in \nrun-command.c is structured such that callers have to check the return code, \nbut they do not report errors themselves. Therefore, even if callers request \nsilent_exec_failure, they will not do their own error reporting if there is \nsome other failure.\n\n-- Hannes\n"},{"id":"164848","messageId":"20110401013404.GA21036@sigill.intra.peff.net","threadId":"26948","inReplyTo":"201103312245.38987.j6t@kdbg.org","subject":"Re: [PATCH 0/4] fix hang in git push when pack-objects fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T01:34:04Z","receivedAt":"2011-04-01T01:34:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 31, 2011 at 10:45:38PM +0200, Johannes Sixt wrote:\n\n> On Donnerstag, 31. März 2011, Jeff King wrote:\n> >   1. do nothing. I'm not 100% sure why, but the bug does not manifest\n> >      itself with pthreads. I don't know how it behaves on win32.\n> \n> The reason might be that with threads we carefully close file descriptors, so \n> that the other end sees EOF or broken pipe and terminates, while with a \n> forked sideband demux one of the channels remains open in the forked process?\n\nThat was my thought, too, except that:\n\n 1. I don't see us closing the descriptor after we fork the async code\n off. So even if the async code carefully closed it, the main\n process would still have it open.\n\n 2. In some cases it may not even be a file descriptor that needs\n closed, but rather a half-duplex shutdown (e.g., for git-over-tcp).\n Because the point of the async code is that it is reading from the\n remote socket.\n\nSo I'm really not sure. I can do some straces and try to investigate\nfurther on what happens in the pthread case. But regardless, it seems\nlike we should be killing off any async processes explicitly on error.\n\n-Peff\n"},{"id":"164850","messageId":"20110401013519.GB21036@sigill.intra.peff.net","threadId":"26948","inReplyTo":"201103312256.39317.j6t@kdbg.org","subject":"Re: [PATCH 1/4] teach wait_or_whine a \"quiet\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T01:35:20Z","receivedAt":"2011-04-01T01:35:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 31, 2011 at 10:56:39PM +0200, Johannes Sixt wrote:\n\n> On Donnerstag, 31. März 2011, Jeff King wrote:\n> > The wait_or_whine function will complain to stderr in a few\n> > cases:\n> >\n> > 1. We fail to actually waitpid() correctly.\n> >\n> > 2. The child died of a signal.\n> >\n> > 3. The child returned exit code 127, indicating a missing\n> > command to exec after forking.\n> >\n> > We already have a silent_exec_failure flag to silence (3).\n> > Let's convert that into a \"quiet\" flag to also silence (2).\n> \n> I'm rather negative on controlling these two error reports with the same flag \n> because...\n\nFair enough. We can add an extra flag, then, or just drop both patches\nentirely. They are just trying to save the \"child process killed\"\nmessage when we abort a separate-process async.\n\n-Peff\n"},{"id":"164877","messageId":"AANLkTinAQiuYjs+pxBVKM2-aQNSDd_-CC_fMahasTB=V@mail.gmail.com","threadId":"26948","inReplyTo":"20110331184421.GC16906@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] run-command: allow aborting async code prematurely","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T09:36:53Z","receivedAt":"2011-04-01T09:36:53Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Mar 31, 2011 at 8:44 PM, Jeff King wrote:\n> We provide only two abstract promitives for async code:\n> start and finish. Where \"finish\" means to wait until the\n> async code tells us it is done. However, it may also be\n> useful for us to to tell the async code to abort right away,\n> because whatever it was doing is no longer useful.\n>\n> For a separate process, we just kill() it. For Windows, we\n> need to do whatever the equivalent to pthread_cancel is.\n>\n> Signed-off-by: Jeff King \n> ---\n>  run-command.c |   10 ++++++++++\n>  run-command.h |    1 +\n>  2 files changed, 11 insertions(+), 0 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 258c880..f179d2a 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -439,6 +439,16 @@ int finish_async(struct async *async)\n>        return ret;\n>  }\n>\n> +void abort_async(struct async *async)\n> +{\n> +#ifndef WIN32\n> +       kill(async->pid, 15);\n\nThis doesn't compile unless NO_PTHREADS is set, no?\n\n> +#else\n> +       /* no clue */\n> +#endif\n> +       finish_async(async);\n> +}\n> +\n\nThis should probably be\n\nvoid abort_async(struct async *async)\n{\n#ifdef NO_PTHREADS\n\tkill(async->pid, 15);\n#else\n\tpthread_cancel(async->tid)\n#endif\n\tfinish_async(async);\n}\n\n... and then us Windows-guys must implement something like pthread_cancel().\n\nOr maybe not. Can pthread reliably cancel a thread while making sure\nthat thread isn't holding a mutex (like some thread-safe malloc\nimplementations do)? If not, I'm not entirely sure we can even reach\nthis goal.\n"},{"id":"164878","messageId":"AANLkTikF_xLTdY0BgaXmUXvBJTG3HD3by9zN59KwPii5@mail.gmail.com","threadId":"26948","inReplyTo":"20110331184546.GA16981@sigill.intra.peff.net","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T09:41:12Z","receivedAt":"2011-04-01T09:41:12Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Mar 31, 2011 at 8:45 PM, Jeff King wrote:\n> We just need to cancel the thread, instead of sending it a\n> signal as we do for fork()'d async sections.\n>\n> Signed-off-by: Jeff King \n> ---\n> This could also just be part of the merge resolution, but I thought it\n> would be easier to see what is going on if I put it here.\n>\n>  run-command.c |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index e31c073..46ea07d 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -607,7 +607,7 @@ void abort_async(struct async *async)\n>  #ifdef NO_PTHREADS\n\nThis context-line doesn't match 3/4... Did you send out the wrong\nversion of that patch?\n\n>        kill(async->pid, 15);\n>  #else\n> -       /* no clue */\n> +       pthread_cancel(async->tid);\n\nMy worry about terminating a thread that's currently holding a mutex\n(implicitly through the CRT?) still applies though...\n"},{"id":"164880","messageId":"AANLkTin3+PzYcQEaaxgr9_5mgDGO5oiYDyBD_2tJqCTk@mail.gmail.com","threadId":"26948","inReplyTo":"AANLkTikF_xLTdY0BgaXmUXvBJTG3HD3by9zN59KwPii5@mail.gmail.com","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T10:15:50Z","receivedAt":"2011-04-01T10:15:50Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 1, 2011 at 11:41 AM, Erik Faye-Lund wrote:\n> On Thu, Mar 31, 2011 at 8:45 PM, Jeff King wrote:\n>> We just need to cancel the thread, instead of sending it a\n>> signal as we do for fork()'d async sections.\n>>\n>> Signed-off-by: Jeff King \n>> ---\n>> This could also just be part of the merge resolution, but I thought it\n>> would be easier to see what is going on if I put it here.\n>>\n>>  run-command.c |    2 +-\n>>  1 files changed, 1 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index e31c073..46ea07d 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -607,7 +607,7 @@ void abort_async(struct async *async)\n>>  #ifdef NO_PTHREADS\n>\n> This context-line doesn't match 3/4... Did you send out the wrong\n> version of that patch?\n>\n>>        kill(async->pid, 15);\n>>  #else\n>> -       /* no clue */\n>> +       pthread_cancel(async->tid);\n>\n> My worry about terminating a thread that's currently holding a mutex\n> (implicitly through the CRT?) still applies though...\n>\n\nOK, I've read up on thread-cancellation, and this code seems correct.\npthread_cancel doesn't kill the thread right away, it just signals a\ncancellation-event, which is checked for at certain\ncancellation-points. A lot of the CRT functions are defined as\ncancellation points, so it'll be a matter for us Win32-guys to\nimplement pthread_testcancel() and inject that into the\nfunction-wrappers of the CRT functions that are marked as\ncancellation-points.\n"},{"id":"164886","messageId":"20110401135910.GA1650@sigill.intra.peff.net","threadId":"26948","inReplyTo":"AANLkTinAQiuYjs+pxBVKM2-aQNSDd_-CC_fMahasTB=V@mail.gmail.com","subject":"Re: [PATCH 3/4] run-command: allow aborting async code prematurely","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T13:59:11Z","receivedAt":"2011-04-01T13:59:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2011 at 11:36:53AM +0200, Erik Faye-Lund wrote:\n\n> > diff --git a/run-command.c b/run-command.c\n> > index 258c880..f179d2a 100644\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -439,6 +439,16 @@ int finish_async(struct async *async)\n> >        return ret;\n> >  }\n> >\n> > +void abort_async(struct async *async)\n> > +{\n> > +#ifndef WIN32\n> > +       kill(async->pid, 15);\n> \n> This doesn't compile unless NO_PTHREADS is set, no?\n\nRead the cover letter again. This goes on top of an old version,\npre-pthreads. Patch 5/4 modernizes it.\n\n> This should probably be\n> \n> void abort_async(struct async *async)\n> {\n> #ifdef NO_PTHREADS\n> \tkill(async->pid, 15);\n> #else\n> \tpthread_cancel(async->tid)\n> #endif\n> \tfinish_async(async);\n> }\n\nYes, eventually it becomes that, in 5/4.\n\n> ... and then us Windows-guys must implement something like pthread_cancel().\n> \n> Or maybe not. Can pthread reliably cancel a thread while making sure\n> that thread isn't holding a mutex (like some thread-safe malloc\n> implementations do)? If not, I'm not entirely sure we can even reach\n> this goal.\n\nYes, as you figured out in a later email, there are cancellation points.\nHowever, I'm not sure it matters for this. Run-command's async code is a\nvery abstract primitive that can even be implemented in a separate\nprocess. So async code must not use mutexes or assume it shares memory\nwith the parent. Real threaded code should use pthreads directly (and\nmust be optional, or we alienate non-threaded platforms).\n\n-Peff\n"},{"id":"164889","messageId":"20110401140011.GB1650@sigill.intra.peff.net","threadId":"26948","inReplyTo":"AANLkTikF_xLTdY0BgaXmUXvBJTG3HD3by9zN59KwPii5@mail.gmail.com","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T14:00:11Z","receivedAt":"2011-04-01T14:00:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2011 at 11:41:12AM +0200, Erik Faye-Lund wrote:\n\n> On Thu, Mar 31, 2011 at 8:45 PM, Jeff King wrote:\n> > We just need to cancel the thread, instead of sending it a\n> > signal as we do for fork()'d async sections.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > This could also just be part of the merge resolution, but I thought it\n> > would be easier to see what is going on if I put it here.\n> >\n> >  run-command.c |    2 +-\n> >  1 files changed, 1 insertions(+), 1 deletions(-)\n> >\n> > diff --git a/run-command.c b/run-command.c\n> > index e31c073..46ea07d 100644\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -607,7 +607,7 @@ void abort_async(struct async *async)\n> >  #ifdef NO_PTHREADS\n> \n> This context-line doesn't match 3/4... Did you send out the wrong\n> version of that patch?\n\nNo, read the cover letter. After 4/4, you have to merge the fix up to a\nmore modern version with pthreads, then this goes on top of the\nresolution.\n\n-Peff\n"},{"id":"164899","messageId":"201104011927.03366.j6t@kdbg.org","threadId":"26948","inReplyTo":"AANLkTin3+PzYcQEaaxgr9_5mgDGO5oiYDyBD_2tJqCTk@mail.gmail.com","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-01T17:27:03Z","receivedAt":"2011-04-01T17:27:03Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n> On Fri, Apr 1, 2011 at 11:41 AM, Erik Faye-Lund wrote:\n> > On Thu, Mar 31, 2011 at 8:45 PM, Jeff King wrote:\n> >>        kill(async->pid, 15);\n> >>  #else\n> >> -       /* no clue */\n> >> +       pthread_cancel(async->tid);\n> >\n> > My worry about terminating a thread that's currently holding a mutex\n> > (implicitly through the CRT?) still applies though...\n>\n> OK, I've read up on thread-cancellation, and this code seems correct.\n> pthread_cancel doesn't kill the thread right away, it just signals a\n> cancellation-event, which is checked for at certain\n> cancellation-points. A lot of the CRT functions are defined as\n> cancellation points, so it'll be a matter for us Win32-guys to\n> implement pthread_testcancel() and inject that into the\n> function-wrappers of the CRT functions that are marked as\n> cancellation-points.\n\nThat's not going to happen. We cannot implement pthread_cancel() on Windows \nbecause it would have to be able to interrupt blocking system calls. \n(TerminateThread() is a no-no, given all the caveats about leaking system \nresources that are mentioned in the manual.)\n\n[OK, \"cannot\" is a hard word. It is possible in some way, I'm sure. But that \nwould mean that we implement the equivalent of Cygwin or so...]\n\nBut if I understand correctly what Jeff wrote so far, then the pthreaded case \nhappens to work - by chance or by design, we don't know (yet). Perhaps we can \nget away with\n\n-\t/* no clue */\n+\t/* pthread_cancel(async->tid); not necessary */\n\n-- Hannes\n"},{"id":"164900","messageId":"20110401173808.GA24934@sigill.intra.peff.net","threadId":"26948","inReplyTo":"201104011927.03366.j6t@kdbg.org","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T17:38:08Z","receivedAt":"2011-04-01T17:38:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2011 at 07:27:03PM +0200, Johannes Sixt wrote:\n\n> > OK, I've read up on thread-cancellation, and this code seems correct.\n> > pthread_cancel doesn't kill the thread right away, it just signals a\n> > cancellation-event, which is checked for at certain\n> > cancellation-points. A lot of the CRT functions are defined as\n> > cancellation points, so it'll be a matter for us Win32-guys to\n> > implement pthread_testcancel() and inject that into the\n> > function-wrappers of the CRT functions that are marked as\n> > cancellation-points.\n> \n> That's not going to happen. We cannot implement pthread_cancel() on Windows \n> because it would have to be able to interrupt blocking system calls. \n> (TerminateThread() is a no-no, given all the caveats about leaking system \n> resources that are mentioned in the manual.)\n> \n> [OK, \"cannot\" is a hard word. It is possible in some way, I'm sure. But that \n> would mean that we implement the equivalent of Cygwin or so...]\n> \n> But if I understand correctly what Jeff wrote so far, then the pthreaded case \n> happens to work - by chance or by design, we don't know (yet). Perhaps we can \n> get away with\n> \n> -\t/* no clue */\n> +\t/* pthread_cancel(async->tid); not necessary */\n\nYeah, I think that would probably work, but I haven't had a chance yet\nto look deeper into why the pthread case doesn't hang.\n\nI have another case, too, which is that killing a \"git push\" in progress\nvia signal will leave crufty child-processes around, still trying to\npush. One of these is the pack-objects sub-process, and the other is\n(in the no-pthreads case) the sideband demuxer.\n\nAnd obviously fixing that involves aborting the async process, too[1].\nBut we can again get away without pthread_cancel, because in the pthread\ncase, we can just rely on the parent process dying to take down the\nthread.\n\n-Peff\n\n[1] Actually, my plan is to set up a signal/atexit handler to kill off\nchildren. Run-command callers can specify an option for \"yes, this\nchild should be killed if I am killed\". Async callers will have it\nturned on automatically (since they won't even know if it's a subthread\nor a different process). So we won't actually be calling abort_async()\nanyway.\n"},{"id":"164911","messageId":"AANLkTi=8+V3w-1uQgekjntM1qREGZYWSTE_ra1NbDxG8@mail.gmail.com","threadId":"26948","inReplyTo":"201104011927.03366.j6t@kdbg.org","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T19:26:41Z","receivedAt":"2011-04-01T19:26:41Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 1, 2011 at 7:27 PM, Johannes Sixt wrote:\n> On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n>> On Fri, Apr 1, 2011 at 11:41 AM, Erik Faye-Lund wrote:\n>> > On Thu, Mar 31, 2011 at 8:45 PM, Jeff King wrote:\n>> >>        kill(async->pid, 15);\n>> >>  #else\n>> >> -       /* no clue */\n>> >> +       pthread_cancel(async->tid);\n>> >\n>> > My worry about terminating a thread that's currently holding a mutex\n>> > (implicitly through the CRT?) still applies though...\n>>\n>> OK, I've read up on thread-cancellation, and this code seems correct.\n>> pthread_cancel doesn't kill the thread right away, it just signals a\n>> cancellation-event, which is checked for at certain\n>> cancellation-points. A lot of the CRT functions are defined as\n>> cancellation points, so it'll be a matter for us Win32-guys to\n>> implement pthread_testcancel() and inject that into the\n>> function-wrappers of the CRT functions that are marked as\n>> cancellation-points.\n>\n> That's not going to happen. We cannot implement pthread_cancel() on Windows\n> because it would have to be able to interrupt blocking system calls.\n> (TerminateThread() is a no-no, given all the caveats about leaking system\n> resources that are mentioned in the manual.)\n>\n\nDid you read my suggestion? I was talking about implementing\ncancellation-points, just like on other platforms. This should not\nlead to TerminateThread, but instead a conditional ExitThread from the\nthread in question.\n\nSomething like this (I've only added a cancellation-point at close,\njust to illustrate what I mean):\n\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 878b1de..253be14 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -302,6 +302,14 @@ int mingw_open (const char *filename, int oflags, ...)\n \treturn fd;\n }\n\n+#undef close\n+int mingw_close(int fd)\n+{\n+\tint ret = close(fd);\n+\tpthread_testcancel();\n+\treturn ret;\n+}\n+\n #undef write\n ssize_t mingw_write(int fd, const void *buf, size_t count)\n {\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 62eccd3..3e904c8 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -175,6 +175,9 @@ int mingw_rmdir(const char *path);\n int mingw_open (const char *filename, int oflags, ...);\n #define open mingw_open\n\n+int mingw_close(int fd);\n+#define close mingw_close\n+\n ssize_t mingw_write(int fd, const void *buf, size_t count);\n #define write mingw_write\n\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nindex 010e875..47d620b 100644\n--- a/compat/win32/pthread.c\n+++ b/compat/win32/pthread.c\n@@ -18,6 +18,7 @@ static unsigned __stdcall win32_start_routine(void *arg)\n \tpthread_t *thread = arg;\n \tthread->tid = GetCurrentThreadId();\n \tthread->arg = thread->start_routine(thread->arg);\n+\tCloseHandle(thread->cancel_event);\n \treturn 0;\n }\n\n@@ -26,6 +27,9 @@ int pthread_create(pthread_t *thread, const void *unused,\n {\n \tthread->arg = arg;\n \tthread->start_routine = start_routine;\n+\tthread->cancel_event = CreateEvent(NULL, FALSE, FALSE, NULL);\n+\tif (!thread->cancel_event)\n+\t\tdie(\"failed to create event\");\n \tthread->handle = (HANDLE)\n \t\t_beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);\n\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nindex 2e20548..7fce39d 100644\n--- a/compat/win32/pthread.h\n+++ b/compat/win32/pthread.h\n@@ -56,6 +56,7 @@ extern int pthread_cond_broadcast(pthread_cond_t *cond);\n */\n typedef struct {\n \tHANDLE handle;\n+\tHANDLE cancel_event;\n \tvoid *(*start_routine)(void*);\n \tvoid *arg;\n \tDWORD tid;\n@@ -96,4 +97,13 @@ static inline void *pthread_getspecific(pthread_key_t key)\n \treturn TlsGetValue(key);\n }\n\n+#define pthread_cancel(a) SetEvent(a.cancel_event)\n+\n+static inline void pthread_testcancel(void)\n+{\n+\tpthread_t thread = pthread_self();\n+\tif (WaitForSingleObject(thread.cancel_event, 0))\n+\t\tpthread_exit(NULL);\n+}\n+\n #endif /* PTHREAD_H */\n"},{"id":"164912","messageId":"AANLkTimk4cLDjj0xKgshm-Xqh7G2FeNA5Y-DXLmkEi3s@mail.gmail.com","threadId":"26948","inReplyTo":"AANLkTi=8+V3w-1uQgekjntM1qREGZYWSTE_ra1NbDxG8@mail.gmail.com","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T19:33:46Z","receivedAt":"2011-04-01T19:33:46Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 1, 2011 at 9:26 PM, Erik Faye-Lund wrote:\n> On Fri, Apr 1, 2011 at 7:27 PM, Johannes Sixt wrote:\n>> On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n>>> On Fri, Apr 1, 2011 at 11:41 AM, Erik Faye-Lund wrote:\n>>> > On Thu, Mar 31, 2011 at 8:45 PM, Jeff King wrote:\n>>> >>        kill(async->pid, 15);\n>>> >>  #else\n>>> >> -       /* no clue */\n>>> >> +       pthread_cancel(async->tid);\n>>> >\n>>> > My worry about terminating a thread that's currently holding a mutex\n>>> > (implicitly through the CRT?) still applies though...\n>>>\n>>> OK, I've read up on thread-cancellation, and this code seems correct.\n>>> pthread_cancel doesn't kill the thread right away, it just signals a\n>>> cancellation-event, which is checked for at certain\n>>> cancellation-points. A lot of the CRT functions are defined as\n>>> cancellation points, so it'll be a matter for us Win32-guys to\n>>> implement pthread_testcancel() and inject that into the\n>>> function-wrappers of the CRT functions that are marked as\n>>> cancellation-points.\n>>\n>> That's not going to happen. We cannot implement pthread_cancel() on Windows\n>> because it would have to be able to interrupt blocking system calls.\n>> (TerminateThread() is a no-no, given all the caveats about leaking system\n>> resources that are mentioned in the manual.)\n>>\n>\n> Did you read my suggestion? I was talking about implementing\n> cancellation-points, just like on other platforms. This should not\n> lead to TerminateThread, but instead a conditional ExitThread from the\n> thread in question.\n>\n> Something like this (I've only added a cancellation-point at close,\n> just to illustrate what I mean):\n>\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 878b1de..253be14 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -302,6 +302,14 @@ int mingw_open (const char *filename, int oflags, ...)\n>        return fd;\n>  }\n>\n> +#undef close\n> +int mingw_close(int fd)\n> +{\n> +       int ret = close(fd);\n> +       pthread_testcancel();\n> +       return ret;\n> +}\n> +\n>  #undef write\n>  ssize_t mingw_write(int fd, const void *buf, size_t count)\n>  {\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index 62eccd3..3e904c8 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -175,6 +175,9 @@ int mingw_rmdir(const char *path);\n>  int mingw_open (const char *filename, int oflags, ...);\n>  #define open mingw_open\n>\n> +int mingw_close(int fd);\n> +#define close mingw_close\n> +\n>  ssize_t mingw_write(int fd, const void *buf, size_t count);\n>  #define write mingw_write\n>\n> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\n> index 010e875..47d620b 100644\n> --- a/compat/win32/pthread.c\n> +++ b/compat/win32/pthread.c\n> @@ -18,6 +18,7 @@ static unsigned __stdcall win32_start_routine(void *arg)\n>        pthread_t *thread = arg;\n>        thread->tid = GetCurrentThreadId();\n>        thread->arg = thread->start_routine(thread->arg);\n> +       CloseHandle(thread->cancel_event);\n>        return 0;\n>  }\n>\n> @@ -26,6 +27,9 @@ int pthread_create(pthread_t *thread, const void *unused,\n>  {\n>        thread->arg = arg;\n>        thread->start_routine = start_routine;\n> +       thread->cancel_event = CreateEvent(NULL, FALSE, FALSE, NULL);\n> +       if (!thread->cancel_event)\n> +               die(\"failed to create event\");\n>        thread->handle = (HANDLE)\n>                _beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);\n>\n> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\n> index 2e20548..7fce39d 100644\n> --- a/compat/win32/pthread.h\n> +++ b/compat/win32/pthread.h\n> @@ -56,6 +56,7 @@ extern int pthread_cond_broadcast(pthread_cond_t *cond);\n>  */\n>  typedef struct {\n>        HANDLE handle;\n> +       HANDLE cancel_event;\n>        void *(*start_routine)(void*);\n>        void *arg;\n>        DWORD tid;\n> @@ -96,4 +97,13 @@ static inline void *pthread_getspecific(pthread_key_t key)\n>        return TlsGetValue(key);\n>  }\n>\n> +#define pthread_cancel(a) SetEvent(a.cancel_event)\n> +\n> +static inline void pthread_testcancel(void)\n> +{\n> +       pthread_t thread = pthread_self();\n> +       if (WaitForSingleObject(thread.cancel_event, 0))\n\nThat should be:\n-\tif (WaitForSingleObject(thread.cancel_event, 0))\n+\tif (WaitForSingleObject(thread.cancel_event, 0) == WAIT_OBJECT_0)\n...Of course.\n"},{"id":"164913","messageId":"201104012142.22065.j6t@kdbg.org","threadId":"26948","inReplyTo":"AANLkTi=8+V3w-1uQgekjntM1qREGZYWSTE_ra1NbDxG8@mail.gmail.com","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-01T19:42:21Z","receivedAt":"2011-04-01T19:42:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n> On Fri, Apr 1, 2011 at 7:27 PM, Johannes Sixt wrote:\n> > On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n> >> OK, I've read up on thread-cancellation, and this code seems correct.\n> >> pthread_cancel doesn't kill the thread right away, it just signals a\n> >> cancellation-event, which is checked for at certain\n> >> cancellation-points. A lot of the CRT functions are defined as\n> >> cancellation points, so it'll be a matter for us Win32-guys to\n> >> implement pthread_testcancel() and inject that into the\n> >> function-wrappers of the CRT functions that are marked as\n> >> cancellation-points.\n> >\n> > That's not going to happen. We cannot implement pthread_cancel() on\n> > Windows because it would have to be able to interrupt blocking system\n> > calls. (TerminateThread() is a no-no, given all the caveats about leaking\n> > system resources that are mentioned in the manual.)\n>\n> Did you read my suggestion?\n\nYes, I did.\n\n> I was talking about implementing \n> cancellation-points, just like on other platforms. This should not\n> lead to TerminateThread, but instead a conditional ExitThread from the\n> thread in question.\n>\n> Something like this (I've only added a cancellation-point at close,\n> just to illustrate what I mean):\n\nBut this does not help the case at hand in any way. How would you interrupt a \nthread that is blocked in ReadFile()? The point of pthread_cancel() is that \nit interrupts blocked system calls, something that you cannot achieve if you \nwant to keep using MS's C runtime.\n\n-- Hannes\n"},{"id":"164914","messageId":"AANLkTimKPD6rw1B9gG3ZN0KEOY41nk=6jE0MVcdotpkp@mail.gmail.com","threadId":"26948","inReplyTo":"201104012142.22065.j6t@kdbg.org","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T19:57:14Z","receivedAt":"2011-04-01T19:57:14Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 1, 2011 at 9:42 PM, Johannes Sixt wrote:\n> On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n>> On Fri, Apr 1, 2011 at 7:27 PM, Johannes Sixt wrote:\n>> > On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n>> >> OK, I've read up on thread-cancellation, and this code seems correct.\n>> >> pthread_cancel doesn't kill the thread right away, it just signals a\n>> >> cancellation-event, which is checked for at certain\n>> >> cancellation-points. A lot of the CRT functions are defined as\n>> >> cancellation points, so it'll be a matter for us Win32-guys to\n>> >> implement pthread_testcancel() and inject that into the\n>> >> function-wrappers of the CRT functions that are marked as\n>> >> cancellation-points.\n>> >\n>> > That's not going to happen. We cannot implement pthread_cancel() on\n>> > Windows because it would have to be able to interrupt blocking system\n>> > calls. (TerminateThread() is a no-no, given all the caveats about leaking\n>> > system resources that are mentioned in the manual.)\n>>\n>> Did you read my suggestion?\n>\n> Yes, I did.\n>\n>> I was talking about implementing\n>> cancellation-points, just like on other platforms. This should not\n>> lead to TerminateThread, but instead a conditional ExitThread from the\n>> thread in question.\n>>\n>> Something like this (I've only added a cancellation-point at close,\n>> just to illustrate what I mean):\n>\n> But this does not help the case at hand in any way. How would you interrupt a\n> thread that is blocked in ReadFile()? The point of pthread_cancel() is that\n> it interrupts blocked system calls\n\nThere is no mention of such a guarantee in POSIX (section 2.9.5 Thread\nCancellation), so relying on that is undefined behavior.\n"},{"id":"164915","messageId":"20110401200537.GA15778@sigill.intra.peff.net","threadId":"26948","inReplyTo":"AANLkTimKPD6rw1B9gG3ZN0KEOY41nk=6jE0MVcdotpkp@mail.gmail.com","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T20:05:37Z","receivedAt":"2011-04-01T20:05:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2011 at 09:57:14PM +0200, Erik Faye-Lund wrote:\n\n> > But this does not help the case at hand in any way. How would you interrupt a\n> > thread that is blocked in ReadFile()? The point of pthread_cancel() is that\n> > it interrupts blocked system calls\n> \n> There is no mention of such a guarantee in POSIX (section 2.9.5 Thread\n> Cancellation), so relying on that is undefined behavior.\n\nEh? My pthreads(7) says that read() is required to be a cancellation\npoint acrroding to POSIX. I didn't dig up the actual reference in the\nstandard, though.\n\n-Peff\n"},{"id":"164916","messageId":"AANLkTi=ro88pwpA+Gfu5p2dB3ntn8PUcwvjHRh3iRy_H@mail.gmail.com","threadId":"26948","inReplyTo":"20110401200537.GA15778@sigill.intra.peff.net","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T20:13:23Z","receivedAt":"2011-04-01T20:13:23Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 1, 2011 at 10:05 PM, Jeff King wrote:\n> On Fri, Apr 01, 2011 at 09:57:14PM +0200, Erik Faye-Lund wrote:\n>\n>> > But this does not help the case at hand in any way. How would you interrupt a\n>> > thread that is blocked in ReadFile()? The point of pthread_cancel() is that\n>> > it interrupts blocked system calls\n>>\n>> There is no mention of such a guarantee in POSIX (section 2.9.5 Thread\n>> Cancellation), so relying on that is undefined behavior.\n>\n> Eh? My pthreads(7) says that read() is required to be a cancellation\n> point acrroding to POSIX. I didn't dig up the actual reference in the\n> standard, though.\n\nI don't understand where the implementor would get that from after\nreading through it, but if there's something I've missed we can fix it\nby replacing my pthread_cancel with this, no?\n\nstatic inline int pthread_cancel(pthread_t thread)\n{\n\tSetEvent(thread.cancel_event);\n\tCancelSynchronousIo(thread.handle);\n}\n"},{"id":"164917","messageId":"20110401201714.GA16501@sigill.intra.peff.net","threadId":"26948","inReplyTo":"AANLkTi=ro88pwpA+Gfu5p2dB3ntn8PUcwvjHRh3iRy_H@mail.gmail.com","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T20:17:14Z","receivedAt":"2011-04-01T20:17:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2011 at 10:13:23PM +0200, Erik Faye-Lund wrote:\n\n> On Fri, Apr 1, 2011 at 10:05 PM, Jeff King wrote:\n> > On Fri, Apr 01, 2011 at 09:57:14PM +0200, Erik Faye-Lund wrote:\n> >\n> >> > But this does not help the case at hand in any way. How would you interrupt a\n> >> > thread that is blocked in ReadFile()? The point of pthread_cancel() is that\n> >> > it interrupts blocked system calls\n> >>\n> >> There is no mention of such a guarantee in POSIX (section 2.9.5 Thread\n> >> Cancellation), so relying on that is undefined behavior.\n> >\n> > Eh? My pthreads(7) says that read() is required to be a cancellation\n> > point acrroding to POSIX. I didn't dig up the actual reference in the\n> > standard, though.\n> \n> I don't understand where the implementor would get that from after\n> reading through it, but if there's something I've missed we can fix it\n> by replacing my pthread_cancel with this, no?\n\nOut of curiosity, which POSIX are you reading? My page references\nPOSIX.1-2001, but technically pthreads were originally defined in\nPOSIX.1c-1995.\n\n> static inline int pthread_cancel(pthread_t thread)\n> {\n> \tSetEvent(thread.cancel_event);\n> \tCancelSynchronousIo(thread.handle);\n> }\n\nThere are a ton of cancellation points, not just I/O (e.g., sleep).\nHowever, interrupting a read would probably be sufficient for git's\npurposes.\n\n-Peff\n"},{"id":"164918","messageId":"20110401201801.GA16602@sigill.intra.peff.net","threadId":"26948","inReplyTo":"20110401201714.GA16501@sigill.intra.peff.net","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T20:18:01Z","receivedAt":"2011-04-01T20:18:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2011 at 04:17:14PM -0400, Jeff King wrote:\n\n> > static inline int pthread_cancel(pthread_t thread)\n> > {\n> > \tSetEvent(thread.cancel_event);\n> > \tCancelSynchronousIo(thread.handle);\n> > }\n> \n> There are a ton of cancellation points, not just I/O (e.g., sleep).\n> However, interrupting a read would probably be sufficient for git's\n> purposes.\n\nActually, I take that back. It would be sufficient for abort_async with\nits current callers, but adding a pthread_cancel that is only partially\nthere may end up causing headaches down the road.\n\n-Peff\n"},{"id":"164919","messageId":"201104012218.07872.j6t@kdbg.org","threadId":"26948","inReplyTo":"AANLkTimKPD6rw1B9gG3ZN0KEOY41nk=6jE0MVcdotpkp@mail.gmail.com","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-01T20:18:07Z","receivedAt":"2011-04-01T20:18:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n> On Fri, Apr 1, 2011 at 9:42 PM, Johannes Sixt wrote:\n> > But this does not help the case at hand in any way. How would you\n> > interrupt a thread that is blocked in ReadFile()? The point of\n> > pthread_cancel() is that it interrupts blocked system calls\n>\n> There is no mention of such a guarantee in POSIX (section 2.9.5 Thread\n> Cancellation), so relying on that is undefined behavior.\n\nIn the paragraph before the bulleted list at the end of \"Cancellation Points\":\n\n\"...If a thread has cancelability enabled and a cancellation request is made \nwith the thread as a target while the thread is suspended at a cancellation \npoint, the thread shall be awakened and the cancellation request shall be \nacted upon...\"\n\n-- Hannes\n"},{"id":"164920","messageId":"AANLkTi=ec9azPovJoDVO6kyXEik4uFQu3dZNxAJyvDY=@mail.gmail.com","threadId":"26948","inReplyTo":"201104012218.07872.j6t@kdbg.org","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T20:31:42Z","receivedAt":"2011-04-01T20:31:42Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 1, 2011 at 10:18 PM, Johannes Sixt wrote:\n> On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n>> On Fri, Apr 1, 2011 at 9:42 PM, Johannes Sixt wrote:\n>> > But this does not help the case at hand in any way. How would you\n>> > interrupt a thread that is blocked in ReadFile()? The point of\n>> > pthread_cancel() is that it interrupts blocked system calls\n>>\n>> There is no mention of such a guarantee in POSIX (section 2.9.5 Thread\n>> Cancellation), so relying on that is undefined behavior.\n>\n> In the paragraph before the bulleted list at the end of \"Cancellation Points\":\n>\n> \"...If a thread has cancelability enabled and a cancellation request is made\n> with the thread as a target while the thread is suspended at a cancellation\n> point, the thread shall be awakened and the cancellation request shall be\n> acted upon...\"\n>\n\nA blocking thread and a suspended are two different matters. A\nsuspended thread is a thread that has been explicitly suspended by\nwait, waitpid, sleep, pause etc. These functions explicitly say that\nthey suspend the thread (\"shall suspend the calling thread until\"),\nwhile read etc does not (\"shall block the calling thread until\").\n\nSimilarly, making a blocking read/write fail (or terminate mid-way) is\nnot the same thing as awakening the thread.\n\nI see how some people can read something like this into this section,\nbut I think it's pretty clear - this is not what it's talking about.\nIn fact, the more I read the relevant texts, the more convinced I get\nthat implementations that does terminate read/write strictly speaking\nis in violation of the standard.\n"},{"id":"164921","messageId":"AANLkTinr+_9tHj8neHOxZMtKRMQ3LSVzqCPpjFup2hy=@mail.gmail.com","threadId":"26948","inReplyTo":"20110401201714.GA16501@sigill.intra.peff.net","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T20:34:30Z","receivedAt":"2011-04-01T20:34:30Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 1, 2011 at 10:17 PM, Jeff King wrote:\n> On Fri, Apr 01, 2011 at 10:13:23PM +0200, Erik Faye-Lund wrote:\n>\n>> On Fri, Apr 1, 2011 at 10:05 PM, Jeff King wrote:\n>> > On Fri, Apr 01, 2011 at 09:57:14PM +0200, Erik Faye-Lund wrote:\n>> >\n>> >> > But this does not help the case at hand in any way. How would you interrupt a\n>> >> > thread that is blocked in ReadFile()? The point of pthread_cancel() is that\n>> >> > it interrupts blocked system calls\n>> >>\n>> >> There is no mention of such a guarantee in POSIX (section 2.9.5 Thread\n>> >> Cancellation), so relying on that is undefined behavior.\n>> >\n>> > Eh? My pthreads(7) says that read() is required to be a cancellation\n>> > point acrroding to POSIX. I didn't dig up the actual reference in the\n>> > standard, though.\n>>\n>> I don't understand where the implementor would get that from after\n>> reading through it, but if there's something I've missed we can fix it\n>> by replacing my pthread_cancel with this, no?\n>\n> Out of curiosity, which POSIX are you reading? My page references\n> POSIX.1-2001, but technically pthreads were originally defined in\n> POSIX.1c-1995.\n>\n\nSorry that I wasn't more clear about this:\n\n\"The Open Group Base Specifications Issue 6\"\n\"IEEE Std 1003.1, 2004 Edition\"\n\nAvailable here:\nhttp://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_09.html\n"},{"id":"164922","messageId":"201104012236.12844.j6t@kdbg.org","threadId":"26948","inReplyTo":"AANLkTi=ro88pwpA+Gfu5p2dB3ntn8PUcwvjHRh3iRy_H@mail.gmail.com","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-01T20:36:12Z","receivedAt":"2011-04-01T20:36:12Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n> On Fri, Apr 1, 2011 at 10:05 PM, Jeff King wrote:\n> > On Fri, Apr 01, 2011 at 09:57:14PM +0200, Erik Faye-Lund wrote:\n> >> > But this does not help the case at hand in any way. How would you\n> >> > interrupt a thread that is blocked in ReadFile()? The point of\n> >> > pthread_cancel() is that it interrupts blocked system calls\n> >>\n> >> There is no mention of such a guarantee in POSIX (section 2.9.5 Thread\n> >> Cancellation), so relying on that is undefined behavior.\n> >\n> > Eh? My pthreads(7) says that read() is required to be a cancellation\n> > point acrroding to POSIX. I didn't dig up the actual reference in the\n> > standard, though.\n>\n> I don't understand where the implementor would get that from after\n> reading through it, but if there's something I've missed we can fix it\n> by replacing my pthread_cancel with this, no?\n>\n> static inline int pthread_cancel(pthread_t thread)\n> {\n> \tSetEvent(thread.cancel_event);\n> \tCancelSynchronousIo(thread.handle);\n> }\n\nDo I deserve this? There *is* a function that does what we need. I get what I \ndeserve when I don't study all 100,000 functions in the manual...\n\nWait a minute! I *did* study 100,000 functions. This is one of the 25,000 \nfunctions that are new in Vista... Those I haven't studied, yet.\n\n;)\n"},{"id":"164923","messageId":"AANLkTinQFReBnDAQrJUaCXTY7E6fqo0LySVr9QTQWV33@mail.gmail.com","threadId":"26948","inReplyTo":"201104012236.12844.j6t@kdbg.org","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T20:41:47Z","receivedAt":"2011-04-01T20:41:47Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 1, 2011 at 10:36 PM, Johannes Sixt wrote:\n> On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n>> On Fri, Apr 1, 2011 at 10:05 PM, Jeff King wrote:\n>> > On Fri, Apr 01, 2011 at 09:57:14PM +0200, Erik Faye-Lund wrote:\n>> >> > But this does not help the case at hand in any way. How would you\n>> >> > interrupt a thread that is blocked in ReadFile()? The point of\n>> >> > pthread_cancel() is that it interrupts blocked system calls\n>> >>\n>> >> There is no mention of such a guarantee in POSIX (section 2.9.5 Thread\n>> >> Cancellation), so relying on that is undefined behavior.\n>> >\n>> > Eh? My pthreads(7) says that read() is required to be a cancellation\n>> > point acrroding to POSIX. I didn't dig up the actual reference in the\n>> > standard, though.\n>>\n>> I don't understand where the implementor would get that from after\n>> reading through it, but if there's something I've missed we can fix it\n>> by replacing my pthread_cancel with this, no?\n>>\n>> static inline int pthread_cancel(pthread_t thread)\n>> {\n>>       SetEvent(thread.cancel_event);\n>>       CancelSynchronousIo(thread.handle);\n>> }\n>\n> Do I deserve this? There *is* a function that does what we need. I get what I\n> deserve when I don't study all 100,000 functions in the manual...\n>\n\nHey, it's mentioned in the ReadFile documentation!\n\n> Wait a minute! I *did* study 100,000 functions. This is one of the 25,000\n> functions that are new in Vista... Those I haven't studied, yet.\n>\n\nBah, Vista and up. That blows, we support WinXP as well! Well,\ndynamically loading the function when it's there is better than doing\nNOTHING, which was the previous alternative, no? :)\n"},{"id":"164925","messageId":"20110401211649.GA16787@sigill.intra.peff.net","threadId":"26948","inReplyTo":"AANLkTi=ec9azPovJoDVO6kyXEik4uFQu3dZNxAJyvDY=@mail.gmail.com","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T21:16:49Z","receivedAt":"2011-04-01T21:16:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2011 at 10:31:42PM +0200, Erik Faye-Lund wrote:\n\n> On Fri, Apr 1, 2011 at 10:18 PM, Johannes Sixt wrote:\n> > On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n> >> On Fri, Apr 1, 2011 at 9:42 PM, Johannes Sixt wrote:\n> >> > But this does not help the case at hand in any way. How would you\n> >> > interrupt a thread that is blocked in ReadFile()? The point of\n> >> > pthread_cancel() is that it interrupts blocked system calls\n> >>\n> >> There is no mention of such a guarantee in POSIX (section 2.9.5 Thread\n> >> Cancellation), so relying on that is undefined behavior.\n> >\n> > In the paragraph before the bulleted list at the end of \"Cancellation Points\":\n> >\n> > \"...If a thread has cancelability enabled and a cancellation request is made\n> > with the thread as a target while the thread is suspended at a cancellation\n> > point, the thread shall be awakened and the cancellation request shall be\n> > acted upon...\"\n> >\n> \n> A blocking thread and a suspended are two different matters. A\n> suspended thread is a thread that has been explicitly suspended by\n> wait, waitpid, sleep, pause etc. These functions explicitly say that\n> they suspend the thread (\"shall suspend the calling thread until\"),\n> while read etc does not (\"shall block the calling thread until\").\n> \n> Similarly, making a blocking read/write fail (or terminate mid-way) is\n> not the same thing as awakening the thread.\n> \n> I see how some people can read something like this into this section,\n> but I think it's pretty clear - this is not what it's talking about.\n> In fact, the more I read the relevant texts, the more convinced I get\n> that implementations that does terminate read/write strictly speaking\n> is in violation of the standard.\n\nWhat about the text right before the bit that Johannes quoted:\n\n The side effects of acting upon a cancellation request while suspended\n during a call of a function are the same as the side effects that may\n be seen in a single-threaded program when a call to a function is\n interrupted by a signal and the given function returns [EINTR]. Any\n such side effects occur before any cancellation cleanup handlers are\n called.\n\nI agree it would be nicer if it explicitly said \"when you are in a\nfunction which is a cancellation point, pending cancellation requests\nwhich are delivered are acuted upon immediately\".\n\nBut it is implied to me by the surrounding text, and it's the only\nsensible behavior IMHO. Plus it seems to be what at least glibc pthreads\ndoes on Linux, so I'm going to assume that people smarter than me\nthought about it and came up with the same interpretation.\n\nMy test program was:\n\n-- >8 --\n#include \n#include \n#include \n\nvoid *child(void *data)\n{\n char buf[32];\n int r;\n\n fprintf(stderr, \"child reading from stdin...\\n\");\n r = read(0, buf, sizeof(buf));\n fprintf(stderr, \"child read returned %d\\n\", r);\n return NULL;\n}\n\nint main(void)\n{\n pthread_t t;\n void *r;\n\n pthread_create(&t, NULL, child, NULL);\n sleep(3);\n pthread_cancel(t);\n\n pthread_join(t, &r);\n if (r == PTHREAD_CANCELED)\n fprintf(stderr, \"thread was canceled\\n\");\n else\n fprintf(stderr, \"thread returned %p\\n\", r);\n\n return 0;\n}\n-- >8 --\n\nIf you input something before 3 seconds is up, the thread prints its\nmessage and returns NULL. But if you let it go, the cancel interrupts\nthe read().\n\n-Peff\n"},{"id":"164949","messageId":"AANLkTikN=fSNvgmJfT8grBL=zb9fU680=K6H5h6dzWpf@mail.gmail.com","threadId":"26948","inReplyTo":"20110401211649.GA16787@sigill.intra.peff.net","subject":"Re: [PATCH 5/4] run-command: implement abort_async for pthreads","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-02T12:27:57Z","receivedAt":"2011-04-02T12:27:57Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 1, 2011 at 11:16 PM, Jeff King wrote:\n> On Fri, Apr 01, 2011 at 10:31:42PM +0200, Erik Faye-Lund wrote:\n>\n>> On Fri, Apr 1, 2011 at 10:18 PM, Johannes Sixt wrote:\n>> > On Freitag, 1. April 2011, Erik Faye-Lund wrote:\n>> >> On Fri, Apr 1, 2011 at 9:42 PM, Johannes Sixt wrote:\n>> >> > But this does not help the case at hand in any way. How would you\n>> >> > interrupt a thread that is blocked in ReadFile()? The point of\n>> >> > pthread_cancel() is that it interrupts blocked system calls\n>> >>\n>> >> There is no mention of such a guarantee in POSIX (section 2.9.5 Thread\n>> >> Cancellation), so relying on that is undefined behavior.\n>> >\n>> > In the paragraph before the bulleted list at the end of \"Cancellation Points\":\n>> >\n>> > \"...If a thread has cancelability enabled and a cancellation request is made\n>> > with the thread as a target while the thread is suspended at a cancellation\n>> > point, the thread shall be awakened and the cancellation request shall be\n>> > acted upon...\"\n>> >\n>>\n>> A blocking thread and a suspended are two different matters. A\n>> suspended thread is a thread that has been explicitly suspended by\n>> wait, waitpid, sleep, pause etc. These functions explicitly say that\n>> they suspend the thread (\"shall suspend the calling thread until\"),\n>> while read etc does not (\"shall block the calling thread until\").\n>>\n>> Similarly, making a blocking read/write fail (or terminate mid-way) is\n>> not the same thing as awakening the thread.\n>>\n>> I see how some people can read something like this into this section,\n>> but I think it's pretty clear - this is not what it's talking about.\n>> In fact, the more I read the relevant texts, the more convinced I get\n>> that implementations that does terminate read/write strictly speaking\n>> is in violation of the standard.\n>\n> What about the text right before the bit that Johannes quoted:\n>\n>  The side effects of acting upon a cancellation request while suspended\n>  during a call of a function are the same as the side effects that may\n>  be seen in a single-threaded program when a call to a function is\n>  interrupted by a signal and the given function returns [EINTR]. Any\n>  such side effects occur before any cancellation cleanup handlers are\n>  called.\n>\n\nYeah, this is much closer, because it explicitly defines the behavior\nto \"fail\" in the same way as when a signal interrupts a wait (which is\nnot simply awakening the thread). The text has the same problem for\nthis purpose as the one Johannes quoted; it talks about suspension,\nnot necessarily blocking. But it's mention of side-effects makes me\nsuspect that they mean blocking when they say suspension in this case,\nbecause none of the functions that are documented as \"blocking\" seems\nto have any side-effects in this case.\n\nSo yes, I think this is the most reasonable interpretation of this\nparagraph. Thanks for making me re-read it :)\n\n> I agree it would be nicer if it explicitly said \"when you are in a\n> function which is a cancellation point, pending cancellation requests\n> which are delivered are acuted upon immediately\".\n>\n> But it is implied to me by the surrounding text, and it's the only\n> sensible behavior IMHO.\n\nI tend to agree with you on this.\n\n> Plus it seems to be what at least glibc pthreads\n> does on Linux, so I'm going to assume that people smarter than me\n> thought about it and came up with the same interpretation.\n>\n> My test program was:\n>\n> -- >8 --\n> #include \n> #include \n> #include \n>\n> void *child(void *data)\n> {\n>  char buf[32];\n>  int r;\n>\n>  fprintf(stderr, \"child reading from stdin...\\n\");\n>  r = read(0, buf, sizeof(buf));\n>  fprintf(stderr, \"child read returned %d\\n\", r);\n>  return NULL;\n> }\n>\n> int main(void)\n> {\n>  pthread_t t;\n>  void *r;\n>\n>  pthread_create(&t, NULL, child, NULL);\n>  sleep(3);\n>  pthread_cancel(t);\n>\n>  pthread_join(t, &r);\n>  if (r == PTHREAD_CANCELED)\n>    fprintf(stderr, \"thread was canceled\\n\");\n>  else\n>    fprintf(stderr, \"thread returned %p\\n\", r);\n>\n>  return 0;\n> }\n> -- >8 --\n>\n> If you input something before 3 seconds is up, the thread prints its\n> message and returns NULL. But if you let it go, the cancel interrupts\n> the read().\n>\n\nI'm not sure I agree that measured behavior is the same as defined\noperation. But it does support the best theory we have.\n\nSo now we're left to figure out how to safely terminate a blocking\nread on versions of Windows earlier than Windows Vista. Perhaps just\nletting it time-out (assuming it does), and handle the cancellation at\nthe end of read() is acceptable (when there's no support for\nCancelSynchronousIo, that is)? After all, this deadlock hasn't been\nobserved on threaded implementations making this issue kind of\ntheoretical on Windows, no? Also, this seems to be roughly how\npthreads-win32 implements cancellation...\n"},{"id":"165775","messageId":"201104132153.06429.j6t@kdbg.org","threadId":"26948","inReplyTo":"20110331184443.GD16906@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-13T19:53:06Z","receivedAt":"2011-04-13T19:53:06Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 31. März 2011, Jeff King wrote:\n> If pack-objects dies prematurely (for example, because there\n> is some repo corruption), we are careful to clean up the\n> sideband demuxer (if it is being used) with finish_async.\n> However, for an async implementation which forks (i.e., not\n> Windows), that means we will waitpid() for the async\n> process.\n>\n> Meanwhile, the async sideband demuxer will continue trying\n> to stream data from the remote repo until it gets EOF.\n> Depending on what data pack-objects actually sent, the\n> remote repo may not actually send anything (e.g., if we sent\n> nothing and it is simply waiting for the pack). This leads\n> to deadlock cycle in which send-pack waits on the demuxer,\n> the demuxer waits on the remote receive-pack, and the remote\n> receive-pack waits on send-pack to send the pack data.\n\nThis is an indication that a writable end of the pipe between send-pack and \nreceive-pack is still open. This fixes the deadlock for me without having to \nkill the demuxer explicitly:\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 5e772c7..db32ded 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -229,6 +229,9 @@ static void print_helper_status(struct ref *ref)\n static int sideband_demux(int in, int out, void *data)\n {\n \tint *fd = data;\n+#ifdef NO_PTHREADS\n+\tclose(fd[1]);\n+#endif\n \tint ret = recv_sideband(\"send-pack\", fd[0], out);\n \tclose(out);\n \treturn ret;\n\nIf only I had a brilliant idea how to forge this into a re-usable pattern...\n\n-- Hannes\n"},{"id":"165809","messageId":"20110414135449.GD12410@sigill.intra.peff.net","threadId":"26948","inReplyTo":"201104132153.06429.j6t@kdbg.org","subject":"Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T13:54:49Z","receivedAt":"2011-04-14T13:54:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 13, 2011 at 09:53:06PM +0200, Johannes Sixt wrote:\n\n> > Meanwhile, the async sideband demuxer will continue trying\n> > to stream data from the remote repo until it gets EOF.\n> > Depending on what data pack-objects actually sent, the\n> > remote repo may not actually send anything (e.g., if we sent\n> > nothing and it is simply waiting for the pack). This leads\n> > to deadlock cycle in which send-pack waits on the demuxer,\n> > the demuxer waits on the remote receive-pack, and the remote\n> > receive-pack waits on send-pack to send the pack data.\n> \n> This is an indication that a writable end of the pipe between send-pack and \n> receive-pack is still open. This fixes the deadlock for me without having to \n> kill the demuxer explicitly:\n> \n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index 5e772c7..db32ded 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -229,6 +229,9 @@ static void print_helper_status(struct ref *ref)\n> static int sideband_demux(int in, int out, void *data)\n> {\n> \tint *fd = data;\n> +#ifdef NO_PTHREADS\n> +\tclose(fd[1]);\n> +#endif\n> \tint ret = recv_sideband(\"send-pack\", fd[0], out);\n> \tclose(out);\n> \treturn ret;\n> \n> If only I had a brilliant idea how to forge this into a re-usable pattern...\n\nThanks for finding that. I had the notion that there was a pipe end\nhanging open somewhere, but looking through the async code, I found us\nclosing the pipes properly. But of course I failed to check the fds\ncoming into send_pack.\n\nObviously it totally breaks the start_async abstraction if the called\ncode needs to care whether it forked or not. But we can use that to our\nadvantage, since it means start_async callers must assume the interface\nis very limited. So I think we can do something like:\n\n 1. Async code declares which file descriptors it cares about. This\n would automatically include the pipe we give to it, of course.\n So the declared ones for a sideband demuxer would be stderr, and\n some network fd for reading.\n\n 2. In the pthreads case, we do nothing. In the forked case, the child\n closes every descriptor except the \"interesting\" ones.\n\nAnd that solves this problem, and the general case that async-callers\nhave no idea if they have just leaked pipe descriptors in the forked\ncase.\n\nI'm still slightly confused, though, because I never see that descriptor\nget closed in the threaded case. So I still don't understand why it\n_doesn't_ deadlock with pthreads.\n\n-Peff\n"},{"id":"165833","messageId":"201104142136.25778.j6t@kdbg.org","threadId":"26948","inReplyTo":"20110414135449.GD12410@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-14T19:36:25Z","receivedAt":"2011-04-14T19:36:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 14. April 2011, Jeff King wrote:\n> Obviously it totally breaks the start_async abstraction if the called\n> code needs to care whether it forked or not. But we can use that to our\n> advantage, since it means start_async callers must assume the interface\n> is very limited. So I think we can do something like:\n>\n> 1. Async code declares which file descriptors it cares about. This\n> would automatically include the pipe we give to it, of course.\n> So the declared ones for a sideband demuxer would be stderr, and\n> some network fd for reading.\n>\n> 2. In the pthreads case, we do nothing. In the forked case, the child\n> closes every descriptor except the \"interesting\" ones.\n>\n> And that solves this problem, and the general case that async-callers\n> have no idea if they have just leaked pipe descriptors in the forked\n> case.\n\nSounds like a plan. How do you close all file descriptors? Just iterate up to \ngetrlimit(RLIMIT_NOFILE)?\n\n>\n> I'm still slightly confused, though, because I never see that descriptor\n> get closed in the threaded case. So I still don't understand why it\n> _doesn't_ deadlock with pthreads.\n\nIn the threaded case, this fd is closed by start_command(), where it is passed \nas po.out in pack_objects(). In the fork case this is too late because a \nduplicate was already inherited to the sideband demuxer.\n\nHowever, pack_objects() works differently in the stateless_rpc case: then it \ndoes not close fd anywhere, and I think it should be possible to construct a \nsimilar case that hangs even in the threaded case. And the fix could simply \nlook like this:\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 5e772c7..c8f601f 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -101,6 +101,7 @@ static int pack_objects(int fd, struct ref *refs,\n \t\tfree(buf);\n \t\tclose(po.out);\n \t\tpo.out = -1;\n+\t\tclose(fd);\n \t}\n \n \tif (finish_command(&po))\n\n-- Hannes\n"},{"id":"165837","messageId":"20110414202110.GA6525@sigill.intra.peff.net","threadId":"26948","inReplyTo":"201104142136.25778.j6t@kdbg.org","subject":"Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T20:21:10Z","receivedAt":"2011-04-14T20:21:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2011 at 09:36:25PM +0200, Johannes Sixt wrote:\n\n> On Donnerstag, 14. April 2011, Jeff King wrote:\n> > Obviously it totally breaks the start_async abstraction if the called\n> > code needs to care whether it forked or not. But we can use that to our\n> > advantage, since it means start_async callers must assume the interface\n> > is very limited. So I think we can do something like:\n> >\n> > 1. Async code declares which file descriptors it cares about. This\n> > would automatically include the pipe we give to it, of course.\n> > So the declared ones for a sideband demuxer would be stderr, and\n> > some network fd for reading.\n> >\n> > 2. In the pthreads case, we do nothing. In the forked case, the child\n> > closes every descriptor except the \"interesting\" ones.\n> >\n> > And that solves this problem, and the general case that async-callers\n> > have no idea if they have just leaked pipe descriptors in the forked\n> > case.\n> \n> Sounds like a plan. How do you close all file descriptors? Just iterate up to \n> getrlimit(RLIMIT_NOFILE)?\n\nSadly, yes, I think that is what we would have to do. It does feel like\nan awful hack. And it will interact badly with things like valgrind,\nwhich open descriptors behind the scenes (but can properly handle\nthe forking).\n\nI just don't see another way around it for the general case. The\n\"usual\" fix for this sort of thing is that the descriptors should have\nclose-on-exec set, but that doesn't work for us here, because we are\nonly forking.\n\nIt's sufficiently ugly (and still possible to break in the pthreads\ncase!) that it may be worth not worrying about the general case at all,\nand just fixing this one with the explicit close.\n\n> > I'm still slightly confused, though, because I never see that descriptor\n> > get closed in the threaded case. So I still don't understand why it\n> > _doesn't_ deadlock with pthreads.\n> \n> In the threaded case, this fd is closed by start_command(), where it is passed \n> as po.out in pack_objects(). In the fork case this is too late because a \n> duplicate was already inherited to the sideband demuxer.\n\nHrm, I see the code now. That seems like an odd thing to do to me.\nDoesn't it disallow:\n\n /* set up a command */\n const char **argv = { \"some\", \"command\" };\n struct child_process c;\n c.argv = argv;\n c.out = fd;\n\n /* run it */\n run_command(&c);\n\n /* now tack our own output to the end */\n write(fd, \"foo\", 3);\n\nAnd even weirder, we only do the close for high file descriptors. So you\n_can_ do that above if \"fd\" is stdout, but not with an arbitrary fd.\n\nI guess it is neither here nor there with respect to this problem; it\nclearly is not something we want to do a lot, as it doesn't seem to have\ncome up.\n\nBut at least it explains what's going on here in the threaded case.\n\n> However, pack_objects() works differently in the stateless_rpc case: then it \n> does not close fd anywhere, and I think it should be possible to construct a \n> similar case that hangs even in the threaded case. And the fix could simply \n> look like this:\n> \n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index 5e772c7..c8f601f 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -101,6 +101,7 @@ static int pack_objects(int fd, struct ref *refs,\n> \t\tfree(buf);\n> \t\tclose(po.out);\n> \t\tpo.out = -1;\n> +\t\tclose(fd);\n> \t}\n> \n> \tif (finish_command(&po))\n\nYeah, from my reading of the code, you are right.\n\n-Peff\n"},{"id":"165840","messageId":"201104142243.33522.j6t@kdbg.org","threadId":"26948","inReplyTo":"20110414202110.GA6525@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-14T20:43:33Z","receivedAt":"2011-04-14T20:43:33Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 14. April 2011, Jeff King wrote:\n> On Thu, Apr 14, 2011 at 09:36:25PM +0200, Johannes Sixt wrote:\n> > On Donnerstag, 14. April 2011, Jeff King wrote:\n> > > Obviously it totally breaks the start_async abstraction if the called\n> > > code needs to care whether it forked or not. But we can use that to our\n> > > advantage, since it means start_async callers must assume the interface\n> > > is very limited. So I think we can do something like:\n> > >\n> > > 1. Async code declares which file descriptors it cares about. This\n> > > would automatically include the pipe we give to it, of course.\n> > > So the declared ones for a sideband demuxer would be stderr, and\n> > > some network fd for reading.\n> > >\n> > > 2. In the pthreads case, we do nothing. In the forked case, the child\n> > > closes every descriptor except the \"interesting\" ones.\n> > >\n> > > And that solves this problem, and the general case that async-callers\n> > > have no idea if they have just leaked pipe descriptors in the forked\n> > > case.\n> >\n> > Sounds like a plan. How do you close all file descriptors? Just iterate\n> > up to getrlimit(RLIMIT_NOFILE)?\n>\n> Sadly, yes, I think that is what we would have to do. It does feel like\n> an awful hack. And it will interact badly with things like valgrind,\n> which open descriptors behind the scenes (but can properly handle\n> the forking).\n>\n> I just don't see another way around it for the general case. The\n> \"usual\" fix for this sort of thing is that the descriptors should have\n> close-on-exec set, but that doesn't work for us here, because we are\n> only forking.\n>\n> It's sufficiently ugly (and still possible to break in the pthreads\n> case!) that it may be worth not worrying about the general case at all,\n> and just fixing this one with the explicit close.\n>\n> > > I'm still slightly confused, though, because I never see that\n> > > descriptor get closed in the threaded case. So I still don't understand\n> > > why it _doesn't_ deadlock with pthreads.\n> >\n> > In the threaded case, this fd is closed by start_command(), where it is\n> > passed as po.out in pack_objects(). In the fork case this is too late\n> > because a duplicate was already inherited to the sideband demuxer.\n>\n> Hrm, I see the code now. That seems like an odd thing to do to me.\n\nWhy so? It's a matter of resource ownership: If you pass a positive value, you \ngive away ownership; if you pass -1, you gain ownership; if you pass 0, \nownership remains unchanged.\n\n> Doesn't it disallow:\n>\n> /* set up a command */\n> const char **argv = { \"some\", \"command\" };\n> struct child_process c;\n> c.argv = argv;\n> c.out = fd;\n>\n> /* run it */\n> run_command(&c);\n>\n> /* now tack our own output to the end */\n> write(fd, \"foo\", 3);\n\nYou would have to dup() the fd before run_command().\n\n> And even weirder, we only do the close for high file descriptors. So you\n> _can_ do that above if \"fd\" is stdout, but not with an arbitrary fd.\n\nAh, right, that's a bit dubious. The reason is that if you want to tell the \nchild process to use the parent's stdout for its own stdout, you specify 0 \naka \"no special treatement\", i.e. just inherit from the parent, not 1. IOW, 1 \nis never a sane candidate to be assigned to c.out.\n\n-- Hannes\n"},{"id":"165841","messageId":"20110414205113.GA7451@sigill.intra.peff.net","threadId":"26948","inReplyTo":"201104142243.33522.j6t@kdbg.org","subject":"Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T20:51:13Z","receivedAt":"2011-04-14T20:51:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2011 at 10:43:33PM +0200, Johannes Sixt wrote:\n\n> > > In the threaded case, this fd is closed by start_command(), where it is\n> > > passed as po.out in pack_objects(). In the fork case this is too late\n> > > because a duplicate was already inherited to the sideband demuxer.\n> >\n> > Hrm, I see the code now. That seems like an odd thing to do to me.\n> \n> Why so? It's a matter of resource ownership: If you pass a positive value, you \n> give away ownership; if you pass -1, you gain ownership; if you pass 0, \n> ownership remains unchanged.\n\nI can see how that is useful. Mostly I was just surprised, because I\nwouldn't expect ownership to be transferred there.\n\n> > Doesn't it disallow:\n> >\n> > /* set up a command */\n> > const char **argv = { \"some\", \"command\" };\n> > struct child_process c;\n> > c.argv = argv;\n> > c.out = fd;\n> >\n> > /* run it */\n> > run_command(&c);\n> >\n> > /* now tack our own output to the end */\n> > write(fd, \"foo\", 3);\n> \n> You would have to dup() the fd before run_command().\n\nTrue. That makes it less of a big deal, because for the times that you\ndon't want full ownership transferred, you can work around it.\n\n> > And even weirder, we only do the close for high file descriptors. So you\n> > _can_ do that above if \"fd\" is stdout, but not with an arbitrary fd.\n> \n> Ah, right, that's a bit dubious. The reason is that if you want to tell the \n> child process to use the parent's stdout for its own stdout, you specify 0 \n> aka \"no special treatement\", i.e. just inherit from the parent, not 1. IOW, 1 \n> is never a sane candidate to be assigned to c.out.\n\nFair enough.\n\nSo what do you want to do about the fd that needs closing? The options\nI see are:\n\n 1. Try for a general solution. That probably means the \"close every\n descriptor in the child\" hackiness that I mentioned earlier.\n\n 2. Fix this case by having the async code close it if it was forked.\n It needs to know whether we forked, so we can:\n\n a. Use NO_PTHREADS. Easy and simple, though it does break\n start_async's abstraction a bit.\n\n b. Have start_async pass in a flag telling what happened. This\n really breaks the abstraction very similarly to (a), but it\n makes the connection more explicit.\n\nI think I am leaning a bit towards (2a). It's simple, and it's not like\nthis is library code with a million unknown callers; fixing it simply\nand cleanly with a nice commit message is probably sufficient.\n\n-Peff\n"},{"id":"165844","messageId":"201104142305.49579.j6t@kdbg.org","threadId":"26948","inReplyTo":"20110414205113.GA7451@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-14T21:05:49Z","receivedAt":"2011-04-14T21:05:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 14. April 2011, Jeff King wrote:\n> So what do you want to do about the fd that needs closing? The options\n> I see are:\n>\n> 1. Try for a general solution. That probably means the \"close every\n> descriptor in the child\" hackiness that I mentioned earlier.\n>\n> 2. Fix this case by having the async code close it if it was forked.\n> It needs to know whether we forked, so we can:\n>\n> a. Use NO_PTHREADS. Easy and simple, though it does break\n> start_async's abstraction a bit.\n>\n> b. Have start_async pass in a flag telling what happened. This\n> really breaks the abstraction very similarly to (a), but it\n> makes the connection more explicit.\n>\n> I think I am leaning a bit towards (2a). It's simple, and it's not like\n> this is library code with a million unknown callers; fixing it simply\n> and cleanly with a nice commit message is probably sufficient.\n\n(2a) would be good enough for my taste, too!\n\n-- Hannes\n"},{"id":"165851","messageId":"7vsjtkfs10.fsf@alter.siamese.dyndns.org","threadId":"26948","inReplyTo":"20110414205113.GA7451@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-14T21:21:31Z","receivedAt":"2011-04-14T21:21:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think I am leaning a bit towards (2a). It's simple, and it's not like\n> this is library code with a million unknown callers; fixing it simply\n> and cleanly with a nice commit message is probably sufficient.\n\nThis really sounds like a plan. Even if we _might_ later want to go to\n2b. or some other solution, we will know what pattern to grep for.\n\nThanks.\n"},{"id":"166282","messageId":"4DB48B2C.2090904@kdbg.org","threadId":"26948","inReplyTo":"7vsjtkfs10.fsf@alter.siamese.dyndns.org","subject":"[PATCH/RFC 1/2] send-pack --stateless-rpc: properly close the outgoing channel","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-24T20:42:20Z","receivedAt":"2011-04-24T20:42:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"In the non-stateless-rpc case, the writable end of the channel to the\nremote repo is closed by the start_command() call that runs the\npack-objects process (after pack-objects inherited a copy). But in the\n--stateless-rpc case, where send-pack takes care of writing data to the\nchannel, this was missed.\n\nSigned-off-by: Johannes Sixt \n---\nAm 14.04.2011 23:21, schrieb Junio C Hamano:\n> Jeff King writes:\n> \n>> I think I am leaning a bit towards (2a). It's simple, and it's not like\n>> this is library code with a million unknown callers; fixing it simply\n>> and cleanly with a nice commit message is probably sufficient.\n> \n> This really sounds like a plan. Even if we _might_ later want to go to\n> 2b. or some other solution, we will know what pattern to grep for.\n\nHere's a 2-patch series that implements this plan. The patches go on top of\n38a81b4e (receive-pack: Wrap status reports inside side-band-64k) just like\nJeff's series (jk/maint-push-async-hang).\n\nWarning: This patch is untested. Furthermore, it does not even fix a resource\nleak because the fd that is now closed in pack_objects() would be closed\nlater in cmd_send_pack. However, without closing the fd earlier like this,\na --stateless-rpc invocation could theoretically dead-lock just like a regular\ninvocation in a NO_PTHREADS build. But I also don't know how to test-drive\nsend-pack --stateless-rpc to construct such a case. Any hints how to do that\nwould be appreciated.\n\n-- Hannes\n\n builtin-send-pack.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 2478e18..089058b 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -97,6 +97,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\tfree(buf);\n \t\tclose(po.out);\n \t\tpo.out = -1;\n+\t\tclose(fd);\n \t}\n \n \tif (finish_command(&po))\n-- \n1.7.5.rc1.97.ge0653\n"},{"id":"166283","messageId":"4DB48CCD.40304@kdbg.org","threadId":"26948","inReplyTo":"4DB48B2C.2090904@kdbg.org","subject":"[PATCH 2/2] send-pack: avoid deadlock when pack-object dies early","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-24T20:49:17Z","receivedAt":"2011-04-24T20:49:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"When pack-objects dies prematurely (for example, because there is some\nrepo corruption), we are careful to clean up the sideband demuxer (if it\nis being used) with finish_async. For an async implementation which forks\n(i.e., not Windows), that means we will waitpid() for the async process.\n\nMeanwhile, the async sideband demuxer will continue trying to stream data\nfrom the remote repo until it gets EOF. Depending on what data\npack-objects actually sent, the remote repo may not actually send\nanything, in particular, after the initial rev-exchange it is waiting for\nthe pack data to arrive.\n\nThe send-pack parent process closes the writable end of the channel so\nthat after the death of the pack-objects process all writable ends should\nhave been closed and the remote repo should see EOF. This does not\nhappen, however, because when the sideband demuxer was forked earlier, it\nalso inherited a writable end; it remains open and keeps the remote repo\nfrom seeing EOF. This leads to a deadlock cycle in which send-pack waits\non the demuxer, the demuxer waits on the remote receive-pack, and the\nremote receive-pack waits on send-pack to send the pack data.\n\nTo break this, we close the writable end in the demuxer, but only when\nit runs in a forked async process.\n\nAnalyzed-by: Jeff King \nSigned-off-by: Johannes Sixt \n---\nThis is the replacement of the series that Jeff proposed earlier. It\nshould also be merged into f6b60983 (Enable threaded async procedures\nwhenever pthreads is available). To keep bisectability, the merge commit\nshould replace '#ifndef ASYNC_AS_THREAD' by '#ifdef NO_PTHREADS'.\n\n-- Hannes\n\n builtin-send-pack.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 089058b..b371c79 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -376,6 +376,9 @@ static void print_helper_status(struct ref *ref)\n static int sideband_demux(int in, int out, void *data)\n {\n \tint *fd = data;\n+#ifndef ASYNC_AS_THREAD\n+\tclose(fd[1]);\n+#endif\n \tint ret = recv_sideband(\"send-pack\", fd[0], out);\n \tclose(out);\n \treturn ret;\n-- \n1.7.5.rc1.97.ge0653\n"},{"id":"166315","messageId":"20110425164038.GA1589@sigill.intra.peff.net","threadId":"26948","inReplyTo":"4DB48B2C.2090904@kdbg.org","subject":"Re: [PATCH/RFC 1/2] send-pack --stateless-rpc: properly close the outgoing channel","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-25T16:40:38Z","receivedAt":"2011-04-25T16:40:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 24, 2011 at 10:42:20PM +0200, Johannes Sixt wrote:\n\n> In the non-stateless-rpc case, the writable end of the channel to the\n> remote repo is closed by the start_command() call that runs the\n> pack-objects process (after pack-objects inherited a copy). But in the\n> --stateless-rpc case, where send-pack takes care of writing data to the\n> channel, this was missed.\n>\n> [...]\n>\n> Warning: This patch is untested. Furthermore, it does not even fix a resource\n> leak because the fd that is now closed in pack_objects() would be closed\n> later in cmd_send_pack.\n\nNote that we can also call send_pack directly from git-push via the\ntransport.c interface. I didn't check whether one can actually trigger\nstateless-rpc this way, though; it looks like git-remote-http ends up\nexec'ing a separate send-pack.\n\n> However, without closing the fd earlier like this, a --stateless-rpc\n> invocation could theoretically dead-lock just like a regular\n> invocation in a NO_PTHREADS build. But I also don't know how to\n> test-drive send-pack --stateless-rpc to construct such a case. Any\n> hints how to do that would be appreciated.\n\nI was able to get a hang using v1.7.5 compiled with pthreads. You need\nto have a server accepting smart-http push (if you use github, you can\ncreate an empty test repo there, which is sufficient).\n\nAnd then do a modified version of the test I posted earlier:\n\n UPSTREAM=https://peff@github.com/peff/test.git\n\n git init child &&\n cd child &&\n git remote add origin $UPSTREAM &&\n echo content >file &&\n git add file &&\n git commit -m one &&\n echo content >>file &&\n git add file &&\n git commit -m two &&\n sha1=`git rev-parse HEAD:file` &&\n file=`echo $sha1 | sed 's,..,&/,'` &&\n rm -fv .git/objects/$file\n\nwhere obviously you need to tweak $UPSTREAM to point to the repo you\ncreated. That sets up the broken repo state. You can then try \"git\npush\" in the repo with various versions to check their behavior.\n\nWith stock v1.7.5, this will hang after pack-objects reports the fatal\nerror. With your patch, it exits immediately, though the output looks\nlike this:\n\n $ git push\n Password:\n Counting objects: 5, done.\n error: unable to find ea0faeb6073ff6cb085727c3647be457051e6ed7\n fatal: unable to read ea0faeb6073ff6cb085727c3647be457051e6ed7\n fatal: The remote end hung up unexpectedly\n fatal: The remote end hung up unexpectedly\n fatal: write error: Bad file descriptor\n\nwhich could perhaps be a little nicer, but is probably not a big deal (I\ndidn't dig very deep, but presumably we should exit a little more\nimmediately after seeing pack-objects fail).\n\n-Peff\n"},{"id":"166316","messageId":"20110425165007.GB1589@sigill.intra.peff.net","threadId":"26948","inReplyTo":"4DB48CCD.40304@kdbg.org","subject":"Re: [PATCH 2/2] send-pack: avoid deadlock when pack-object dies early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-25T16:50:07Z","receivedAt":"2011-04-25T16:50:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 24, 2011 at 10:49:17PM +0200, Johannes Sixt wrote:\n\n> diff --git a/builtin-send-pack.c b/builtin-send-pack.c\n> index 089058b..b371c79 100644\n> --- a/builtin-send-pack.c\n> +++ b/builtin-send-pack.c\n> @@ -376,6 +376,9 @@ static void print_helper_status(struct ref *ref)\n> static int sideband_demux(int in, int out, void *data)\n> {\n> \tint *fd = data;\n> +#ifndef ASYNC_AS_THREAD\n> +\tclose(fd[1]);\n> +#endif\n\nIn the comments for 1/2, you said this goes directly on 38a81b4e. But in\nthat commit, we use #ifndef WIN32 to decide whether or not to fork for\nasync code. So shouldn't this use the same test (I don't even see\nASYNC_AS_THREAD defined anywhere else)?\n\nAnd of course in more modern versions, it should be NO_PTHREADS, as you\nnoted.\n\n-Peff\n"},{"id":"166318","messageId":"4DB5B261.1080804@kdbg.org","threadId":"26948","inReplyTo":"20110425165007.GB1589@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] send-pack: avoid deadlock when pack-object dies early","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-25T17:41:53Z","receivedAt":"2011-04-25T17:41:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.04.2011 18:50, schrieb Jeff King:\n> On Sun, Apr 24, 2011 at 10:49:17PM +0200, Johannes Sixt wrote:\n> \n>> diff --git a/builtin-send-pack.c b/builtin-send-pack.c\n>> index 089058b..b371c79 100644\n>> --- a/builtin-send-pack.c\n>> +++ b/builtin-send-pack.c\n>> @@ -376,6 +376,9 @@ static void print_helper_status(struct ref *ref)\n>> static int sideband_demux(int in, int out, void *data)\n>> {\n>> \tint *fd = data;\n>> +#ifndef ASYNC_AS_THREAD\n>> +\tclose(fd[1]);\n>> +#endif\n> \n> In the comments for 1/2, you said this goes directly on 38a81b4e. But in\n> that commit, we use #ifndef WIN32 to decide whether or not to fork for\n> async code. So shouldn't this use the same test (I don't even see\n> ASYNC_AS_THREAD defined anywhere else)?\n\nOops, you are right. I was looking at f6b60983, the one that the\ntwo-patch series should be merged to; there, we remove ASYNC_AS_THREAD\nand replace it by NO_PTHREADS. Therefore, I assumed that we use the\nformer symbol to decide whether to use threaded async procedures.\nObviously, the symbol was introduced only later. Will resend.\n\n-- Hannes\n"},{"id":"166319","messageId":"7vwriijk25.fsf@alter.siamese.dyndns.org","threadId":"26948","inReplyTo":"4DB5B261.1080804@kdbg.org","subject":"Re: [PATCH 2/2] send-pack: avoid deadlock when pack-object dies early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-25T17:51:46Z","receivedAt":"2011-04-25T17:51:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> ... Therefore, I assumed that we use the\n> former symbol to decide whether to use threaded async procedures.\n> Obviously, the symbol was introduced only later. Will resend.\n\nThanks.\n"},{"id":"166330","messageId":"4DB5E1CA.8000105@kdbg.org","threadId":"26948","inReplyTo":"20110425165007.GB1589@sigill.intra.peff.net","subject":"[PATCH v2] send-pack: avoid deadlock when pack-object dies early","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-04-25T21:04:10Z","receivedAt":"2011-04-25T21:04:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Send-pack deadlocks in two ways when pack-object dies early (for example,\nbecause there is some repo corruption).\n\nThe first deadlock happens with the smart push protocol (--stateless-rpc).\nAfter the initial rev-exchange, the remote is waiting for the pack data\nto arrive, and the sideband demuxer at the local side continues trying to\nstream data from the remote repository until it gets EOF. Meanwhile,\nsend-pack (in function pack_objects()) has noticed that pack-objects did\nnot produce output and died. Back in send_pack(), it now tries to clean\nup the sideband demuxer using finish_async(). The demuxer, however, waits\nfor the remote end to close down, the remote waits for pack data, and\nthe reason that it still waits is that send-pack forgot to close the\noutgoing channel. Add the missing close() in pack_objects().\n\nThe second deadlock happens in a similar constellation when the sideband\ndemuxer runs in a forked process (rather than in a thread). Again, the\nremote end waits for pack data to arrive, the sideband demuxer waits for\nthe remote to shut down, and send-pack (in the regular clean-up) waits for\nthe demuxer to terminate. This time, the send-pack parent process closes\nthe writable end of the outgoing channel (in start_command() that spawned\npack-objects) so that after the death of the pack-objects process all\nwritable ends should have been closed and the remote repo should see EOF.\nThis does not happen, however, because when the sideband demuxer was forked\nearlier, it also inherited a writable end; it remains open and keeps the\nremote repo from seeing EOF. To break this deadlock, close the writable end\nin the demuxer.\n\nAnalyzed-by: Jeff King \nSigned-off-by: Johannes Sixt \n---\nAm 25.04.2011 18:50, schrieb Jeff King:\n> In the comments for 1/2, you said this goes directly on 38a81b4e. But in\n> that commit, we use #ifndef WIN32 to decide whether or not to fork for\n> async code. So shouldn't this use the same test (I don't even see\n> ASYNC_AS_THREAD defined anywhere else)?\n\nHere's the fixed patch. I squashed both earlier patches into a single patch\nbecause they are about the same topic, as you showed with your tests of\ngit-push via smart http.\n\nAgain, this should go on top of 38a81b4e. When it is merged to f6b60983 or\nlater, the '#ifndef WIN32' must be changed to '#ifdef NO_PTHREADS'.\n\n-- Hannes\n\n builtin-send-pack.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 2478e18..6516288 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -97,6 +97,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\tfree(buf);\n \t\tclose(po.out);\n \t\tpo.out = -1;\n+\t\tclose(fd);\n \t}\n \n \tif (finish_command(&po))\n@@ -375,6 +376,9 @@ static void print_helper_status(struct ref *ref)\n static int sideband_demux(int in, int out, void *data)\n {\n \tint *fd = data;\n+#ifndef WIN32\n+\tclose(fd[1]);\n+#endif\n \tint ret = recv_sideband(\"send-pack\", fd[0], out);\n \tclose(out);\n \treturn ret;\n-- \n1.7.5.rc1.97.ge0653\n"},{"id":"166362","messageId":"20110426082302.GC26874@sigill.intra.peff.net","threadId":"26948","inReplyTo":"4DB5E1CA.8000105@kdbg.org","subject":"Re: [PATCH v2] send-pack: avoid deadlock when pack-object dies early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-26T08:23:02Z","receivedAt":"2011-04-26T08:23:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 25, 2011 at 11:04:10PM +0200, Johannes Sixt wrote:\n\n> > In the comments for 1/2, you said this goes directly on 38a81b4e. But in\n> > that commit, we use #ifndef WIN32 to decide whether or not to fork for\n> > async code. So shouldn't this use the same test (I don't even see\n> > ASYNC_AS_THREAD defined anywhere else)?\n> \n> Here's the fixed patch. I squashed both earlier patches into a single patch\n> because they are about the same topic, as you showed with your tests of\n> git-push via smart http.\n\nThanks, this one looks good to me, and I think the squash is sensible.\n\n> Again, this should go on top of 38a81b4e. When it is merged to f6b60983 or\n> later, the '#ifndef WIN32' must be changed to '#ifdef NO_PTHREADS'.\n\nI see Junio has an evil merge in pu that takes care of this.\n\nI wonder if it might have been nicer to \"cherry-pick -n\" the old fix on\ntop of master, fixing up the tree to use NO_PTHREADS, and then resolving\nthe ensuing merge conflict in favor of the cherry-picked version. . I\nguess it is just a matter of style (the fact that a few of our\narchaeology tools do not find content in evil merges very well is a\ndownside, but that just means we should fix the tools :) ).\n\n-Peff\n"}]}