More comments, and detection of fork-reentrancy. Much effort was spent

trying to come to a fork-safe solution, but ultimately we gave up.

9 files changed, 111 insertions, 35 deletions

diff --git a/src/core/pipe.c b/src/core/pipe.c
index 408964ab..40a810c8 100644
--- a/src/core/pipe.c
+++ b/src/core/pipe.c
@@ -63,6 +63,8 @@ nni_pipe_recv(nng_pipe_t p, nng_msg_t *msgp)
 void
 nni_pipe_close(nng_pipe_t p)
 {
+        /* XXX: we need to unregister from the parent socket. */
+        /* XXX: also unregister from the protocol. */
         return (p->p_ops.p_close(p->p_tran));
 }
 
diff --git a/src/core/platform.h b/src/core/platform.h
index e2d81139..f7af7c30 100644
--- a/src/core/platform.h
+++ b/src/core/platform.h
@@ -39,6 +39,23 @@
  */
 
 /*
+ * A word about fork-safety: This library is *NOT* fork safe, in that
+ * functions may not be called in the child process without an intervening
+ * exec().  The library attempts to detect this situation, and crashes the
+ * process with an error message if it encounters it.  (See nn_platform_init
+ * below.)
+ *
+ * Additionally, some file descriptors may leak across fork even to
+ * child processes.  We make every reasonable effort to ensure that this
+ * does not occur, but on some platforms there are unavoidable race
+ * conditions between file creation and marking the file close-on-exec.
+ *
+ * Forkers should use posix_spawn() if possible, and as much as possible
+ * arrange for file close on exec by posix_spawn, or close the descriptors
+ * they do not need in the child.
+ */
+
+/*
  * nni_abort crashes the system; it should do whatever is appropriate
  * for abnormal programs on the platform, such as calling abort().
  */
@@ -155,7 +172,7 @@ uint64_t nni_clock(void);
 void nni_usleep(uint64_t);
 
 /*
- * nni_init is called to allow the platform the chance to
+ * nni_platform_init is called to allow the platform the chance to
  * do any necessary initialization.  This routine MUST be idempotent,
  * and threadsafe, and will be called before any other API calls, and
  * may be called at any point thereafter.  It is permitted to return
@@ -170,6 +187,6 @@ int nni_platform_init(void);
  * be called as the last thing executed in the library, and no other functions
  * will be called until nni_platform_init is called.
  */
-void nni_fini(void);
+void nni_platform_fini(void);
 
 #endif /* CORE_PLATFORM_H */
diff --git a/src/core/protocol.h b/src/core/protocol.h
index ae97e896..f7b7b49b 100644
--- a/src/core/protocol.h
+++ b/src/core/protocol.h
@@ -25,7 +25,20 @@
 
 /*
  * Protocol implementation details.  Protocols must implement the
- * interfaces in this file.
+ * interfaces in this file.  Note that implementing new protocols is
+ * not necessarily intended to be a trivial task.  The protocol developer
+ * must understand the nature of nng, as they are responsible for handling
+ * most of the logic.  The protocol generally does most of the work for
+ * locking, and calls into the transport's pipe functions to do actual
+ * work, and the pipe functions generally assume no locking is needed.
+ * As a consequence, most of the concurrency in nng exists in the protocol
+ * implementations.
+ *
+ * A special exception to this is nni_pipe_close(), which actually does
+ * call back into the socket, which will then call the protocol's add
+ * pipe methods.  Its therefore important that no locks are held by the
+ * protocol during nni_pipe_close().  (Generally, its preferred that the
+ * protocol do not hold locks across calls to any pipe functions.)
  */
 
 struct nni_protocol {
diff --git a/src/core/socket.c b/src/core/socket.c
index dffc8a2d..5b93454d 100644
--- a/src/core/socket.c
+++ b/src/core/socket.c
@@ -43,8 +43,8 @@ struct nng_socket {
 };
 
 /*
- * nni_socket_sendq and nni_socket_recvq are called by the protocol
- * to obtain the upper read and write queues.
+ * nni_socket_sendq and nni_socket_recvq are called by the protocol to obtain
+ * the upper read and write queues.
  */
 nni_msgqueue_t
 nng_socket_sendq(nng_socket_t s)
@@ -112,8 +112,8 @@ nng_socket_sendmsg(nng_socket_t sock, nng_msg_t msg, int tmout)
 
 	if (besteffort) {
 		/*
-		 * BestEffort mode -- if we cannot handle the message due
-		 * to backpressure, we just throw it away, and don't complain.
+                 * BestEffort mode -- if we cannot handle the message due to
+                 * backpressure, we just throw it away, and don't complain.
 		 */
 		tmout = 0;
 	}
diff --git a/src/core/transport.c b/src/core/transport.c
index 1053cf16..c4ac9faf 100644
--- a/src/core/transport.c
+++ b/src/core/transport.c
@@ -49,14 +49,14 @@ nni_transport_init(void)
 }
 
 void
-nni_transport_fork(int prefork)
+nni_transport_fini(void)
 {
 	int i;
 	struct nni_transport *ops;
 
 	for (i = 0; (ops = transports[i]) != NULL; i++) {
-		if (ops->tran_fork != NULL) {
-			ops->tran_fork(prefork);
+		if (ops->tran_fini != NULL) {
+			ops->tran_fini();
 		}
 	}
 }
diff --git a/src/core/transport.h b/src/core/transport.h
index c79c12ad..71eb8a28 100644
--- a/src/core/transport.h
+++ b/src/core/transport.h
@@ -52,19 +52,16 @@ struct nni_transport {
 
 	/*
 	 * tran_fini, if not NULL, is called during library deinitialization.
-	 * It should release any global resources.
+	 * It should release any global resources, close any open files, etc.
+	 *
+	 * There will be no locks held, and no other threads running in the
+	 * library.
+	 *
+	 * It is invalid to use any mutexes, condition variables, or
+	 * threading routines.  Mutexes and condition variables may be
+	 * safely destroyed.
 	 */
 	void (*tran_fini)(void);
-
-	/*
-	 * tran_fork, if not NULL, is called just before fork
-	 * (e.g. during pthread_atfork() for the prefork phase),
-	 * and again just after returning in the parent.  There is
-	 * nothing called for the child.  If the transport does not
-	 * create any threads of its own, this can be NULL.  (The
-	 * intended use is to prevent O_CLOEXEC races.)
-	 */
-	void (*tran_fork)(int prefork);
 };
 
 /*
diff --git a/src/platform/posix/posix_synch.h b/src/platform/posix/posix_synch.h
index 94058d32..d3e44411 100644
--- a/src/platform/posix/posix_synch.h
+++ b/src/platform/posix/posix_synch.h
@@ -85,11 +85,6 @@ nni_mutex_destroy(nni_mutex_t m)
 	if (pthread_mutex_destroy(&m->mx) != 0) {
 		nni_panic("pthread_mutex_destroy failed");
 	}
-	/*
-	 * If destroy fails for some reason, we can't really do
-	 * anything about it.  This would actually represent a programming
-	 * bug, and the right thing to do here would be to panic.
-	 */
 	nni_free(m, sizeof (*m));
 }
 
@@ -183,7 +178,6 @@ nni_cond_create(nni_cond_t *cvp, nni_mutex_t mx)
 		nni_free(c, sizeof (*c));
 		return (NNG_ENOMEM);
 	}
-
 	*cvp = c;
 	return (0);
 }
@@ -232,12 +226,12 @@ nni_cond_timedwait(nni_cond_t c, uint64_t usec)
 	ts.tv_sec = usec / 1000000;
 	ts.tv_nsec = (usec % 10000) * 1000;
 
-	if ((rv = pthread_cond_timedwait(&c->cv, c->mx, &ts)) != 0) {
-		if (rv == ETIMEDOUT) {
-			return (NNG_ETIMEDOUT);
-		} else {
-			nni_panic("pthread_cond_timedwait returned %d", rv);
-		}
+	rv = pthread_cond_timedwait(&c->cv, c->mx, &ts);
+
+	if (rv == ETIMEDOUT) {
+		return (NNG_ETIMEDOUT);
+	} else if (rv != 0) {
+		nni_panic("pthread_cond_timedwait returned %d", rv);
 	}
 	return (0);
 }
diff --git a/src/platform/posix/posix_thread.h b/src/platform/posix/posix_thread.h
index 239959f8..27ad888f 100644
--- a/src/platform/posix/posix_thread.h
+++ b/src/platform/posix/posix_thread.h
@@ -37,8 +37,23 @@
 
 struct nni_thread {
 	pthread_t	tid;
+	void *arg;
+	void (*func)(void *);
 };
 
+static pthread_mutex_t plat_lock = PTHREAD_MUTEX_INITIALIZER;
+static int plat_init = 0;
+static int plat_fork = 0;
+
+static void *
+thrfunc(void *arg)
+{
+	nni_thread_t thr = arg;
+
+	thr->func(thr->arg);
+	return (NULL);
+}
+
 int
 nni_thread_create(nni_thread_t *tp, void (*fn)(void *), void *arg)
 {
@@ -48,7 +63,10 @@ nni_thread_create(nni_thread_t *tp, void (*fn)(void *), void *arg)
 	if ((thr = nni_alloc(sizeof (*thr))) == NULL) {
 		return (NNG_ENOMEM);
 	}
-	if ((rv = pthread_create(&thr->tid, NULL, (void *)fn, arg)) != 0) {
+	thr->func = fn;
+	thr->arg = arg;
+
+	if ((rv = pthread_create(&thr->tid, thr, thrfunc, arg)) != 0) {
 		nni_free(thr, sizeof (*thr));
 		return (NNG_ENOMEM);
 	}
@@ -65,3 +83,39 @@ nni_thread_reap(nni_thread_t thr)
 	}
 	nni_free(thr, sizeof (*thr));
 }
+
+void
+atfork_child(void)
+{
+	plat_fork = 1;
+}
+
+int
+nni_platform_init(void)
+{
+	if (plat_fork) {
+		nni_panic("nng is fork-reentrant safe");
+	}
+	if (plat_init) {
+		return (0);	/* fast path */
+	}
+	pthread_mutex_lock(&plat_lock);
+	if (plat_init) {	/* check again under the lock to be sure */
+		pthread_mutex_unlock(&plat_lock);
+		return (0);
+	}
+	if (pthread_atfork(NULL, NULL, atfork_child) != 0) {
+		pthread_mutex_unlock(&plat_lock);
+		return (NNG_ENOMEM);
+	}
+	plat_init = 1;
+	pthread_mutex_unlock(&plat_lock);
+
+	return (0);
+}
+
+void
+nni_platform_fini(void)
+{
+	/* XXX: NOTHING *YET* */
+}
\ No newline at end of file
diff --git a/src/transport/inproc/inproc.c b/src/transport/inproc/inproc.c
index 810641bb..e6e72b82 100644
--- a/src/transport/inproc/inproc.c
+++ b/src/transport/inproc/inproc.c
@@ -393,5 +393,4 @@ struct nni_transport nni_inproc_transport = {
 	&inproc_pipe_ops,
 	inproc_init,		/* tran_init */
 	inproc_fini,		/* tran_fini */
-	NULL,			/* tran_fork */
 };