On Mittwoch, 10. März 2010, Junio C Hamano wrote:
> Johannes Sixt <j6t@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html