* [PATCH 0/6] Pass t5530 on Windows @ 2010-03-06 15:40 Johannes Sixt 2010-03-06 20:12 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Johannes Sixt @ 2010-03-06 15:40 UTC (permalink / raw) To: git; +Cc: Johannes Sixt The goal of this series is to fix async procedures to the extent that t5530-upload-pack-error.sh passes on Windows. The problem is that on Windows async procedures are run in threads (due to the lack of fork()), and a die() call from an async procedure tears down the whole process while on POSIX only the async procedure is terminated. The approach taken is to use a custom die routine that exits only the thread when it detects that it is not called from the main procedure. As a bonus, the threaded async infrastructure now uses pthreads instead of Windows specific API, and can be enabled on POSIX as well through a new compile-time option. Quite frankly, I don't quite know what to do with this series. On the one hand, it is a clean-up, but in practice it is not relevant whether die() kills only the async thread or the whole process because all callers of async die() themselves anyway when the async procedure died. On the other hand, it does enable threaded async procedures on POSIX... Johannes Sixt (6): Modernize t5530-upload-pack-error. Make report() from usage.c public as vreportf() and use it. Fix signature of fcntl() compatibility dummy Windows: more pthreads functions Reimplement async procedures using pthreads Dying in an async procedure should only exit the thread, not the process. Makefile | 5 +++ compat/mingw.h | 2 +- compat/win32/pthread.c | 8 ++++ compat/win32/pthread.h | 25 ++++++++++++++ fast-import.c | 8 ++--- git-compat-util.h | 1 + http-backend.c | 5 +-- run-command.c | 75 ++++++++++++++++++++++++++++++++---------- run-command.h | 8 +++- t/t5530-upload-pack-error.sh | 18 +++++----- usage.c | 10 +++--- 11 files changed, 121 insertions(+), 44 deletions(-) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/6] Pass t5530 on Windows 2010-03-06 15:40 [PATCH 0/6] Pass t5530 on Windows Johannes Sixt @ 2010-03-06 20:12 ` Junio C Hamano 2010-03-06 21:50 ` Shawn O. Pearce 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2010-03-06 20:12 UTC (permalink / raw) To: Johannes Sixt; +Cc: git Johannes Sixt <j6t@kdbg.org> writes: > Quite frankly, I don't quite know what to do with this series. On the > one hand, it is a clean-up, but in practice it is not relevant whether > die() kills only the async thread or the whole process because all > callers of async die() themselves anyway when the async procedure died. > On the other hand, it does enable threaded async procedures on POSIX... You wrote in [PATCH 5/6]: A new configuration option is introduced so that the threaded implementation can also be used on POSIX systems. Since this option is intended only as playground on POSIX, but is mandatory on Windows, the option is not documented. but I am wondering how much of the real world is threads-challenged these days. Here is what you have at the end of technical/api-run-command.txt: There are serious restrictions on what the asynchronous function can do because this facility is implemented by a pipe to a forked process on UNIX, but by a thread in the same address space on Windows: . It cannot change the program's state (global variables, environment, etc.) in a way that the caller notices; in other words, .in and .out are the only communication channels to the caller. . It must not change the program's state that the caller of the facility also uses. And calling die() from async is obviously "change the program's state that the caller of the facility also uses". We didn't uncover this as a bug because the above "serious restrictions" go both ways. If we make threaded-async the default on any platform that is thread capable, we would increase the likelihood of catching bugs that violate the latter condition. I am sure there may be downsides for going that route, but it might be better than the current situation where two major platforms use quite different underlying semantics for the same call, and rely on the program (both caller and callee) to honor the above conditions, without much tool support [*1*]. [Footnote] *1* I sometimes wonder if people who are interseted in static analysis can help with issues like this: "In a function started by start_async() and its callees, you are not supposed to touch these globals, nor call those functions". That would be more useful than reports we occasionally get "in this codepath this variable can be used before assigned" with many false positives, that presumably come from from the canned set of rules these static checkers may have. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/6] Pass t5530 on Windows 2010-03-06 20:12 ` Junio C Hamano @ 2010-03-06 21:50 ` Shawn O. Pearce 2010-03-09 20:00 ` [PATCH 7/6] Enable threaded async procedures whenever pthreads is available Johannes Sixt 0 siblings, 1 reply; 16+ messages in thread From: Shawn O. Pearce @ 2010-03-06 21:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, git Junio C Hamano <gitster@pobox.com> wrote: > Johannes Sixt <j6t@kdbg.org> writes: > > > Quite frankly, I don't quite know what to do with this series. On the > > one hand, it is a clean-up, but in practice it is not relevant whether > > die() kills only the async thread or the whole process because all > > callers of async die() themselves anyway when the async procedure died. > > On the other hand, it does enable threaded async procedures on POSIX... ... > . It must not change the program's state that the caller of the > facility also uses. > > And calling die() from async is obviously "change the program's state that > the caller of the facility also uses". We didn't uncover this as a bug > because the above "serious restrictions" go both ways. I agree with you Junio. I think that any async helper that is invoking die() in its code path is wrong. Just like its also wrong for an async helper to try and use the sha1_file.c family of functions. The helper should return failure and let its caller handle the process termination. Hell, its even wrong for an async helper to use xmalloc(), because in a low-memory situation that xmalloc may try to remove pack windows *without locking*. _DOUBLE_PLUS_UNGOOD_ > If we make threaded-async the default on any platform that is thread > capable, we would increase the likelihood of catching bugs that violate > the latter condition. I'm in favor of that. If we have threaded delta search enabled, we probably can also run these async procedures in a POSIX thread rather than forking off a child. Though on our primary target platform of Linux, the performance difference either way probably cannot be measured. -- Shawn. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-06 21:50 ` Shawn O. Pearce @ 2010-03-09 20:00 ` Johannes Sixt 2010-03-09 23:43 ` Shawn O. Pearce 2010-03-10 22:28 ` Junio C Hamano 0 siblings, 2 replies; 16+ messages in thread From: Johannes Sixt @ 2010-03-09 20:00 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: Junio C Hamano, git Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- On Samstag, 6. März 2010, Shawn O. Pearce wrote: > I'm in favor of that. If we have threaded delta search enabled, > we probably can also run these async procedures in a POSIX thread > rather than forking off a child. OK. The patch could look like this. -- Hannes Documentation/technical/api-run-command.txt | 5 +++-- Makefile | 5 ----- run-command.c | 6 +++--- run-command.h | 4 ++-- 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt index 44876fa..f18b4f4 100644 --- a/Documentation/technical/api-run-command.txt +++ b/Documentation/technical/api-run-command.txt @@ -231,8 +231,9 @@ The function pointer in .proc has the following signature: There are serious restrictions on what the asynchronous function can do -because this facility is implemented by a pipe to a forked process on -UNIX, but by a thread in the same address space on Windows: +because this facility is implemented by a thread in the same address +space on most platforms (when pthreads is available), but by a pipe to +a forked process otherwise: . It cannot change the program's state (global variables, environment, etc.) in a way that the caller notices; in other words, .in and .out diff --git a/Makefile b/Makefile index 2fe52f8..52f2cc0 100644 --- a/Makefile +++ b/Makefile @@ -979,7 +979,6 @@ ifeq ($(uname_S),Windows) NO_CURL = YesPlease NO_PYTHON = YesPlease BLK_SHA1 = YesPlease - ASYNC_AS_THREAD = YesPlease CC = compat/vcbuild/scripts/clink.pl AR = compat/vcbuild/scripts/lib.pl @@ -1031,7 +1030,6 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_REGEX = YesPlease NO_PYTHON = YesPlease BLK_SHA1 = YesPlease - ASYNC_AS_THREAD = YesPlease COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch -Icompat/win32 COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \ @@ -1344,9 +1342,6 @@ ifdef NO_PTHREADS else EXTLIBS += $(PTHREAD_LIBS) LIB_OBJS += thread-utils.o -ifdef ASYNC_AS_THREAD - BASIC_CFLAGS += -DASYNC_AS_THREAD -endif endif ifdef DIR_HAS_BSD_GROUP_SEMANTICS diff --git a/run-command.c b/run-command.c index 66cc4bf..053b28f 100644 --- a/run-command.c +++ b/run-command.c @@ -447,7 +447,7 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const return run_command(&cmd); } -#ifdef ASYNC_AS_THREAD +#ifndef NO_PTHREADS static pthread_t main_thread; static int main_thread_set; static pthread_key_t async_key; @@ -521,7 +521,7 @@ int start_async(struct async *async) else proc_out = -1; -#ifndef ASYNC_AS_THREAD +#ifdef NO_PTHREADS /* Flush stdio before fork() to avoid cloning buffers */ fflush(NULL); @@ -590,7 +590,7 @@ error: int finish_async(struct async *async) { -#ifndef ASYNC_AS_THREAD +#ifdef NO_PTHREADS return wait_or_whine(async->pid, "child process", 0); #else void *ret = (void *)(intptr_t)(-1); diff --git a/run-command.h b/run-command.h index 40db39c..56491b9 100644 --- a/run-command.h +++ b/run-command.h @@ -1,7 +1,7 @@ #ifndef RUN_COMMAND_H #define RUN_COMMAND_H -#ifdef ASYNC_AS_THREAD +#ifndef NO_PTHREADS #include <pthread.h> #endif @@ -78,7 +78,7 @@ struct async { void *data; int in; /* caller writes here and closes it */ int out; /* caller reads from here and closes it */ -#ifndef ASYNC_AS_THREAD +#ifdef NO_PTHREADS pid_t pid; #else pthread_t tid; -- 1.7.0.rc2.65.g7b13a ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-09 20:00 ` [PATCH 7/6] Enable threaded async procedures whenever pthreads is available Johannes Sixt @ 2010-03-09 23:43 ` Shawn O. Pearce 2010-03-10 22:28 ` Junio C Hamano 1 sibling, 0 replies; 16+ messages in thread From: Shawn O. Pearce @ 2010-03-09 23:43 UTC (permalink / raw) To: Johannes Sixt; +Cc: Junio C Hamano, git Johannes Sixt <j6t@kdbg.org> wrote: > Signed-off-by: Johannes Sixt <j6t@kdbg.org> > --- > On Samstag, 6. M?rz 2010, Shawn O. Pearce wrote: > > I'm in favor of that. If we have threaded delta search enabled, > > we probably can also run these async procedures in a POSIX thread > > rather than forking off a child. > > OK. The patch could look like this. Looks fine to me. But given my earlier statement about xmalloc() damage... I wonder if we shouldn't try to ensure that isn't a problem first. -- Shawn. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-09 20:00 ` [PATCH 7/6] Enable threaded async procedures whenever pthreads is available Johannes Sixt 2010-03-09 23:43 ` Shawn O. Pearce @ 2010-03-10 22:28 ` Junio C Hamano 2010-03-11 19:53 ` Johannes Sixt 2010-03-17 21:28 ` Johannes Sixt 1 sibling, 2 replies; 16+ messages in thread From: Junio C Hamano @ 2010-03-10 22:28 UTC (permalink / raw) To: Johannes Sixt; +Cc: Shawn O. Pearce, git Johannes Sixt <j6t@kdbg.org> writes: > Signed-off-by: Johannes Sixt <j6t@kdbg.org> > --- > On Samstag, 6. März 2010, Shawn O. Pearce wrote: >> I'm in favor of that. If we have threaded delta search enabled, >> we probably can also run these async procedures in a POSIX thread >> rather than forking off a child. > > OK. The patch could look like this. Will queue in 'pu', but as Shawn said, we should probably give another closer look at the callees that are started with this interface before moving forward. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-10 22:28 ` Junio C Hamano @ 2010-03-11 19:53 ` Johannes Sixt 2010-03-12 5:56 ` Junio C Hamano 2010-03-17 21:28 ` Johannes Sixt 1 sibling, 1 reply; 16+ messages in thread From: Johannes Sixt @ 2010-03-11 19:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Shawn O. Pearce, git On Mittwoch, 10. März 2010, Junio C Hamano wrote: > Will queue in 'pu', but as Shawn said, we should probably give another > closer look at the callees that are started with this interface before > moving forward. I'll audit the call paths. But I'll need some time for this. -- Hannes ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-11 19:53 ` Johannes Sixt @ 2010-03-12 5:56 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2010-03-12 5:56 UTC (permalink / raw) To: Johannes Sixt; +Cc: Junio C Hamano, Shawn O. Pearce, git Johannes Sixt <j6t@kdbg.org> writes: > On Mittwoch, 10. März 2010, Junio C Hamano wrote: >> Will queue in 'pu', but as Shawn said, we should probably give another >> closer look at the callees that are started with this interface before >> moving forward. > > I'll audit the call paths. But I'll need some time for this. Thanks for volunteering. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-10 22:28 ` Junio C Hamano 2010-03-11 19:53 ` Johannes Sixt @ 2010-03-17 21:28 ` Johannes Sixt 2010-03-17 22:19 ` Junio C Hamano 2010-03-23 8:15 ` Fredrik Kuivinen 1 sibling, 2 replies; 16+ messages in thread From: Johannes Sixt @ 2010-03-17 21:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: Shawn O. Pearce, git On Mittwoch, 10. März 2010, Junio C Hamano wrote: > Johannes Sixt <j6t@kdbg.org> writes: > > On Samstag, 6. März 2010, Shawn O. Pearce wrote: > >> I'm in favor of that. If we have threaded delta search enabled, > >> we probably can also run these async procedures in a POSIX thread > >> rather than forking off a child. > > > > OK. The patch could look like this. > > Will queue in 'pu', but as Shawn said, we should probably give another > closer look at the callees that are started with this interface before > moving forward. So here is my analysis. There are currently these users of the async infrastructure: builtin-fetch-pack.c builtin-receive-pack.c builtin-send-pack.c convert.c remote-curl.c upload_pack.c The list below shows all functions that read or write potentially global state that are called from the async procedure (except for upload_pack.c, see below). I ignored all functions that do not depend on global state, like strcmp, memcpy, sprintf, strerror, etc. ---------- remote-curl.c:write_discovery(): write close These two functions only communicate with the parent. This is absolutely thread-safe. ---------- builtin-fetch-pack.c:sideband_demux() and builtin-send-pack.c:sideband_demux() are virtually identical, and builtin-receive-pack.c:copy_to_sideband() has the same footprint: getenv read die_errno die write close Again, the functions only communicate with the parent and nothing else. getenv could be a problem if the parent called setenv, but it doesn't. die (and die_errno) are overridden in the threaded version, and are uncritical. In total, this user is thread-safe. (This is getenv("TERM") from recv_sideband, btw.) ---------- convert.c:filter_buffer() pipe close getenv fork read write fprintf waitpid if GIT_TRACE is set: xrealloc die free This one is less trivial. It calls start_command+finish_command from the async procedure. The parent calls read() xrealloc() (via strbuf_read()) error() (and nothing else) between start_async() and finish_async(). As long as GIT_TRACE is not set, we are safe, because the async procedure carefully releases all global resources that it allocated, and the parent is looking in the other direction while it does os. Even if GIT_TRACE is set, xrealloc() in the parent and the async procedure cannot be called simultanously because the procedure calls it only before it begins writing output, and the parent only after it received this output. On Windows, the situation is slightly different, because we always call into xmalloc() during start_command, but again we do so before the output is produced. I think we are safe in all cases. ---------- upload_pack:create_pack_file(): This user is different because it calls into the revision walker in the async procedure, which definitely affects global state. Therefore, here is the list of functions called by the parent until it exits: pipe close getenv fork read write fprintf waitpid alarm die die_errno pthread_join / waitpid if GIT_TRACE is set: xrealloc die free It also calls start_command+finish_command. If GIT_TRACE is set, we are less safe because this time, xrealloc() can be called simultanously with pack functions while the revision walker is operating. But as long as GIT_TRACE is not set, it looks like we are safe, though I haven't looked through the revision walker whether it messes with alarms or the environment. One aspect is, though, that the async procedure is used *only* for a shallow clone, and the reason that it is used is that pack-objects (or the way it is called from upload-pack?) has not been taught to produce a pack for a shallow clone. This concludes my analysis. -- Hannes ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-17 21:28 ` Johannes Sixt @ 2010-03-17 22:19 ` Junio C Hamano 2010-03-23 8:15 ` Fredrik Kuivinen 1 sibling, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2010-03-17 22:19 UTC (permalink / raw) To: Johannes Sixt; +Cc: Shawn O. Pearce, git Johannes Sixt <j6t@kdbg.org> writes: >> Will queue in 'pu', but as Shawn said, we should probably give another >> closer look at the callees that are started with this interface before >> moving forward. > > So here is my analysis. > ... > This concludes my analysis. Thanks; very much appreciated. I'll read it over and start moving things to 'next', then. I'll be slower than my usual self for the coming few days, though. A bit of patience please. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-17 21:28 ` Johannes Sixt 2010-03-17 22:19 ` Junio C Hamano @ 2010-03-23 8:15 ` Fredrik Kuivinen 2010-03-23 20:19 ` Johannes Sixt 1 sibling, 1 reply; 16+ messages in thread From: Fredrik Kuivinen @ 2010-03-23 8:15 UTC (permalink / raw) To: Johannes Sixt; +Cc: Junio C Hamano, Shawn O. Pearce, git On Wed, Mar 17, 2010 at 22:28, Johannes Sixt <j6t@kdbg.org> wrote: > On Mittwoch, 10. März 2010, Junio C Hamano wrote: >> Johannes Sixt <j6t@kdbg.org> writes: >> > On Samstag, 6. März 2010, Shawn O. Pearce wrote: >> >> I'm in favor of that. If we have threaded delta search enabled, >> >> we probably can also run these async procedures in a POSIX thread >> >> rather than forking off a child. >> > >> > OK. The patch could look like this. >> >> Will queue in 'pu', but as Shawn said, we should probably give another >> closer look at the callees that are started with this interface before >> moving forward. > > So here is my analysis. > > There are currently these users of the async infrastructure: > > builtin-fetch-pack.c > builtin-receive-pack.c > builtin-send-pack.c > convert.c > remote-curl.c > upload_pack.c > > The list below shows all functions that read or write potentially global state > that are called from the async procedure (except for upload_pack.c, see > below). I ignored all functions that do not depend on global state, like > strcmp, memcpy, sprintf, strerror, etc. > > ---------- > convert.c:filter_buffer() > pipe > close > getenv > fork > read > write > fprintf > waitpid > > if GIT_TRACE is set: > xrealloc > die > free > > This one is less trivial. It calls start_command+finish_command from the async > procedure. The parent calls > read() > xrealloc() (via strbuf_read()) > error() > (and nothing else) between start_async() and finish_async(). > > As long as GIT_TRACE is not set, we are safe, because the async procedure > carefully releases all global resources that it allocated, and the parent is > looking in the other direction while it does os. Even if GIT_TRACE is set, > xrealloc() in the parent and the async procedure cannot be called > simultanously because the procedure calls it only before it begins writing > output, and the parent only after it received this output. Maybe I'm missing something but, isn't it possible that xrealloc is called simultaneously from the two threads if GIT_TRACE is set? Immediately after start_async the parent calls strbuf_read. We then get the call chain strbuf_read -> strbuf_grow -> ALLOG_GROW -> xrealloc, so xrealloc is called before we read any data in the parent. In the child we have start_command -> trace_argv_printf -> strbuf_grow -> ... That xmalloc and xrealloc aren't thread-safe feels a bit fragile. Maybe we should try to fix that. > ---------- > upload_pack:create_pack_file(): This user is different because it calls into > the revision walker in the async procedure, which definitely affects global > state. Therefore, here is the list of functions called by the parent until it > exits: > pipe > close > getenv > fork > read > write > fprintf > waitpid > alarm > die > die_errno > pthread_join / waitpid sha1_to_hex is also called by the parent and the current implementation of that function is not thread-safe. sha1_to_hex is also called by some paths in the revision machinery, but I don't know if it will ever be called in this particular case. Maybe it would be a good idea to create wrappers for getenv, setenv, unsetenv, and putenv to make them thread-safe as well. Then won't have to worry about them in the future. - Fredrik ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-23 8:15 ` Fredrik Kuivinen @ 2010-03-23 20:19 ` Johannes Sixt 2010-03-23 20:25 ` Johannes Sixt 2010-03-23 21:42 ` Fredrik Kuivinen 0 siblings, 2 replies; 16+ messages in thread From: Johannes Sixt @ 2010-03-23 20:19 UTC (permalink / raw) To: Fredrik Kuivinen; +Cc: Junio C Hamano, Shawn O. Pearce, git On Dienstag, 23. März 2010, Fredrik Kuivinen wrote: > On Wed, Mar 17, 2010 at 22:28, Johannes Sixt <j6t@kdbg.org> wrote: > > ---------- > > convert.c:filter_buffer() > ... > Maybe I'm missing something but, isn't it possible that xrealloc is > called simultaneously from the two threads if GIT_TRACE is set? > > Immediately after start_async the parent calls strbuf_read. We then > get the call chain > strbuf_read -> strbuf_grow -> ALLOG_GROW -> xrealloc, so xrealloc is > called before we read any data in the parent. > > In the child we have start_command -> trace_argv_printf -> strbuf_grow -> > ... Outch! You are right. It seems I missed the call of strbuf_grow before the loop in strbuf_read. OK, this means that convert.c is not safe if (and only if) GIT_TRACE is set. :-( > That xmalloc and xrealloc aren't thread-safe feels a bit fragile. > Maybe we should try to fix that. The point of this assessment was to find out whether this is necessary (and whether something else that is not thread-safe is used). > > ---------- > > upload_pack:create_pack_file(): > ... > sha1_to_hex is also called by the parent and the current > implementation of that function is not thread-safe. sha1_to_hex is > also called by some paths in the revision machinery, but I don't know > if it will ever be called in this particular case. sha1_to_hex is only called by the parent when the async procedure is not used. -- Hannes ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-23 20:19 ` Johannes Sixt @ 2010-03-23 20:25 ` Johannes Sixt 2010-03-23 20:44 ` Junio C Hamano 2010-03-23 21:42 ` Fredrik Kuivinen 1 sibling, 1 reply; 16+ messages in thread From: Johannes Sixt @ 2010-03-23 20:25 UTC (permalink / raw) To: Fredrik Kuivinen; +Cc: Junio C Hamano, Shawn O. Pearce, git On Dienstag, 23. März 2010, Johannes Sixt wrote: > On Dienstag, 23. März 2010, Fredrik Kuivinen wrote: > > On Wed, Mar 17, 2010 at 22:28, Johannes Sixt <j6t@kdbg.org> wrote: > > > ---------- > > > upload_pack:create_pack_file(): > > > > ... > > sha1_to_hex is also called by the parent and the current > > implementation of that function is not thread-safe. sha1_to_hex is > > also called by some paths in the revision machinery, but I don't know > > if it will ever be called in this particular case. > > sha1_to_hex is only called by the parent when the async procedure is not > used. BTW, the real fix for this potentially problematic case is to teach pack-objects to create a pack file for a shallow clone. Then we don't need this instance of an async procedure. -- Hannes ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-23 20:25 ` Johannes Sixt @ 2010-03-23 20:44 ` Junio C Hamano 2010-03-23 21:09 ` Johannes Sixt 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2010-03-23 20:44 UTC (permalink / raw) To: Johannes Sixt; +Cc: Fredrik Kuivinen, Shawn O. Pearce, git Johannes Sixt <j6t@kdbg.org> writes: > BTW, the real fix for this potentially problematic case is to teach > pack-objects to create a pack file for a shallow clone. Then we don't need > this instance of an async procedure. I keep hearing "shallow clone" here, but doesn't "bundle -n 1" essentially do something quite similar using pack-objects? Do they make a call into pack-objects in a different way, and if so can we update "clone" to mimic how "bundle" drives pack-objects? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-23 20:44 ` Junio C Hamano @ 2010-03-23 21:09 ` Johannes Sixt 0 siblings, 0 replies; 16+ messages in thread From: Johannes Sixt @ 2010-03-23 21:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Fredrik Kuivinen, Shawn O. Pearce, git On Dienstag, 23. März 2010, Junio C Hamano wrote: > Johannes Sixt <j6t@kdbg.org> writes: > > BTW, the real fix for this potentially problematic case is to teach > > pack-objects to create a pack file for a shallow clone. Then we don't > > need this instance of an async procedure. > > I keep hearing "shallow clone" here, but doesn't "bundle -n 1" essentially > do something quite similar using pack-objects? Do they make a call into > pack-objects in a different way, and if so can we update "clone" to mimic > how "bundle" drives pack-objects? Both *current* implementations of shallow clone and bundle feed pack-objects with rev-list output. In the case of non-shallow clone, upload-pack does not need rev-list because pack-objects can walk the revisions itself. I cannot tell what pack-objects does not have that it cannot walk the revisions itself for shallow clones. -- Hannes ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available 2010-03-23 20:19 ` Johannes Sixt 2010-03-23 20:25 ` Johannes Sixt @ 2010-03-23 21:42 ` Fredrik Kuivinen 1 sibling, 0 replies; 16+ messages in thread From: Fredrik Kuivinen @ 2010-03-23 21:42 UTC (permalink / raw) To: Johannes Sixt; +Cc: Junio C Hamano, Shawn O. Pearce, git On Tue, Mar 23, 2010 at 21:19, Johannes Sixt <j6t@kdbg.org> wrote: > On Dienstag, 23. März 2010, Fredrik Kuivinen wrote: >> On Wed, Mar 17, 2010 at 22:28, Johannes Sixt <j6t@kdbg.org> wrote: >> That xmalloc and xrealloc aren't thread-safe feels a bit fragile. >> Maybe we should try to fix that. > > The point of this assessment was to find out whether this is necessary (and > whether something else that is not thread-safe is used). It may not be necessary now, but my point was that by having thread-unsafe xmalloc and xrealloc it is a bit too easy to introduce new bugs. >> > ---------- >> > upload_pack:create_pack_file(): >> ... >> sha1_to_hex is also called by the parent and the current >> implementation of that function is not thread-safe. sha1_to_hex is >> also called by some paths in the revision machinery, but I don't know >> if it will ever be called in this particular case. > > sha1_to_hex is only called by the parent when the async procedure is not used. Yes, you are right. - Fredrik ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2010-03-23 21:42 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-03-06 15:40 [PATCH 0/6] Pass t5530 on Windows Johannes Sixt 2010-03-06 20:12 ` Junio C Hamano 2010-03-06 21:50 ` Shawn O. Pearce 2010-03-09 20:00 ` [PATCH 7/6] Enable threaded async procedures whenever pthreads is available Johannes Sixt 2010-03-09 23:43 ` Shawn O. Pearce 2010-03-10 22:28 ` Junio C Hamano 2010-03-11 19:53 ` Johannes Sixt 2010-03-12 5:56 ` Junio C Hamano 2010-03-17 21:28 ` Johannes Sixt 2010-03-17 22:19 ` Junio C Hamano 2010-03-23 8:15 ` Fredrik Kuivinen 2010-03-23 20:19 ` Johannes Sixt 2010-03-23 20:25 ` Johannes Sixt 2010-03-23 20:44 ` Junio C Hamano 2010-03-23 21:09 ` Johannes Sixt 2010-03-23 21:42 ` Fredrik Kuivinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.