{"thread":{"id":"22931","subject":"Re: [PATCH 0/6] Pass t5530 on Windows","startedAt":"2010-03-06T15:40:37Z","lastAt":"2010-03-23T21:42:46Z","messageCount":16,"participants":["Shawn O. Pearce","Junio C Hamano","Johannes Sixt","Fredrik Kuivinen"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"136325","messageId":"cover.1267889072.git.j6t@kdbg.org","threadId":"22931","inReplyTo":null,"subject":"[PATCH 0/6] Pass t5530 on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-06T15:40:37Z","receivedAt":"2010-03-06T15:40:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The goal of this series is to fix async procedures to the extent that\nt5530-upload-pack-error.sh passes on Windows.\n\nThe problem is that on Windows async procedures are run in threads (due\nto the lack of fork()), and a die() call from an async procedure tears\ndown the whole process while on POSIX only the async procedure is\nterminated.\n\nThe approach taken is to use a custom die routine that exits only the\nthread when it detects that it is not called from the main procedure.\n\nAs a bonus, the threaded async infrastructure now uses pthreads instead\nof Windows specific API, and can be enabled on POSIX as well through a\nnew compile-time option.\n\nQuite frankly, I don't quite know what to do with this series. On the\none hand, it is a clean-up, but in practice it is not relevant whether\ndie() kills only the async thread or the whole process because all\ncallers of async die() themselves anyway when the async procedure died.\nOn the other hand, it does enable threaded async procedures on POSIX...\n\nJohannes Sixt (6):\n  Modernize t5530-upload-pack-error.\n  Make report() from usage.c public as vreportf() and use it.\n  Fix signature of fcntl() compatibility dummy\n  Windows: more pthreads functions\n  Reimplement async procedures using pthreads\n  Dying in an async procedure should only exit the thread, not the\n    process.\n\n Makefile                     |    5 +++\n compat/mingw.h               |    2 +-\n compat/win32/pthread.c       |    8 ++++\n compat/win32/pthread.h       |   25 ++++++++++++++\n fast-import.c                |    8 ++---\n git-compat-util.h            |    1 +\n http-backend.c               |    5 +--\n run-command.c                |   75 ++++++++++++++++++++++++++++++++----------\n run-command.h                |    8 +++-\n t/t5530-upload-pack-error.sh |   18 +++++-----\n usage.c                      |   10 +++---\n 11 files changed, 121 insertions(+), 44 deletions(-)\n"},{"id":"136316","messageId":"7vk4tpdx9x.fsf@alter.siamese.dyndns.org","threadId":"22931","inReplyTo":"cover.1267889072.git.j6t@kdbg.org","subject":"Re: [PATCH 0/6] Pass t5530 on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-06T20:12:58Z","receivedAt":"2010-03-06T20:12:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Quite frankly, I don't quite know what to do with this series. On the\n> one hand, it is a clean-up, but in practice it is not relevant whether\n> die() kills only the async thread or the whole process because all\n> callers of async die() themselves anyway when the async procedure died.\n> On the other hand, it does enable threaded async procedures on POSIX...\n\nYou wrote in [PATCH 5/6]:\n\n    A new configuration option is introduced so that the threaded\n    implementation can also be used on POSIX systems. Since this option is\n    intended only as playground on POSIX, but is mandatory on Windows, the\n    option is not documented.\n\nbut I am wondering how much of the real world is threads-challenged these\ndays.  Here is what you have at the end of technical/api-run-command.txt:\n\n   There are serious restrictions on what the asynchronous function can do\n   because this facility is implemented by a pipe to a forked process on\n   UNIX, but by a thread in the same address space on Windows:\n\n   . It cannot change the program's state (global variables, environment,\n     etc.) in a way that the caller notices; in other words, .in and .out\n     are the only communication channels to the caller.\n\n   . It must not change the program's state that the caller of the\n     facility also uses.\n\nAnd calling die() from async is obviously \"change the program's state that\nthe caller of the facility also uses\".  We didn't uncover this as a bug\nbecause the above \"serious restrictions\" go both ways.\n\nIf we make threaded-async the default on any platform that is thread\ncapable, we would increase the likelihood of catching bugs that violate\nthe latter condition.  I am sure there may be downsides for going that\nroute, but it might be better than the current situation where two major\nplatforms use quite different underlying semantics for the same call, and\nrely on the program (both caller and callee) to honor the above\nconditions, without much tool support [*1*].\n\n[Footnote]\n\n*1* I sometimes wonder if people who are interseted in static analysis can\nhelp with issues like this: \"In a function started by start_async() and\nits callees, you are not supposed to touch these globals, nor call those\nfunctions\".  That would be more useful than reports we occasionally get\n\"in this codepath this variable can be used before assigned\" with many\nfalse positives, that presumably come from from the canned set of rules\nthese static checkers may have.\n"},{"id":"136245","messageId":"20100306215051.GE2529@spearce.org","threadId":"22931","inReplyTo":"7vk4tpdx9x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/6] Pass t5530 on Windows","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-03-06T21:50:51Z","receivedAt":"2010-03-06T21:50:51Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n> > Quite frankly, I don't quite know what to do with this series. On the\n> > one hand, it is a clean-up, but in practice it is not relevant whether\n> > die() kills only the async thread or the whole process because all\n> > callers of async die() themselves anyway when the async procedure died.\n> > On the other hand, it does enable threaded async procedures on POSIX...\n...\n>    . It must not change the program's state that the caller of the\n>      facility also uses.\n> \n> And calling die() from async is obviously \"change the program's state that\n> the caller of the facility also uses\".  We didn't uncover this as a bug\n> because the above \"serious restrictions\" go both ways.\n\nI agree with you Junio.  I think that any async helper that is\ninvoking die() in its code path is wrong.  Just like its also\nwrong for an async helper to try and use the sha1_file.c family\nof functions.  The helper should return failure and let its caller\nhandle the process termination.\n\nHell, its even wrong for an async helper to use xmalloc(), because in\na low-memory situation that xmalloc may try to remove pack windows\n*without locking*.  _DOUBLE_PLUS_UNGOOD_\n \n> If we make threaded-async the default on any platform that is thread\n> capable, we would increase the likelihood of catching bugs that violate\n> the latter condition.\n\nI'm in favor of that.  If we have threaded delta search enabled,\nwe probably can also run these async procedures in a POSIX thread\nrather than forking off a child.  Though on our primary target\nplatform of Linux, the performance difference either way probably\ncannot be measured.\n\n-- \nShawn.\n"},{"id":"136468","messageId":"201003092100.36616.j6t@kdbg.org","threadId":"22931","inReplyTo":"20100306215051.GE2529@spearce.org","subject":"[PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-09T20:00:36Z","receivedAt":"2010-03-09T20:00:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n---\nOn Samstag, 6. März 2010, Shawn O. Pearce wrote:\n> I'm in favor of that.  If we have threaded delta search enabled,\n> we probably can also run these async procedures in a POSIX thread\n> rather than forking off a child.\n\nOK. The patch could look like this.\n\n-- Hannes\n\n Documentation/technical/api-run-command.txt |    5 +++--\n Makefile                                    |    5 -----\n run-command.c                               |    6 +++---\n run-command.h                               |    4 ++--\n 4 files changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt\nindex 44876fa..f18b4f4 100644\n--- a/Documentation/technical/api-run-command.txt\n+++ b/Documentation/technical/api-run-command.txt\n@@ -231,8 +231,9 @@ The function pointer in .proc has the following signature:\n \n \n There are serious restrictions on what the asynchronous function can do\n-because this facility is implemented by a pipe to a forked process on\n-UNIX, but by a thread in the same address space on Windows:\n+because this facility is implemented by a thread in the same address\n+space on most platforms (when pthreads is available), but by a pipe to\n+a forked process otherwise:\n \n . It cannot change the program's state (global variables, environment,\n   etc.) in a way that the caller notices; in other words, .in and .out\ndiff --git a/Makefile b/Makefile\nindex 2fe52f8..52f2cc0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -979,7 +979,6 @@ ifeq ($(uname_S),Windows)\n \tNO_CURL = YesPlease\n \tNO_PYTHON = YesPlease\n \tBLK_SHA1 = YesPlease\n-\tASYNC_AS_THREAD = YesPlease\n \n \tCC = compat/vcbuild/scripts/clink.pl\n \tAR = compat/vcbuild/scripts/lib.pl\n@@ -1031,7 +1030,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tNO_REGEX = YesPlease\n \tNO_PYTHON = YesPlease\n \tBLK_SHA1 = YesPlease\n-\tASYNC_AS_THREAD = YesPlease\n \tCOMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch -Icompat/win32\n \tCOMPAT_CFLAGS += -DSTRIP_EXTENSION=\\\".exe\\\"\n \tCOMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \\\n@@ -1344,9 +1342,6 @@ ifdef NO_PTHREADS\n else\n \tEXTLIBS += $(PTHREAD_LIBS)\n \tLIB_OBJS += thread-utils.o\n-ifdef ASYNC_AS_THREAD\n-\tBASIC_CFLAGS += -DASYNC_AS_THREAD\n-endif\n endif\n \n ifdef DIR_HAS_BSD_GROUP_SEMANTICS\ndiff --git a/run-command.c b/run-command.c\nindex 66cc4bf..053b28f 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -447,7 +447,7 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const\n \treturn run_command(&cmd);\n }\n \n-#ifdef ASYNC_AS_THREAD\n+#ifndef NO_PTHREADS\n static pthread_t main_thread;\n static int main_thread_set;\n static pthread_key_t async_key;\n@@ -521,7 +521,7 @@ int start_async(struct async *async)\n \telse\n \t\tproc_out = -1;\n \n-#ifndef ASYNC_AS_THREAD\n+#ifdef NO_PTHREADS\n \t/* Flush stdio before fork() to avoid cloning buffers */\n \tfflush(NULL);\n \n@@ -590,7 +590,7 @@ error:\n \n int finish_async(struct async *async)\n {\n-#ifndef ASYNC_AS_THREAD\n+#ifdef NO_PTHREADS\n \treturn wait_or_whine(async->pid, \"child process\", 0);\n #else\n \tvoid *ret = (void *)(intptr_t)(-1);\ndiff --git a/run-command.h b/run-command.h\nindex 40db39c..56491b9 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -1,7 +1,7 @@\n #ifndef RUN_COMMAND_H\n #define RUN_COMMAND_H\n \n-#ifdef ASYNC_AS_THREAD\n+#ifndef NO_PTHREADS\n #include <pthread.h>\n #endif\n \n@@ -78,7 +78,7 @@ struct async {\n \tvoid *data;\n \tint in;\t\t/* caller writes here and closes it */\n \tint out;\t/* caller reads from here and closes it */\n-#ifndef ASYNC_AS_THREAD\n+#ifdef NO_PTHREADS\n \tpid_t pid;\n #else\n \tpthread_t tid;\n-- \n1.7.0.rc2.65.g7b13a\n"},{"id":"136479","messageId":"20100309234317.GA12958@spearce.org","threadId":"22931","inReplyTo":"201003092100.36616.j6t@kdbg.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-03-09T23:43:17Z","receivedAt":"2010-03-09T23:43:17Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> wrote:\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n> On Samstag, 6. M?rz 2010, Shawn O. Pearce wrote:\n> > I'm in favor of that.  If we have threaded delta search enabled,\n> > we probably can also run these async procedures in a POSIX thread\n> > rather than forking off a child.\n> \n> OK. The patch could look like this.\n\nLooks fine to me.  But given my earlier statement about xmalloc()\ndamage... I wonder if we shouldn't try to ensure that isn't a\nproblem first.\n\n-- \nShawn.\n"},{"id":"136549","messageId":"7v7hpjq0aw.fsf@alter.siamese.dyndns.org","threadId":"22931","inReplyTo":"201003092100.36616.j6t@kdbg.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-10T22:28:07Z","receivedAt":"2010-03-10T22:28:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n> On Samstag, 6. März 2010, Shawn O. Pearce wrote:\n>> I'm in favor of that.  If we have threaded delta search enabled,\n>> we probably can also run these async procedures in a POSIX thread\n>> rather than forking off a child.\n>\n> OK. The patch could look like this.\n\nWill queue in 'pu', but as Shawn said, we should probably give another\ncloser look at the callees that are started with this interface before\nmoving forward.\n"},{"id":"136615","messageId":"201003112053.07260.j6t@kdbg.org","threadId":"22931","inReplyTo":"7v7hpjq0aw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-11T19:53:07Z","receivedAt":"2010-03-11T19:53:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Mittwoch, 10. März 2010, Junio C Hamano wrote:\n> Will queue in 'pu', but as Shawn said, we should probably give another\n> closer look at the callees that are started with this interface before\n> moving forward.\n\nI'll audit the call paths. But I'll need some time for this.\n\n-- Hannes\n"},{"id":"136642","messageId":"7v8w9yt74z.fsf@alter.siamese.dyndns.org","threadId":"22931","inReplyTo":"201003112053.07260.j6t@kdbg.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-12T05:56:44Z","receivedAt":"2010-03-12T05:56:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> On Mittwoch, 10. März 2010, Junio C Hamano wrote:\n>> Will queue in 'pu', but as Shawn said, we should probably give another\n>> closer look at the callees that are started with this interface before\n>> moving forward.\n>\n> I'll audit the call paths. But I'll need some time for this.\n\nThanks for volunteering.\n"},{"id":"137060","messageId":"201003172228.18939.j6t@kdbg.org","threadId":"22931","inReplyTo":"7v7hpjq0aw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-17T21:28:18Z","receivedAt":"2010-03-17T21:28:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Mittwoch, 10. März 2010, Junio C Hamano wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> > On Samstag, 6. März 2010, Shawn O. Pearce wrote:\n> >> I'm in favor of that.  If we have threaded delta search enabled,\n> >> we probably can also run these async procedures in a POSIX thread\n> >> rather than forking off a child.\n> >\n> > OK. The patch could look like this.\n>\n> Will queue in 'pu', but as Shawn said, we should probably give another\n> closer look at the callees that are started with this interface before\n> moving forward.\n\nSo here is my analysis.\n\nThere are currently these users of the async infrastructure:\n\nbuiltin-fetch-pack.c\nbuiltin-receive-pack.c\nbuiltin-send-pack.c\nconvert.c\nremote-curl.c\nupload_pack.c\n\nThe list below shows all functions that read or write potentially global state \nthat are called from the async procedure (except for upload_pack.c, see \nbelow). I ignored all functions that do not depend on global state, like \nstrcmp, memcpy, sprintf, strerror, etc.\n\n----------\nremote-curl.c:write_discovery():\n  write\n  close\n\nThese two functions only communicate with the parent. This is absolutely \nthread-safe.\n\n----------\nbuiltin-fetch-pack.c:sideband_demux() and\nbuiltin-send-pack.c:sideband_demux() are virtually identical, and\nbuiltin-receive-pack.c:copy_to_sideband() has the same footprint:\n  getenv\n  read\n  die_errno\n  die\n  write\n  close\n\nAgain, the functions only communicate with the parent and nothing else. getenv \ncould be a problem if the parent called setenv, but it doesn't. die (and \ndie_errno) are overridden in the threaded version, and are uncritical. In \ntotal, this user is thread-safe.\n\n(This is getenv(\"TERM\") from recv_sideband, btw.)\n\n----------\nconvert.c:filter_buffer()\n  pipe\n  close\n  getenv\n  fork\n  read\n  write\n  fprintf\n  waitpid\n\nif GIT_TRACE is set:\n  xrealloc\n  die\n  free\n\nThis one is less trivial. It calls start_command+finish_command from the async \nprocedure. The parent calls\n  read()\n  xrealloc() (via strbuf_read())\n  error()\n(and nothing else) between start_async() and finish_async().\n\nAs long as GIT_TRACE is not set, we are safe, because the async procedure \ncarefully releases all global resources that it allocated, and the parent is \nlooking in the other direction while it does os. Even if GIT_TRACE is set, \nxrealloc() in the parent and the async procedure cannot be called \nsimultanously because the procedure calls it only before it begins writing \noutput, and the parent only after it received this output.\n\nOn Windows, the situation is slightly different, because we always call into \nxmalloc() during start_command, but again we do so before the output is \nproduced.\n\nI think we are safe in all cases.\n\n----------\nupload_pack:create_pack_file(): This user is different because it calls into \nthe revision walker in the async procedure, which definitely affects global \nstate. Therefore, here is the list of functions called by the parent until it \nexits:\n  pipe\n  close\n  getenv\n  fork\n  read\n  write\n  fprintf\n  waitpid\n  alarm\n  die\n  die_errno\n  pthread_join / waitpid\n\nif GIT_TRACE is set:\n  xrealloc\n  die\n  free\n\nIt also calls start_command+finish_command. If GIT_TRACE is set, we are less \nsafe because this time, xrealloc() can be called simultanously with pack \nfunctions while the revision walker is operating.\n\nBut as long as GIT_TRACE is not set, it looks like we are safe, though I \nhaven't looked through the revision walker whether it messes with alarms or \nthe environment.\n\nOne aspect is, though, that the async procedure is used *only* for a shallow \nclone, and the reason that it is used is that pack-objects (or the way it is \ncalled from upload-pack?) has not been taught to produce a pack for a shallow \nclone.\n\nThis concludes my analysis.\n\n-- Hannes\n"},{"id":"137062","messageId":"7vvdcuzj4c.fsf@alter.siamese.dyndns.org","threadId":"22931","inReplyTo":"201003172228.18939.j6t@kdbg.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-17T22:19:31Z","receivedAt":"2010-03-17T22:19:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>> Will queue in 'pu', but as Shawn said, we should probably give another\n>> closer look at the callees that are started with this interface before\n>> moving forward.\n>\n> So here is my analysis.\n> ...\n> This concludes my analysis.\n\nThanks; very much appreciated.\n\nI'll read it over and start moving things to 'next', then.  I'll be slower\nthan my usual self for the coming few days, though.\n\nA bit of patience please.\n"},{"id":"137615","messageId":"4c8ef71003230115y64d36094y178fcfe6576e9c66@mail.gmail.com","threadId":"22931","inReplyTo":"201003172228.18939.j6t@kdbg.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-03-23T08:15:45Z","receivedAt":"2010-03-23T08:15:45Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Wed, Mar 17, 2010 at 22:28, Johannes Sixt <j6t@kdbg.org> wrote:\n> On Mittwoch, 10. März 2010, Junio C Hamano wrote:\n>> Johannes Sixt <j6t@kdbg.org> writes:\n>> > On Samstag, 6. März 2010, Shawn O. Pearce wrote:\n>> >> I'm in favor of that.  If we have threaded delta search enabled,\n>> >> we probably can also run these async procedures in a POSIX thread\n>> >> rather than forking off a child.\n>> >\n>> > OK. The patch could look like this.\n>>\n>> Will queue in 'pu', but as Shawn said, we should probably give another\n>> closer look at the callees that are started with this interface before\n>> moving forward.\n>\n> So here is my analysis.\n>\n> There are currently these users of the async infrastructure:\n>\n> builtin-fetch-pack.c\n> builtin-receive-pack.c\n> builtin-send-pack.c\n> convert.c\n> remote-curl.c\n> upload_pack.c\n>\n> The list below shows all functions that read or write potentially global state\n> that are called from the async procedure (except for upload_pack.c, see\n> below). I ignored all functions that do not depend on global state, like\n> strcmp, memcpy, sprintf, strerror, etc.\n>\n\n> ----------\n> convert.c:filter_buffer()\n>  pipe\n>  close\n>  getenv\n>  fork\n>  read\n>  write\n>  fprintf\n>  waitpid\n>\n> if GIT_TRACE is set:\n>  xrealloc\n>  die\n>  free\n>\n> This one is less trivial. It calls start_command+finish_command from the async\n> procedure. The parent calls\n>  read()\n>  xrealloc() (via strbuf_read())\n>  error()\n> (and nothing else) between start_async() and finish_async().\n>\n> As long as GIT_TRACE is not set, we are safe, because the async procedure\n> carefully releases all global resources that it allocated, and the parent is\n> looking in the other direction while it does os. Even if GIT_TRACE is set,\n> xrealloc() in the parent and the async procedure cannot be called\n> simultanously because the procedure calls it only before it begins writing\n> output, and the parent only after it received this output.\n\nMaybe I'm missing something but, isn't it possible that xrealloc is\ncalled simultaneously from the two threads if GIT_TRACE is set?\n\nImmediately after start_async the parent calls strbuf_read. We then\nget the call chain\nstrbuf_read -> strbuf_grow -> ALLOG_GROW -> xrealloc, so xrealloc is\ncalled before we read any data in the parent.\n\nIn the child we have start_command -> trace_argv_printf -> strbuf_grow -> ...\n\nThat xmalloc and xrealloc aren't thread-safe feels a bit fragile.\nMaybe we should try to fix that.\n\n> ----------\n> upload_pack:create_pack_file(): This user is different because it calls into\n> the revision walker in the async procedure, which definitely affects global\n> state. Therefore, here is the list of functions called by the parent until it\n> exits:\n>  pipe\n>  close\n>  getenv\n>  fork\n>  read\n>  write\n>  fprintf\n>  waitpid\n>  alarm\n>  die\n>  die_errno\n>  pthread_join / waitpid\n\nsha1_to_hex is also called by the parent and the current\nimplementation of that function is not thread-safe. sha1_to_hex is\nalso called by some paths in the revision machinery, but I don't know\nif it will ever be called in this particular case.\n\nMaybe it would be a good idea to create wrappers for getenv, setenv,\nunsetenv, and putenv to make them thread-safe as well. Then won't have\nto worry about them in the future.\n\n- Fredrik\n"},{"id":"137665","messageId":"201003232119.19430.j6t@kdbg.org","threadId":"22931","inReplyTo":"4c8ef71003230115y64d36094y178fcfe6576e9c66@mail.gmail.com","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-23T20:19:18Z","receivedAt":"2010-03-23T20:19:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Dienstag, 23. März 2010, Fredrik Kuivinen wrote:\n> On Wed, Mar 17, 2010 at 22:28, Johannes Sixt <j6t@kdbg.org> wrote:\n> > ----------\n> > convert.c:filter_buffer()\n> ...\n> Maybe I'm missing something but, isn't it possible that xrealloc is\n> called simultaneously from the two threads if GIT_TRACE is set?\n>\n> Immediately after start_async the parent calls strbuf_read. We then\n> get the call chain\n> strbuf_read -> strbuf_grow -> ALLOG_GROW -> xrealloc, so xrealloc is\n> called before we read any data in the parent.\n>\n> In the child we have start_command -> trace_argv_printf -> strbuf_grow ->\n> ...\n\nOutch! You are right. It seems I missed the call of strbuf_grow before the \nloop in strbuf_read.\n\nOK, this means that convert.c is not safe if (and only if) GIT_TRACE is \nset. :-(\n\n> That xmalloc and xrealloc aren't thread-safe feels a bit fragile.\n> Maybe we should try to fix that.\n\nThe point of this assessment was to find out whether this is necessary (and \nwhether something else that is not thread-safe is used).\n\n> > ----------\n> > upload_pack:create_pack_file(): \n> ...\n> sha1_to_hex is also called by the parent and the current\n> implementation of that function is not thread-safe. sha1_to_hex is\n> also called by some paths in the revision machinery, but I don't know\n> if it will ever be called in this particular case.\n\nsha1_to_hex is only called by the parent when the async procedure is not used.\n\n-- Hannes\n"},{"id":"137667","messageId":"201003232125.49933.j6t@kdbg.org","threadId":"22931","inReplyTo":"201003232119.19430.j6t@kdbg.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-23T20:25:49Z","receivedAt":"2010-03-23T20:25:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Dienstag, 23. März 2010, Johannes Sixt wrote:\n> On Dienstag, 23. März 2010, Fredrik Kuivinen wrote:\n> > On Wed, Mar 17, 2010 at 22:28, Johannes Sixt <j6t@kdbg.org> wrote:\n> > > ----------\n> > > upload_pack:create_pack_file():\n> >\n> > ...\n> > sha1_to_hex is also called by the parent and the current\n> > implementation of that function is not thread-safe. sha1_to_hex is\n> > also called by some paths in the revision machinery, but I don't know\n> > if it will ever be called in this particular case.\n>\n> sha1_to_hex is only called by the parent when the async procedure is not\n> used.\n\nBTW, the real fix for this potentially problematic case is to teach \npack-objects to create a pack file for a shallow clone. Then we don't need \nthis instance of an async procedure.\n\n-- Hannes\n"},{"id":"137668","messageId":"7v7hp2rcn8.fsf@alter.siamese.dyndns.org","threadId":"22931","inReplyTo":"201003232125.49933.j6t@kdbg.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-23T20:44:43Z","receivedAt":"2010-03-23T20:44:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> BTW, the real fix for this potentially problematic case is to teach \n> pack-objects to create a pack file for a shallow clone. Then we don't need \n> this instance of an async procedure.\n\nI keep hearing \"shallow clone\" here, but doesn't \"bundle -n 1\" essentially\ndo something quite similar using pack-objects?  Do they make a call into\npack-objects in a different way, and if so can we update \"clone\" to mimic\nhow \"bundle\" drives pack-objects?\n"},{"id":"137669","messageId":"201003232209.35750.j6t@kdbg.org","threadId":"22931","inReplyTo":"7v7hp2rcn8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-23T21:09:35Z","receivedAt":"2010-03-23T21:09:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Dienstag, 23. März 2010, Junio C Hamano wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> > BTW, the real fix for this potentially problematic case is to teach\n> > pack-objects to create a pack file for a shallow clone. Then we don't\n> > need this instance of an async procedure.\n>\n> I keep hearing \"shallow clone\" here, but doesn't \"bundle -n 1\" essentially\n> do something quite similar using pack-objects?  Do they make a call into\n> pack-objects in a different way, and if so can we update \"clone\" to mimic\n> how \"bundle\" drives pack-objects?\n\nBoth *current* implementations of shallow clone and bundle feed pack-objects \nwith rev-list output.\n\nIn the case of non-shallow clone, upload-pack does not need rev-list because \npack-objects can walk the revisions itself. I cannot tell what pack-objects \ndoes not have that it cannot walk the revisions itself for shallow clones.\n\n-- Hannes\n"},{"id":"137671","messageId":"4c8ef71003231442j30618489n226dd16d4033c3fb@mail.gmail.com","threadId":"22931","inReplyTo":"201003232119.19430.j6t@kdbg.org","subject":"Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-03-23T21:42:46Z","receivedAt":"2010-03-23T21:42:46Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Tue, Mar 23, 2010 at 21:19, Johannes Sixt <j6t@kdbg.org> wrote:\n> On Dienstag, 23. März 2010, Fredrik Kuivinen wrote:\n>> On Wed, Mar 17, 2010 at 22:28, Johannes Sixt <j6t@kdbg.org> wrote:\n\n>> That xmalloc and xrealloc aren't thread-safe feels a bit fragile.\n>> Maybe we should try to fix that.\n>\n> The point of this assessment was to find out whether this is necessary (and\n> whether something else that is not thread-safe is used).\n\nIt may not be necessary now, but my point was that by having\nthread-unsafe xmalloc and xrealloc it is a bit too easy to introduce\nnew bugs.\n\n>> > ----------\n>> > upload_pack:create_pack_file():\n>> ...\n>> sha1_to_hex is also called by the parent and the current\n>> implementation of that function is not thread-safe. sha1_to_hex is\n>> also called by some paths in the revision machinery, but I don't know\n>> if it will ever be called in this particular case.\n>\n> sha1_to_hex is only called by the parent when the async procedure is not used.\n\nYes, you are right.\n\n- Fredrik\n"}]}