Re: [PATCH v4] Handle atexit list internaly for unthreaded builds

Etienne Buira [off-list ref] writes:

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?

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.

Thanks-to: Duy Nguyen [off-list ref]
Thanks-to: Andreas Schwab [off-list ref]

Usually we spell these "Helped-by: " instead.

Signed-off-by: Etienne Buira <redacted>
---

Thanks.

On Mon, Oct 13, 2014 at 10:00 PM, Junio C Hamano [off-list ref] wrote:

Etienne Buira [off-list ref] writes:

quoted

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?

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

quoted

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.

quoted

Thanks-to: Duy Nguyen [off-list ref]
Thanks-to: Andreas Schwab [off-list ref]

Usually we spell these "Helped-by: " instead.

quoted

Signed-off-by: Etienne Buira <redacted>
---

Thanks.

quoted

 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",

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 [off-list ref]
Helped-by: Andreas Schwab [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Etienne Buira <redacted>
---
 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