[PATCH] Handle atexit list internaly fo unthreaded builds

Discussion:

Etienne Buira

2014-10-11 14:53:38 UTC

Replace atexit()s calls with cmd_atexit that is atexit() on threaded
builds, but handles the callbacks list internally for unthreaded builds.

This is needed because on unthreaded builds, asyncs inherits parent's
atexit() list, that gets run as soon as the async exit()s (and again at
the end of the parent process). That led to remove temporary and lock
files too early.

Fixes test 5537 (temporary shallow file vanished before unpack-objects
could open it)

Signed-off-by: Etienne Buira <***@gmail.com>
---
builtin/clone.c | 7 +------
builtin/fetch.c | 2 +-
builtin/gc.c | 2 +-
diff.c | 2 +-
lockfile.c | 2 +-
pager.c | 2 +-
read-cache.c | 2 +-
run-command.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
shallow.c | 7 ++-----
trace.c | 2 +-
10 files changed, 54 insertions(+), 19 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index bbd169c..2992ac0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -390,7 +390,6 @@ static void clone_local(const char *src_repo, const char *dest_repo)

static const char *junk_work_tree;
static const char *junk_git_dir;
-static pid_t junk_pid;
static enum {
JUNK_LEAVE_NONE,
JUNK_LEAVE_REPO,
@@ -417,8 +416,6 @@ static void remove_junk(void)
break;
}

- if (getpid() != junk_pid)
- return;
if (junk_git_dir) {
strbuf_addstr(&sb, junk_git_dir);
remove_dir_recursively(&sb, 0);
@@ -758,8 +755,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
struct refspec *refspec;
const char *fetch_pattern;

- junk_pid = getpid();
-
packet_trace_identity("clone");
argc = parse_options(argc, argv, prefix, builtin_clone_options,
builtin_clone_usage, 0);
@@ -843,7 +838,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
set_git_work_tree(work_tree);
}
junk_git_dir = git_dir;
- atexit(remove_junk);
+ cmd_atexit(remove_junk);
sigchain_push_common(remove_junk_on_signal);

if (safe_create_leading_directories_const(git_dir) < 0)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 159fb7e..1e44d58 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1095,7 +1095,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
}

sigchain_push_common(unlock_pack_on_signal);
- atexit(unlock_pack);
+ cmd_atexit(unlock_pack);
refspec = parse_fetch_refspec(ref_nr, refs);
exit_code = do_fetch(gtransport, refspec, ref_nr);
free_refspec(ref_nr, refspec);
diff --git a/builtin/gc.c b/builtin/gc.c
index 8d219d8..cf2defa 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -254,7 +254,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)

pidfile = git_pathdup("gc.pid");
sigchain_push_common(remove_pidfile_on_signal);
- atexit(remove_pidfile);
+ cmd_atexit(remove_pidfile);

return NULL;
}
diff --git a/diff.c b/diff.c
index 867f034..9c6ef9a 100644
--- a/diff.c
+++ b/diff.c
@@ -2833,7 +2833,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
}

if (!remove_tempfile_installed) {
- atexit(remove_tempfile);
+ cmd_atexit(remove_tempfile);
sigchain_push_common(remove_tempfile_on_signal);
remove_tempfile_installed = 1;
}
diff --git a/lockfile.c b/lockfile.c
index 2564a7f..ad0d1e2 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -141,7 +141,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
if (0 <= lk->fd) {
if (!lock_file_list) {
sigchain_push_common(remove_lock_file_on_signal);
- atexit(remove_lock_file);
+ cmd_atexit(remove_lock_file);
}
lk->owner = getpid();
if (!lk->on_list) {
diff --git a/pager.c b/pager.c
index 8b5cbc5..09ab2fa 100644
--- a/pager.c
+++ b/pager.c
@@ -102,7 +102,7 @@ void setup_pager(void)

/* this makes sure that the parent terminates after the pager */
sigchain_push_common(wait_for_pager_signal);
- atexit(wait_for_pager);
+ cmd_atexit(wait_for_pager);
}

int pager_in_use(void)
diff --git a/read-cache.c b/read-cache.c
index 6f0057f..8b10c92 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2104,7 +2104,7 @@ static int write_shared_index(struct index_state *istate,
return do_write_locked_index(istate, lock, flags);
}
if (!installed_handler) {
- atexit(remove_temporary_sharedindex);
+ cmd_atexit(remove_temporary_sharedindex);
sigchain_push_common(remove_temporary_sharedindex_on_signal);
}
move_cache_to_base_index(istate);
diff --git a/run-command.c b/run-command.c
index 35a3ebf..5df57b5 100644
--- a/run-command.c
+++ b/run-command.c
@@ -45,7 +45,7 @@ static void mark_child_for_cleanup(pid_t pid)
children_to_clean = p;

if (!installed_child_cleanup_handler) {
- atexit(cleanup_children_on_exit);
+ cmd_atexit(cleanup_children_on_exit);
sigchain_push_common(cleanup_children_on_signal);
installed_child_cleanup_handler = 1;
}
@@ -624,6 +624,48 @@ static int async_die_is_recursing(void)
return ret != NULL;
}

+int cmd_atexit(void (*handler)(void))
+{
+ return atexit(handler);
+}
+
+#else
+
+static struct {
+ void (**handlers)(void);
+ size_t nr;
+ size_t alloc;
+} cmd_atexit_hdlrs;
+
+static int cmd_atexit_installed;
+
+static void cmd_atexit_dispatch()
+{
+ size_t i;
+
+ for (i=cmd_atexit_hdlrs.nr ; i ; i--)
+ cmd_atexit_hdlrs.handlers[i-1]();
+}
+
+static void cmd_atexit_clear()
+{
+ free(cmd_atexit_hdlrs.handlers);
+ memset(&cmd_atexit_hdlrs, 0, sizeof(cmd_atexit_hdlrs));
+ cmd_atexit_installed = 0;
+}
+
+int cmd_atexit(void (*handler)(void))
+{
+ ALLOC_GROW(cmd_atexit_hdlrs.handlers, cmd_atexit_hdlrs.nr + 1, cmd_atexit_hdlrs.alloc);
+ cmd_atexit_hdlrs.handlers[cmd_atexit_hdlrs.nr++] = handler;
+ if (!cmd_atexit_installed) {
+ if (!atexit(&cmd_atexit_dispatch))
+ return -1;
+ cmd_atexit_installed = 1;
+ }
+ return 0;
+}
+
#endif

int start_async(struct async *async)
@@ -682,6 +724,7 @@ int start_async(struct async *async)
close(fdin[1]);
if (need_out)
close(fdout[0]);
+ cmd_atexit_clear();
exit(!!async->proc(proc_in, proc_out, async->data));
}

diff --git a/shallow.c b/shallow.c
index de07709..881580b 100644
--- a/shallow.c
+++ b/shallow.c
@@ -226,7 +226,6 @@ static void remove_temporary_shallow_on_signal(int signo)

const char *setup_temporary_shallow(const struct sha1_array *extra)
{
- static int installed_handler;
struct strbuf sb = STRBUF_INIT;
int fd;

@@ -237,10 +236,8 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)
strbuf_addstr(&temporary_shallow, git_path("shallow_XXXXXX"));
fd = xmkstemp(temporary_shallow.buf);

- if (!installed_handler) {
- atexit(remove_temporary_shallow);
- sigchain_push_common(remove_temporary_shallow_on_signal);
- }
+ cmd_atexit(remove_temporary_shallow);
+ sigchain_push_common(remove_temporary_shallow_on_signal);

if (write_in_full(fd, sb.buf, sb.len) != sb.len)
die_errno("failed to write to %s",
diff --git a/trace.c b/trace.c
index e583dc6..36c5076 100644
--- a/trace.c
+++ b/trace.c
@@ -420,7 +420,7 @@ void trace_command_performance(const char **argv)
return;

if (!command_start_time)
- atexit(print_command_performance_atexit);
+ cmd_atexit(print_command_performance_atexit);

strbuf_reset(&command_line);
sq_quote_argv(&command_line, argv, 0);

--
1.8.5.5

Etienne Buira

2014-10-12 09:09:34 UTC

Permalink

Replace atexit()s calls with cmd_atexit that is atexit() on threaded
builds, but handles the callbacks list internally for unthreaded builds.

This is needed because on unthreaded builds, asyncs inherits parent's
atexit() list, that gets run as soon as the async exit()s (and again at
the end of the parent process). That led to remove temporary and lock
files too early.

Fixes test 5537 (temporary shallow file vanished before unpack-objects
could open it)

V2: remove an obvious mistake

Signed-off-by: Etienne Buira <***@gmail.com>
---
builtin/clone.c | 7 +------
builtin/fetch.c | 2 +-
builtin/gc.c | 2 +-
diff.c | 2 +-
lockfile.c | 2 +-
pager.c | 2 +-
read-cache.c | 2 +-
run-command.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
shallow.c | 7 ++-----
trace.c | 2 +-
10 files changed, 54 insertions(+), 19 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index bbd169c..2992ac0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -390,7 +390,6 @@ static void clone_local(const char *src_repo, const char *dest_repo)

static const char *junk_work_tree;
static const char *junk_git_dir;
-static pid_t junk_pid;
static enum {
JUNK_LEAVE_NONE,
JUNK_LEAVE_REPO,
@@ -417,8 +416,6 @@ static void remove_junk(void)
break;
}

- if (getpid() != junk_pid)
- return;
if (junk_git_dir) {
strbuf_addstr(&sb, junk_git_dir);
remove_dir_recursively(&sb, 0);
@@ -758,8 +755,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
struct refspec *refspec;
const char *fetch_pattern;

- junk_pid = getpid();
-
packet_trace_identity("clone");
argc = parse_options(argc, argv, prefix, builtin_clone_options,
builtin_clone_usage, 0);
@@ -843,7 +838,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
set_git_work_tree(work_tree);
}
junk_git_dir = git_dir;
- atexit(remove_junk);
+ cmd_atexit(remove_junk);
sigchain_push_common(remove_junk_on_signal);

if (safe_create_leading_directories_const(git_dir) < 0)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 159fb7e..1e44d58 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1095,7 +1095,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
}

sigchain_push_common(unlock_pack_on_signal);
- atexit(unlock_pack);
+ cmd_atexit(unlock_pack);
refspec = parse_fetch_refspec(ref_nr, refs);
exit_code = do_fetch(gtransport, refspec, ref_nr);
free_refspec(ref_nr, refspec);
diff --git a/builtin/gc.c b/builtin/gc.c
index 8d219d8..cf2defa 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -254,7 +254,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)

pidfile = git_pathdup("gc.pid");
sigchain_push_common(remove_pidfile_on_signal);
- atexit(remove_pidfile);
+ cmd_atexit(remove_pidfile);

return NULL;
}
diff --git a/diff.c b/diff.c
index 867f034..9c6ef9a 100644
--- a/diff.c
+++ b/diff.c
@@ -2833,7 +2833,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
}

if (!remove_tempfile_installed) {
- atexit(remove_tempfile);
+ cmd_atexit(remove_tempfile);
sigchain_push_common(remove_tempfile_on_signal);
remove_tempfile_installed = 1;
}
diff --git a/lockfile.c b/lockfile.c
index 2564a7f..ad0d1e2 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -141,7 +141,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
if (0 <= lk->fd) {
if (!lock_file_list) {
sigchain_push_common(remove_lock_file_on_signal);
- atexit(remove_lock_file);
+ cmd_atexit(remove_lock_file);
}
lk->owner = getpid();
if (!lk->on_list) {
diff --git a/pager.c b/pager.c
index 8b5cbc5..09ab2fa 100644
--- a/pager.c
+++ b/pager.c
@@ -102,7 +102,7 @@ void setup_pager(void)

/* this makes sure that the parent terminates after the pager */
sigchain_push_common(wait_for_pager_signal);
- atexit(wait_for_pager);
+ cmd_atexit(wait_for_pager);
}

int pager_in_use(void)
diff --git a/read-cache.c b/read-cache.c
index 6f0057f..8b10c92 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2104,7 +2104,7 @@ static int write_shared_index(struct index_state *istate,
return do_write_locked_index(istate, lock, flags);
}
if (!installed_handler) {
- atexit(remove_temporary_sharedindex);
+ cmd_atexit(remove_temporary_sharedindex);
sigchain_push_common(remove_temporary_sharedindex_on_signal);
}
move_cache_to_base_index(istate);
diff --git a/run-command.c b/run-command.c
index 35a3ebf..a8a6374 100644
--- a/run-command.c
+++ b/run-command.c
@@ -45,7 +45,7 @@ static void mark_child_for_cleanup(pid_t pid)
children_to_clean = p;

if (!installed_child_cleanup_handler) {
- atexit(cleanup_children_on_exit);
+ cmd_atexit(cleanup_children_on_exit);
sigchain_push_common(cleanup_children_on_signal);
installed_child_cleanup_handler = 1;
}
@@ -624,6 +624,48 @@ static int async_die_is_recursing(void)
return ret != NULL;
}

+int cmd_atexit(void (*handler)(void))
+{
+ return atexit(handler);
+}
+
+#else
+
+static struct {
+ void (**handlers)(void);
+ size_t nr;
+ size_t alloc;
+} cmd_atexit_hdlrs;
+
+static int cmd_atexit_installed;
+
+static void cmd_atexit_dispatch()
+{
+ size_t i;
+
+ for (i=cmd_atexit_hdlrs.nr ; i ; i--)
+ cmd_atexit_hdlrs.handlers[i-1]();
+}
+
+static void cmd_atexit_clear()
+{
+ free(cmd_atexit_hdlrs.handlers);
+ memset(&cmd_atexit_hdlrs, 0, sizeof(cmd_atexit_hdlrs));
+ cmd_atexit_installed = 0;
+}
+
+int cmd_atexit(void (*handler)(void))
+{
+ ALLOC_GROW(cmd_atexit_hdlrs.handlers, cmd_atexit_hdlrs.nr + 1, cmd_atexit_hdlrs.alloc);
+ cmd_atexit_hdlrs.handlers[cmd_atexit_hdlrs.nr++] = handler;
+ if (!cmd_atexit_installed) {
+ if (atexit(&cmd_atexit_dispatch))
+ return -1;
+ cmd_atexit_installed = 1;
+ }
+ return 0;
+}
+
#endif

int start_async(struct async *async)
@@ -682,6 +724,7 @@ int start_async(struct async *async)
close(fdin[1]);
if (need_out)
close(fdout[0]);
+ cmd_atexit_clear();
exit(!!async->proc(proc_in, proc_out, async->data));
}

diff --git a/shallow.c b/shallow.c
index de07709..881580b 100644
--- a/shallow.c
+++ b/shallow.c
@@ -226,7 +226,6 @@ static void remove_temporary_shallow_on_signal(int signo)

const char *setup_temporary_shallow(const struct sha1_array *extra)
{
- static int installed_handler;
struct strbuf sb = STRBUF_INIT;
int fd;

@@ -237,10 +236,8 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)
strbuf_addstr(&temporary_shallow, git_path("shallow_XXXXXX"));
fd = xmkstemp(temporary_shallow.buf);

- if (!installed_handler) {
- atexit(remove_temporary_shallow);
- sigchain_push_common(remove_temporary_shallow_on_signal);
- }
+ cmd_atexit(remove_temporary_shallow);
+ sigchain_push_common(remove_temporary_shallow_on_signal);

if (write_in_full(fd, sb.buf, sb.len) != sb.len)
die_errno("failed to write to %s",
diff --git a/trace.c b/trace.c
index e583dc6..36c5076 100644
--- a/trace.c
+++ b/trace.c
@@ -420,7 +420,7 @@ void trace_command_performance(const char **argv)
return;

if (!command_start_time)
- atexit(print_command_performance_atexit);
+ cmd_atexit(print_command_performance_atexit);

strbuf_reset(&command_line);
sq_quote_argv(&command_line, argv, 0);

--
2.0.4

Duy Nguyen

2014-10-13 00:56:05 UTC

Permalink

Post by Etienne Buira
Replace atexit()s calls with cmd_atexit that is atexit() on threaded
builds, but handles the callbacks list internally for unthreaded builds.

Maybe hide this in git-compat-util.h and "#define atexit(x)
cmd_atexit(x)"? cmd_ is usually for commands' "main" functions. Maybe
rename it to git_atexit().

--
Duy

Etienne Buira

2014-10-13 15:19:11 UTC

Permalink

Post by Duy Nguyen

Post by Etienne Buira
Replace atexit()s calls with cmd_atexit that is atexit() on threaded
builds, but handles the callbacks list internally for unthreaded builds.

Maybe hide this in git-compat-util.h and "#define atexit(x)
cmd_atexit(x)"?

Updated.

Post by Duy Nguyen
cmd_ is usually for commands' "main" functions. Maybe
rename it to git_atexit().

Indeed, renamed. Thank you.

Etienne Buira

2014-10-13 15:19:12 UTC

Permalink

Wrap atexit()s calls on unthreaded builds to handle callback list
internally.

This is needed because on unthreaded builds, asyncs inherits parent's
atexit() list, that gets run as soon as the async exit()s (and again at
the end of the parent process). That led to remove temporary and lock
files too early.

Fixes test 5537 (temporary shallow file vanished before unpack-objects
could open it)

V2: remove an obvious mistake
V3: wrap, rename and add define in git-compat-util.h

Thanks-to: Duy Nguyen <***@gmail.com>
Signed-off-by: Etienne Buira <***@gmail.com>
---
builtin/clone.c | 5 -----
git-compat-util.h | 5 +++++
run-command.c | 42 ++++++++++++++++++++++++++++++++++++++++++
shallow.c | 7 ++-----
4 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index bbd169c..e122f33 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -390,7 +390,6 @@ static void clone_local(const char *src_repo, const char *dest_repo)

static const char *junk_work_tree;
static const char *junk_git_dir;
-static pid_t junk_pid;
static enum {
JUNK_LEAVE_NONE,
JUNK_LEAVE_REPO,
@@ -417,8 +416,6 @@ static void remove_junk(void)
break;
}

- if (getpid() != junk_pid)
- return;
if (junk_git_dir) {
strbuf_addstr(&sb, junk_git_dir);
remove_dir_recursively(&sb, 0);
@@ -758,8 +755,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
struct refspec *refspec;
const char *fetch_pattern;

- junk_pid = getpid();
-
packet_trace_identity("clone");
argc = parse_options(argc, argv, prefix, builtin_clone_options,
builtin_clone_usage, 0);
diff --git a/git-compat-util.h b/git-compat-util.h
index f587749..6dd63dd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -577,6 +577,11 @@ int inet_pton(int af, const char *src, void *dst);
const char *inet_ntop(int af, const void *src, char *dst, size_t size);
#endif

+#ifdef NO_PTHREADS
+#define atexit git_atexit
+extern int git_atexit(void (*handler)(void));
+#endif
+
extern void release_pack_memory(size_t);

typedef void (*try_to_free_t)(size_t);
diff --git a/run-command.c b/run-command.c
index 35a3ebf..f8dc969 100644
--- a/run-command.c
+++ b/run-command.c
@@ -624,6 +624,47 @@ static int async_die_is_recursing(void)
return ret != NULL;
}

+#else
+
+static struct {
+ void (**handlers)(void);
+ size_t nr;
+ size_t alloc;
+} git_atexit_hdlrs;
+
+static int git_atexit_installed;
+
+static void git_atexit_dispatch()
+{
+ size_t i;
+
+ for (i=git_atexit_hdlrs.nr ; i ; i--)
+ git_atexit_hdlrs.handlers[i-1]();
+}
+
+static void git_atexit_clear()
+{
+ free(git_atexit_hdlrs.handlers);
+ memset(&git_atexit_hdlrs, 0, sizeof(git_atexit_hdlrs));
+ git_atexit_installed = 0;
+}
+
+#define tmp_atexit atexit
+#undef atexit
+int git_atexit(void (*handler)(void))
+{
+ ALLOC_GROW(git_atexit_hdlrs.handlers, git_atexit_hdlrs.nr + 1, git_atexit_hdlrs.alloc);
+ git_atexit_hdlrs.handlers[git_atexit_hdlrs.nr++] = handler;
+ if (!git_atexit_installed) {
+ if (atexit(&git_atexit_dispatch))
+ return -1;
+ git_atexit_installed = 1;
+ }
+ return 0;
+}
+#define atexit tmp_atexit
+#undef tmp_atexit
+
#endif

int start_async(struct async *async)
@@ -682,6 +723,7 @@ int start_async(struct async *async)
close(fdin[1]);
if (need_out)
close(fdout[0]);
+ git_atexit_clear();
exit(!!async->proc(proc_in, proc_out, async->data));
}

diff --git a/shallow.c b/shallow.c
index de07709..f067811 100644
--- a/shallow.c
+++ b/shallow.c
@@ -226,7 +226,6 @@ static void remove_temporary_shallow_on_signal(int signo)

const char *setup_temporary_shallow(const struct sha1_array *extra)
{
- static int installed_handler;
struct strbuf sb = STRBUF_INIT;
int fd;

@@ -237,10 +236,8 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)
strbuf_addstr(&temporary_shallow, git_path("shallow_XXXXXX"));
fd = xmkstemp(temporary_shallow.buf);

- if (!installed_handler) {
- atexit(remove_temporary_shallow);
- sigchain_push_common(remove_temporary_shallow_on_signal);
- }
+ atexit(remove_temporary_shallow);
+ sigchain_push_common(remove_temporary_shallow_on_signal);

if (write_in_full(fd, sb.buf, sb.len) != sb.len)
die_errno("failed to write to %s",

--
2.0.4

Andreas Schwab

2014-10-13 16:37:22 UTC

Permalink

Post by Etienne Buira
+#define tmp_atexit atexit
+#define atexit tmp_atexit
+#undef tmp_atexit

What is this supposed to do?

Andreas.

--
Andreas Schwab, ***@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Etienne Buira

2014-10-13 18:35:58 UTC

Permalink

Wrap atexit()s calls on unthreaded builds to handle callback list
internally.

This is needed because on unthreaded builds, asyncs inherits parent's
atexit() list, that gets run as soon as the async exit()s (and again at
the end of the parent process). That led to remove temporary and lock
files too early.

Fixes test 5537 (temporary shallow file vanished before unpack-objects
could open it)

V4: fix bogus preprocessor directives

Thanks-to: Duy Nguyen <***@gmail.com>
Thanks-to: Andreas Schwab <***@linux-m68k.org>
Signed-off-by: Etienne Buira <***@gmail.com>
---
builtin/clone.c | 5 -----
git-compat-util.h | 5 +++++
run-command.c | 40 ++++++++++++++++++++++++++++++++++++++++
shallow.c | 7 ++-----
4 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index bbd169c..e122f33 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -390,7 +390,6 @@ static void clone_local(const char *src_repo, const char *dest_repo)

static const char *junk_work_tree;
static const char *junk_git_dir;
-static pid_t junk_pid;
static enum {
JUNK_LEAVE_NONE,
JUNK_LEAVE_REPO,
@@ -417,8 +416,6 @@ static void remove_junk(void)
break;
}

- if (getpid() != junk_pid)
- return;
if (junk_git_dir) {
strbuf_addstr(&sb, junk_git_dir);
remove_dir_recursively(&sb, 0);
@@ -758,8 +755,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
struct refspec *refspec;
const char *fetch_pattern;

- junk_pid = getpid();
-
packet_trace_identity("clone");
argc = parse_options(argc, argv, prefix, builtin_clone_options,
builtin_clone_usage, 0);
diff --git a/git-compat-util.h b/git-compat-util.h
index f587749..6dd63dd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -577,6 +577,11 @@ int inet_pton(int af, const char *src, void *dst);
const char *inet_ntop(int af, const void *src, char *dst, size_t size);
#endif

+#ifdef NO_PTHREADS
+#define atexit git_atexit
+extern int git_atexit(void (*handler)(void));
+#endif
+
extern void release_pack_memory(size_t);

typedef void (*try_to_free_t)(size_t);
diff --git a/run-command.c b/run-command.c
index 35a3ebf..0f9a9b0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -624,6 +624,45 @@ static int async_die_is_recursing(void)
return ret != NULL;
}

+#else
+
+static struct {
+ void (**handlers)(void);
+ size_t nr;
+ size_t alloc;
+} git_atexit_hdlrs;
+
+static int git_atexit_installed;
+
+static void git_atexit_dispatch()
+{
+ size_t i;
+
+ for (i=git_atexit_hdlrs.nr ; i ; i--)
+ git_atexit_hdlrs.handlers[i-1]();
+}
+
+static void git_atexit_clear()
+{
+ free(git_atexit_hdlrs.handlers);
+ memset(&git_atexit_hdlrs, 0, sizeof(git_atexit_hdlrs));
+ git_atexit_installed = 0;
+}
+
+#undef atexit
+int git_atexit(void (*handler)(void))
+{
+ ALLOC_GROW(git_atexit_hdlrs.handlers, git_atexit_hdlrs.nr + 1, git_atexit_hdlrs.alloc);
+ git_atexit_hdlrs.handlers[git_atexit_hdlrs.nr++] = handler;
+ if (!git_atexit_installed) {
+ if (atexit(&git_atexit_dispatch))
+ return -1;
+ git_atexit_installed = 1;
+ }
+ return 0;
+}
+#define atexit git_atexit
+
#endif

int start_async(struct async *async)
@@ -682,6 +721,7 @@ int start_async(struct async *async)
close(fdin[1]);
if (need_out)
close(fdout[0]);
+ git_atexit_clear();
exit(!!async->proc(proc_in, proc_out, async->data));
}

diff --git a/shallow.c b/shallow.c
index de07709..f067811 100644
--- a/shallow.c
+++ b/shallow.c
@@ -226,7 +226,6 @@ static void remove_temporary_shallow_on_signal(int signo)

const char *setup_temporary_shallow(const struct sha1_array *extra)
{
- static int installed_handler;
struct strbuf sb = STRBUF_INIT;
int fd;

@@ -237,10 +236,8 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)
strbuf_addstr(&temporary_shallow, git_path("shallow_XXXXXX"));
fd = xmkstemp(temporary_shallow.buf);

- if (!installed_handler) {
- atexit(remove_temporary_shallow);
- sigchain_push_common(remove_temporary_shallow_on_signal);
- }
+ atexit(remove_temporary_shallow);
+ sigchain_push_common(remove_temporary_shallow_on_signal);

if (write_in_full(fd, sb.buf, sb.len) != sb.len)
die_errno("failed to write to %s",

--
2.0.4

Junio C Hamano

2014-10-13 20:00:16 UTC

Permalink

Post by Etienne Buira
Wrap atexit()s calls on unthreaded builds to handle callback list
internally.
This is needed because on unthreaded builds, asyncs inherits parent's
atexit() list, that gets run as soon as the async exit()s (and again at
the end of the parent process). That led to remove temporary and lock
files too early.

... that does not explain what you did to builtin/clone.c at all.
Care to enlighten us, please?

Post by Etienne Buira
Fixes test 5537 (temporary shallow file vanished before unpack-objects
could open it)
V4: fix bogus preprocessor directives

Please do not write this "V4:" line in the log message. People who
read "git log" output and find this description would not know
anything about the previous faulty ones. Putting it _after_ the
three-dash line below will help the reviewers a lot.
Usually we spell these "Helped-by: " instead.

Post by Etienne Buira
---

Thanks.

Post by Etienne Buira
builtin/clone.c | 5 -----
git-compat-util.h | 5 +++++
run-command.c | 40 ++++++++++++++++++++++++++++++++++++++++
shallow.c | 7 ++-----
4 files changed, 47 insertions(+), 10 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index bbd169c..e122f33 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -390,7 +390,6 @@ static void clone_local(const char *src_repo, const char *dest_repo)
static const char *junk_work_tree;
static const char *junk_git_dir;
-static pid_t junk_pid;
static enum {
JUNK_LEAVE_NONE,
JUNK_LEAVE_REPO,
@@ -417,8 +416,6 @@ static void remove_junk(void)
break;
}
- if (getpid() != junk_pid)
- return;
if (junk_git_dir) {
strbuf_addstr(&sb, junk_git_dir);
remove_dir_recursively(&sb, 0);
@@ -758,8 +755,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
struct refspec *refspec;
const char *fetch_pattern;
- junk_pid = getpid();
-
packet_trace_identity("clone");
argc = parse_options(argc, argv, prefix, builtin_clone_options,
builtin_clone_usage, 0);
diff --git a/git-compat-util.h b/git-compat-util.h
index f587749..6dd63dd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -577,6 +577,11 @@ int inet_pton(int af, const char *src, void *dst);
const char *inet_ntop(int af, const void *src, char *dst, size_t size);
#endif
+#ifdef NO_PTHREADS
+#define atexit git_atexit
+extern int git_atexit(void (*handler)(void));
+#endif
+
extern void release_pack_memory(size_t);
typedef void (*try_to_free_t)(size_t);
diff --git a/run-command.c b/run-command.c
index 35a3ebf..0f9a9b0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -624,6 +624,45 @@ static int async_die_is_recursing(void)
return ret != NULL;
}
+#else
+
+static struct {
+ void (**handlers)(void);
+ size_t nr;
+ size_t alloc;
+} git_atexit_hdlrs;
+
+static int git_atexit_installed;
+
+static void git_atexit_dispatch()
+{
+ size_t i;
+
+ for (i=git_atexit_hdlrs.nr ; i ; i--)
+ git_atexit_hdlrs.handlers[i-1]();
+}
+
+static void git_atexit_clear()
+{
+ free(git_atexit_hdlrs.handlers);
+ memset(&git_atexit_hdlrs, 0, sizeof(git_atexit_hdlrs));
+ git_atexit_installed = 0;
+}
+
+#undef atexit
+int git_atexit(void (*handler)(void))
+{
+ ALLOC_GROW(git_atexit_hdlrs.handlers, git_atexit_hdlrs.nr + 1, git_atexit_hdlrs.alloc);
+ git_atexit_hdlrs.handlers[git_atexit_hdlrs.nr++] = handler;
+ if (!git_atexit_installed) {
+ if (atexit(&git_atexit_dispatch))
+ return -1;
+ git_atexit_installed = 1;
+ }
+ return 0;
+}
+#define atexit git_atexit
+
#endif
int start_async(struct async *async)
@@ -682,6 +721,7 @@ int start_async(struct async *async)
close(fdin[1]);
if (need_out)
close(fdout[0]);
+ git_atexit_clear();
exit(!!async->proc(proc_in, proc_out, async->data));
}
diff --git a/shallow.c b/shallow.c
index de07709..f067811 100644
--- a/shallow.c
+++ b/shallow.c
@@ -226,7 +226,6 @@ static void remove_temporary_shallow_on_signal(int signo)
const char *setup_temporary_shallow(const struct sha1_array *extra)
{
- static int installed_handler;
struct strbuf sb = STRBUF_INIT;
int fd;
@@ -237,10 +236,8 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)
strbuf_addstr(&temporary_shallow, git_path("shallow_XXXXXX"));
fd = xmkstemp(temporary_shallow.buf);
- if (!installed_handler) {
- atexit(remove_temporary_shallow);
- sigchain_push_common(remove_temporary_shallow_on_signal);
- }
+ atexit(remove_temporary_shallow);
+ sigchain_push_common(remove_temporary_shallow_on_signal);
if (write_in_full(fd, sb.buf, sb.len) != sb.len)
die_errno("failed to write to %s",

Etienne Buira

2014-10-14 15:02:27 UTC

Permalink

Post by Junio C Hamano

... that does not explain what you did to builtin/clone.c at all.
Care to enlighten us, please?

Checking current pid against the one that registered the atexit()
routine (what builtin/clone.c currently does) is another way to guard
against the same issue, but it needs to store a pid for each atexit()
call whenever code called after might use async.
From what I've seen, two out of all atexit() calls were guarded like that:
- builtin/clone.c:cmd_clone
- lockfile.c:lock_file (overlooked it at first, would you mind to
s/temporary and lock files/temporary files/ the commit message if no
other round is needed? I'll do it otherwise).

So the changes in builtin/clone.c are just there because the patch
makes this check redundant (still needed in lockfile.c, as it loops
over a list that might refer to parent process's lockfiles).

Post by Junio C Hamano

Post by Etienne Buira
Fixes test 5537 (temporary shallow file vanished before unpack-objects
could open it)
V4: fix bogus preprocessor directives

Post by Etienne Buira
---

Thanks.

Etienne Buira

2014-10-18 12:31:15 UTC

Permalink

Wrap atexit()s calls on unthreaded builds to handle callback list
internally.

This is needed because on unthreaded builds, asyncs inherits parent's
atexit() list, that gets run as soon as the async exit()s (and again at
the end of async's parent process). That led to remove temporary files
too early.

Also remove a by-atexit-callback guard against this kind of issue in
clone.c, as this patch makes it redundant.

Fixes test 5537 (temporary shallow file vanished before unpack-objects
could open it)

BTW remove an unused variable in shallow.c.

Helped-by: Duy Nguyen <***@gmail.com>
Helped-by: Andreas Schwab <***@linux-m68k.org>
Helped-by: Junio C Hamano <***@pobox.com>
Signed-off-by: Etienne Buira <***@gmail.com>
---
builtin/clone.c | 5 -----
git-compat-util.h | 5 +++++
run-command.c | 40 ++++++++++++++++++++++++++++++++++++++++
shallow.c | 7 ++-----
4 files changed, 47 insertions(+), 10 deletions(-)

V5: update commit message

diff --git a/builtin/clone.c b/builtin/clone.c
index bbd169c..e122f33 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -390,7 +390,6 @@ static void clone_local(const char *src_repo, const char *dest_repo)

static const char *junk_work_tree;
static const char *junk_git_dir;
-static pid_t junk_pid;
static enum {
JUNK_LEAVE_NONE,
JUNK_LEAVE_REPO,
@@ -417,8 +416,6 @@ static void remove_junk(void)
break;
}

- if (getpid() != junk_pid)
- return;
if (junk_git_dir) {
strbuf_addstr(&sb, junk_git_dir);
remove_dir_recursively(&sb, 0);
@@ -758,8 +755,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
struct refspec *refspec;
const char *fetch_pattern;

- junk_pid = getpid();
-
packet_trace_identity("clone");
argc = parse_options(argc, argv, prefix, builtin_clone_options,
builtin_clone_usage, 0);
diff --git a/git-compat-util.h b/git-compat-util.h
index f587749..6dd63dd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -577,6 +577,11 @@ int inet_pton(int af, const char *src, void *dst);
const char *inet_ntop(int af, const void *src, char *dst, size_t size);
#endif

+#ifdef NO_PTHREADS
+#define atexit git_atexit
+extern int git_atexit(void (*handler)(void));
+#endif
+
extern void release_pack_memory(size_t);

typedef void (*try_to_free_t)(size_t);
diff --git a/run-command.c b/run-command.c
index 35a3ebf..0f9a9b0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -624,6 +624,45 @@ static int async_die_is_recursing(void)
return ret != NULL;
}

+#else
+
+static struct {
+ void (**handlers)(void);
+ size_t nr;
+ size_t alloc;
+} git_atexit_hdlrs;
+
+static int git_atexit_installed;
+
+static void git_atexit_dispatch()
+{
+ size_t i;
+
+ for (i=git_atexit_hdlrs.nr ; i ; i--)
+ git_atexit_hdlrs.handlers[i-1]();
+}
+
+static void git_atexit_clear()
+{
+ free(git_atexit_hdlrs.handlers);
+ memset(&git_atexit_hdlrs, 0, sizeof(git_atexit_hdlrs));
+ git_atexit_installed = 0;
+}
+
+#undef atexit
+int git_atexit(void (*handler)(void))
+{
+ ALLOC_GROW(git_atexit_hdlrs.handlers, git_atexit_hdlrs.nr + 1, git_atexit_hdlrs.alloc);
+ git_atexit_hdlrs.handlers[git_atexit_hdlrs.nr++] = handler;
+ if (!git_atexit_installed) {
+ if (atexit(&git_atexit_dispatch))
+ return -1;
+ git_atexit_installed = 1;
+ }
+ return 0;
+}
+#define atexit git_atexit
+
#endif

int start_async(struct async *async)
@@ -682,6 +721,7 @@ int start_async(struct async *async)
close(fdin[1]);
if (need_out)
close(fdout[0]);
+ git_atexit_clear();
exit(!!async->proc(proc_in, proc_out, async->data));
}

diff --git a/shallow.c b/shallow.c
index de07709..f067811 100644
--- a/shallow.c
+++ b/shallow.c
@@ -226,7 +226,6 @@ static void remove_temporary_shallow_on_signal(int signo)

const char *setup_temporary_shallow(const struct sha1_array *extra)
{
- static int installed_handler;
struct strbuf sb = STRBUF_INIT;
int fd;

@@ -237,10 +236,8 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)
strbuf_addstr(&temporary_shallow, git_path("shallow_XXXXXX"));
fd = xmkstemp(temporary_shallow.buf);

- if (!installed_handler) {
- atexit(remove_temporary_shallow);
- sigchain_push_common(remove_temporary_shallow_on_signal);
- }
+ atexit(remove_temporary_shallow);
+ sigchain_push_common(remove_temporary_shallow_on_signal);

if (write_in_full(fd, sb.buf, sb.len) != sb.len)
die_errno("failed to write to %s",

--
2.0.4