Patchwork [3,of,3] chg: hold a lock file before connected to server

login
register
mail settings
Submitter Jun Wu
Date Feb. 16, 2016, 11:12 a.m.
Message ID <c8b24417f06cdcd4821e.1455621122@x1c>
Download mbox | patch
Permalink /patch/13225/
State Superseded
Commit 87de4a22e8c23f5b631b3e4bf3b258aed6e8eec8
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Feb. 16, 2016, 11:12 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1455620932 0
#      Tue Feb 16 11:08:52 2016 +0000
# Node ID c8b24417f06cdcd4821e069eeb020e2da579a008
# Parent  0ce18cbc108990527291701bb0e857f1e05288ef
chg: hold a lock file before connected to server

This is a part of the one server per config series. In multiple-server setup,
multiple clients may try to start different servers (on demand) at the same
time. The old lock will not guarntee a client to connect to the server it
just started, and is not crash friendly.

This patch addressed above issues by using flock and does not release the lock
until the client actually connects to the server or times out.
Yuya Nishihara - Feb. 17, 2016, 4:16 p.m.
On Tue, 16 Feb 2016 11:12:02 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1455620932 0
> #      Tue Feb 16 11:08:52 2016 +0000
> # Node ID c8b24417f06cdcd4821e069eeb020e2da579a008
> # Parent  0ce18cbc108990527291701bb0e857f1e05288ef
> chg: hold a lock file before connected to server

Looked briefly and the direction seems good. lockfd leak is carefully avoided.
I'll revisit this patch later.
Jun Wu - Feb. 17, 2016, 5:41 p.m.
On 02/17/2016 04:16 PM, Yuya Nishihara wrote:
> Looked briefly and the direction seems good. lockfd leak is carefully
> avoided. I'll revisit this patch later.

Known issue: missing "static" for "connectcmdserver".

I still want to do change to --daemon-pipefds first. Having to create a
useless dummy file makes me sad.
Matt Mackall - Feb. 29, 2016, 8:40 p.m.
On Tue, 2016-02-16 at 11:12 +0000, Jun Wu wrote:
> chg: hold a lock file before connected to server

> +	int r = flock(opts->lockfd, LOCK_EX);

You're aware that this won't work on many (most?) network filesystems or on non-
Unix systems, right? Seems a pretty unfortunate choice.

-- 
Mathematics is the supreme nostalgia of our time.
Jun Wu - Feb. 29, 2016, 9:40 p.m.
On 02/29/2016 08:40 PM, Matt Mackall wrote:
> You're aware that this won't work on many (most?) network filesystems
> or on non- Unix systems, right? Seems a pretty unfortunate choice.

Thanks for the reminder. This file lives in /tmp, which seems unlikely
to be a network location.

That said, even with a broken lock, chg should still be able to produce
correct output eventually. It may waste some CPU time though.

Windows is not supported by chg because of fork.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -14,6 +14,7 @@ 
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/file.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <sys/un.h>
@@ -34,10 +35,12 @@ 
 	char pidfile[UNIX_PATH_MAX];
 	int argsize;
 	const char **args;
+	int lockfd;
 };
 
 static void initcmdserveropts(struct cmdserveropts *opts) {
 	memset(opts, 0, sizeof(struct cmdserveropts));
+	opts->lockfd = -1;
 }
 
 static void freecmdserveropts(struct cmdserveropts *opts) {
@@ -165,21 +168,33 @@ 
 }
 
 /*
- * Make lock file that indicates cmdserver process is about to start. Created
- * lock file will be deleted by server. (0: success, -1: lock exists)
+ * Acquire a file lock that indicates a client is trying to start and connect
+ * to a server, before executing a command. The lock is released upon exit or
+ * explicitly unlock. Will block if the lock is held by another process.
  */
-static int lockcmdserver(const struct cmdserveropts *opts)
+static void lockcmdserver(struct cmdserveropts *opts)
 {
-	int r;
-	char info[32];
-	r = snprintf(info, sizeof(info), "%d", getpid());
-	if (r < 0 || (size_t)r >= sizeof(info))
-		abortmsg("failed to format lock info");
-	r = symlink(info, opts->lockfile);
-	if (r < 0 && errno != EEXIST)
-		abortmsg("failed to make lock %s (errno = %d)",
-			 opts->lockfile, errno);
-	return r;
+	if (opts->lockfd == -1) {
+		opts->lockfd = open(opts->lockfile, O_RDWR | O_CREAT | O_NOFOLLOW, 0600);
+		if (opts->lockfd == -1)
+			abortmsg("cannot create lock file %s", opts->lockfile);
+	}
+	int r = flock(opts->lockfd, LOCK_EX);
+	if (r == -1)
+		abortmsg("cannot acquire lock");
+}
+
+/*
+ * Release the file lock held by calling lockcmdserver. Will do nothing if
+ * lockcmdserver is not called.
+ */
+static void unlockcmdserver(struct cmdserveropts *opts)
+{
+	if (opts->lockfd == -1)
+		return;
+	flock(opts->lockfd, LOCK_UN);
+	close(opts->lockfd);
+	opts->lockfd = -1;
 }
 
 static void execcmdserver(const struct cmdserveropts *opts)
@@ -196,7 +211,7 @@ 
 		"--cwd", "/",
 		"--cmdserver", "chgunix",
 		"--address", opts->sockname,
-		"--daemon-pipefds", opts->lockfile,
+		"--daemon-pipefds", "",
 		"--pid-file", opts->pidfile,
 		"--config", "extensions.chgserver=",
 		/* wrap root ui so that it can be disabled/enabled by config */
@@ -216,28 +231,20 @@ 
 	free(argv);
 }
 
-/*
- * Sleep until lock file is deleted, i.e. cmdserver process starts listening.
- * If pid is given, it also checks if the child process fails to start.
- */
-static void waitcmdserver(const struct cmdserveropts *opts, pid_t pid)
+/* Retry until we can connect to the server. Give up after some time. */
+hgclient_t* retryconnectcmdserver(struct cmdserveropts *opts, pid_t pid)
 {
 	static const struct timespec sleepreq = {0, 10 * 1000000};
 	int pst = 0;
 
 	for (unsigned int i = 0; i < 10 * 100; i++) {
-		int r;
-		struct stat lst;
-
-		r = lstat(opts->lockfile, &lst);
-		if (r < 0 && errno == ENOENT)
-			return;  /* lock file deleted by server */
-		if (r < 0)
-			goto cleanup;
+		hgclient_t *hgc = hgc_open(opts->sockname);
+		if (hgc)
+			return hgc;
 
 		if (pid > 0) {
 			/* collect zombie if child process fails to start */
-			r = waitpid(pid, &pst, WNOHANG);
+			int r = waitpid(pid, &pst, WNOHANG);
 			if (r != 0)
 				goto cleanup;
 		}
@@ -245,13 +252,10 @@ 
 		nanosleep(&sleepreq, NULL);
 	}
 
-	abortmsg("timed out waiting for cmdserver %s", opts->lockfile);
-	return;
+	abortmsg("timed out waiting for cmdserver %s", opts->sockname);
+	return NULL;
 
 cleanup:
-	if (pid > 0)
-		/* lockfile should be made by this process */
-		unlink(opts->lockfile);
 	if (WIFEXITED(pst)) {
 		abortmsg("cmdserver exited with status %d", WEXITSTATUS(pst));
 	} else if (WIFSIGNALED(pst)) {
@@ -259,26 +263,32 @@ 
 	} else {
 		abortmsg("error white waiting cmdserver");
 	}
+	return NULL;
 }
 
-/* Spawn new background cmdserver */
-static void startcmdserver(const struct cmdserveropts *opts)
+/* Connect to a cmdserver. Will start a new server on demand. */
+hgclient_t *connectcmdserver(struct cmdserveropts *opts)
 {
-	debugmsg("start cmdserver at %s", opts->sockname);
+	hgclient_t *hgc = hgc_open(opts->sockname);
+	if (hgc)
+		return hgc;
 
-	if (lockcmdserver(opts) < 0) {
-		debugmsg("lock file exists, waiting...");
-		waitcmdserver(opts, 0);
-		return;
+	lockcmdserver(opts);
+	hgc = hgc_open(opts->sockname);
+	if (hgc) {
+		unlockcmdserver(opts);
+		debugmsg("cmdserver is started by another process");
+		return hgc;
 	}
 
-	/* remove dead cmdserver socket if any */
-	unlink(opts->sockname);
+	debugmsg("start cmdserver at %s", opts->sockname);
 
 	pid_t pid = fork();
 	if (pid < 0)
 		abortmsg("failed to fork cmdserver process");
 	if (pid == 0) {
+		/* do not leak lockfd to hg */
+		close(opts->lockfd);
 		/* bypass uisetup() of pager extension */
 		int nullfd = open("/dev/null", O_WRONLY);
 		if (nullfd >= 0) {
@@ -287,8 +297,11 @@ 
 		}
 		execcmdserver(opts);
 	} else {
-		waitcmdserver(opts, pid);
+		hgc = retryconnectcmdserver(opts, pid);
 	}
+
+	unlockcmdserver(opts);
+	return hgc;
 }
 
 static void killcmdserver(const struct cmdserveropts *opts, int sig)
@@ -456,11 +469,7 @@ 
 		}
 	}
 
-	hgclient_t *hgc = hgc_open(opts.sockname);
-	if (!hgc) {
-		startcmdserver(&opts);
-		hgc = hgc_open(opts.sockname);
-	}
+	hgclient_t *hgc = connectcmdserver(&opts);
 	if (!hgc)
 		abortmsg("cannot open hg client");