patch, 6 partsRe: [PATCH 0/6] Pass t5530 on Windows

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(-)

Johannes Sixt <j6t@kdbg.org> writes:

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.

Junio C Hamano <gitster@pobox.com> wrote:

...

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_
 

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.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
On Samstag, 6. März 2010, Shawn O. Pearce wrote:

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(-)

Johannes Sixt <j6t@kdbg.org> wrote:

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.

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.

On Mittwoch, 10. März 2010, Junio C Hamano wrote:

I'll audit the call paths. But I'll need some time for this.

-- Hannes

Johannes Sixt <j6t@kdbg.org> writes:

Thanks for volunteering.

On Mittwoch, 10. März 2010, Junio C Hamano wrote:

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

Johannes Sixt <j6t@kdbg.org> writes:

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.

On Wed, Mar 17, 2010 at 22:28, Johannes Sixt <j6t@kdbg.org> wrote:

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.

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

On Dienstag, 23. März 2010, Fredrik Kuivinen wrote:

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. :-(

The point of this assessment was to find out whether this is necessary (and whether something else that is not thread-safe is used).

sha1_to_hex is only called by the parent when the async procedure is not used.

-- Hannes

On Dienstag, 23. März 2010, Johannes Sixt wrote:

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

Johannes Sixt <j6t@kdbg.org> writes:

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?

On Dienstag, 23. März 2010, Junio C Hamano wrote:

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

On Tue, Mar 23, 2010 at 21:19, Johannes Sixt <j6t@kdbg.org> wrote:

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.

Yes, you are right.

- Fredrik