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

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

6 files changed, 50 insertions, 21 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);
 };
 
 /*