fixes #591 incorrect reuse of server instances by websocket

This also arranges for server shutdown to be handled using
the reaper, leading to more elegant cleanup.

8 files changed, 52 insertions, 98 deletions

diff --git a/docs/man/nng_http_server_stop.3http.adoc b/docs/man/nng_http_server_stop.3http.adoc
index bf45adc9..8082409c 100644
--- a/docs/man/nng_http_server_stop.3http.adoc
+++ b/docs/man/nng_http_server_stop.3http.adoc
@@ -25,9 +25,23 @@ void nng_http_server_stop(nng_http_server *server);
 
 == DESCRIPTION
 
-The `nng_http_server_stop()` stops the HTTP server instance _server_.
-This will cause it to close any underlying TCP sockets, and to terminate
-any HTTP connections associated with it.
+The `nng_http_server_stop()` undoes the effect of
+`<<nng_http_server_start.3http#,nng_http_server_start()>>`.
+
+Each call by
+`<<nng_http_server_start.3http#,nng_http_server_start()>>` acts as reference
+count, and should be matched by a call to `nng_http_server_stop()`.
+When the reference count drops to zero, then the server is actually stopped,
+and existing open connections to it are closed.
+
+This function does not wait for the connections to close.
+
+NOTE: Once the server instance is actually stopped, it cannot be started again,
+and any future calls to `<<nng_http_server_hold.3http#,nng_http_server_hold()>>`
+will return a new instance of the server.
+It is expected that the caller will follow this function call with a call to
+`<<nng_http_server_release.3http#,nng_http_server_release()>>`.
+
 
 == RETURN VALUES
 
@@ -41,5 +55,6 @@ None.
 
 [.text-left]
 <<nng_http_server_hold.3http#,nng_http_server_hold(3http)>>,
+<<nng_http_server_release.3http#,nng_http_server_release(3http)>>,
 <<nng_http_server_start.3http#,nng_http_server_start(3http)>>,
 <<nng.7#,nng(7)>>
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index e89f782b..11c7d82e 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -103,7 +103,6 @@ if (NNG_PLATFORM_POSIX)
         platform/posix/posix_rand.c
         platform/posix/posix_resolv_gai.c
         platform/posix/posix_sockaddr.c
-        platform/posix/posix_tcp.c
         platform/posix/posix_tcpconn.c
         platform/posix/posix_tcpdial.c
         platform/posix/posix_tcplisten.c
diff --git a/src/platform/posix/posix_tcp.c b/src/platform/posix/posix_tcp.c
deleted file mode 100644
index 53ea8026..00000000
--- a/src/platform/posix/posix_tcp.c
+++ /dev/null
@@ -1,69 +0,0 @@
-//
-// Copyright 2018 Staysail Systems, Inc. <info@staysail.tech>
-// Copyright 2018 Capitar IT Group BV <info@capitar.com>
-//
-// This software is supplied under the terms of the MIT License, a
-// copy of which should be located in the distribution where this
-// file was obtained (LICENSE.txt).  A copy of the license may also be
-// found online at https://opensource.org/licenses/MIT.
-//
-
-#include "core/nng_impl.h"
-
-#ifdef NNG_PLATFORM_POSIX
-#include "platform/posix/posix_aio.h"
-
-#include <arpa/inet.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <netinet/in.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/socket.h>
-#include <sys/types.h>
-#include <sys/uio.h>
-#include <unistd.h>
-
-int
-nni_plat_tcp_ntop(const nni_sockaddr *sa, char *ipstr, char *portstr)
-{
-	const void *ap;
-	uint16_t    port;
-	int         af;
-	switch (sa->s_family) {
-	case NNG_AF_INET:
-		ap   = &sa->s_in.sa_addr;
-		port = sa->s_in.sa_port;
-		af   = AF_INET;
-		break;
-	case NNG_AF_INET6:
-		ap   = &sa->s_in6.sa_addr;
-		port = sa->s_in6.sa_port;
-		af   = AF_INET6;
-		break;
-	default:
-		return (NNG_EINVAL);
-	}
-	if (ipstr != NULL) {
-		if (af == AF_INET6) {
-			size_t l;
-			ipstr[0] = '[';
-			inet_ntop(af, ap, ipstr + 1, INET6_ADDRSTRLEN);
-			l          = strlen(ipstr);
-			ipstr[l++] = ']';
-			ipstr[l++] = '\0';
-		} else {
-			inet_ntop(af, ap, ipstr, INET6_ADDRSTRLEN);
-		}
-	}
-	if (portstr != NULL) {
-#ifdef NNG_LITTLE_ENDIAN
-		port = ((port >> 8) & 0xff) | ((port & 0xff) << 8);
-#endif
-		snprintf(portstr, 6, "%u", port);
-	}
-	return (0);
-}
-
-#endif // NNG_PLATFORM_POSIX
diff --git a/src/platform/posix/posix_tcpconn.c b/src/platform/posix/posix_tcpconn.c
index 30525648..5cd1887a 100644
--- a/src/platform/posix/posix_tcpconn.c
+++ b/src/platform/posix/posix_tcpconn.c
@@ -401,7 +401,9 @@ nni_tcp_conn_fini(nni_tcp_conn *c)
 {
 	nni_tcp_conn_close(c);
 	nni_posix_pfd_fini(c->pfd);
+	nni_mtx_lock(&c->mtx); // not strictly needed, but shut up TSAN
 	c->pfd = NULL;
+	nni_mtx_unlock(&c->mtx);
 	nni_mtx_fini(&c->mtx);
 
 	NNI_FREE_STRUCT(c);
diff --git a/src/platform/windows/win_resolv.c b/src/platform/windows/win_resolv.c
index e2cd192e..916f54b1 100644
--- a/src/platform/windows/win_resolv.c
+++ b/src/platform/windows/win_resolv.c
@@ -11,6 +11,7 @@
 #include "core/nng_impl.h"
 
 #include <ctype.h>
+#include <stdio.h>
 #include <string.h>
 
 #ifdef NNG_PLATFORM_WINDOWS
diff --git a/src/platform/windows/win_sockaddr.c b/src/platform/windows/win_sockaddr.c
index 141a1ce5..fbb960b7 100644
--- a/src/platform/windows/win_sockaddr.c
+++ b/src/platform/windows/win_sockaddr.c
@@ -70,4 +70,4 @@ nni_win_sockaddr2nn(nni_sockaddr *sa, const SOCKADDR_STORAGE *ss)
 	return (-1);
 }
 
-#endif // NNG_PLATFORM_WINDOWS
\ No newline at end of file
+#endif // NNG_PLATFORM_WINDOWS
diff --git a/src/supplemental/http/http.h b/src/supplemental/http/http.h
index 21e6b5de..1991da1b 100644
--- a/src/supplemental/http/http.h
+++ b/src/supplemental/http/http.h
@@ -375,7 +375,9 @@ NNG_DECL void nng_http_server_release(nng_http_server *);
 NNG_DECL int nng_http_server_start(nng_http_server *);
 
 // nng_http_server_stop stops the server.  No new client connections are
-// accepted after this returns.
+// accepted after this returns.  Once a server is stopped fully, the
+// instance will no longer be returned by nng_http_server_hold, as the
+// server may not be reused.
 NNG_DECL void nng_http_server_stop(nng_http_server *);
 
 // nng_http_server_add_handler registers a handler on the server.
diff --git a/src/supplemental/http/http_server.c b/src/supplemental/http/http_server.c
index 9d1313b1..a72817b3 100644
--- a/src/supplemental/http/http_server.c
+++ b/src/supplemental/http/http_server.c
@@ -71,7 +71,6 @@ struct nng_http_server {
 	nni_list          handlers;
 	nni_list          conns;
 	nni_mtx           mtx;
-	nni_cv            cv;
 	bool              closed;
 	nng_tls_config *  tls;
 	nni_aio *         accaio;
@@ -80,6 +79,7 @@ struct nng_http_server {
 	char *            hostname;
 	nni_list          errors;
 	nni_mtx           errors_mtx;
+	nni_reap_item     reap;
 };
 
 int
@@ -232,9 +232,6 @@ http_sconn_reap(void *arg)
 	nni_mtx_lock(&s->mtx);
 	if (nni_list_node_active(&sc->node)) {
 		nni_list_remove(&s->conns, sc);
-		if (nni_list_empty(&s->conns)) {
-			nni_cv_wake(&s->cv);
-		}
 	}
 	nni_mtx_unlock(&s->mtx);
 
@@ -766,8 +763,12 @@ http_server_fini(nni_http_server *s)
 	nni_aio_stop(s->accaio);
 
 	nni_mtx_lock(&s->mtx);
-	while (!nni_list_empty(&s->conns)) {
-		nni_cv_wait(&s->cv);
+	if (!nni_list_empty(&s->conns)) {
+		// Try to reap later, after the sconns are done reaping.
+		// (Note, sconns will all have been closed already.)
+		nni_reap(&s->reap, (nni_cb) http_server_fini, s);
+		nni_mtx_unlock(&s->mtx);
+		return;
 	}
 	if (s->listener != NULL) {
 		nni_tcp_listener_fini(s->listener);
@@ -793,25 +794,12 @@ http_server_fini(nni_http_server *s)
 	nni_mtx_fini(&s->errors_mtx);
 
 	nni_aio_fini(s->accaio);
-	nni_cv_fini(&s->cv);
 	nni_mtx_fini(&s->mtx);
 	nni_strfree(s->hostname);
 	nni_strfree(s->port);
 	NNI_FREE_STRUCT(s);
 }
 
-void
-nni_http_server_fini(nni_http_server *s)
-{
-	nni_mtx_lock(&http_servers_lk);
-	s->refcnt--;
-	if (s->refcnt == 0) {
-		nni_list_remove(&http_servers, s);
-		http_server_fini(s);
-	}
-	nni_mtx_unlock(&http_servers_lk);
-}
-
 static int
 http_server_init(nni_http_server **serverp, const nni_url *url)
 {
@@ -831,7 +819,6 @@ http_server_init(nni_http_server **serverp, const nni_url *url)
 		return (NNG_ENOMEM);
 	}
 	nni_mtx_init(&s->mtx);
-	nni_cv_init(&s->cv, &s->mtx);
 	nni_mtx_init(&s->errors_mtx);
 	NNI_LIST_INIT(&s->handlers, nni_http_handler, node);
 	NNI_LIST_INIT(&s->conns, http_sconn, node);
@@ -898,7 +885,7 @@ nni_http_server_init(nni_http_server **serverp, const nni_url *url)
 
 	nni_mtx_lock(&http_servers_lk);
 	NNI_LIST_FOREACH (&http_servers, s) {
-		if ((strcmp(url->u_port, s->port) == 0) &&
+		if ((!s->closed) && (strcmp(url->u_port, s->port) == 0) &&
 		    (strcmp(url->u_hostname, s->hostname) == 0)) {
 			*serverp = s;
 			s->refcnt++;
@@ -957,8 +944,10 @@ http_server_stop(nni_http_server *s)
 	if (s->closed) {
 		return;
 	}
-
 	s->closed = true;
+
+	nni_aio_close(s->accaio);
+
 	// Close the TCP endpoint that is listening.
 	if (s->listener) {
 		nni_tcp_listener_close(s->listener);
@@ -969,7 +958,6 @@ http_server_stop(nni_http_server *s)
 	NNI_LIST_FOREACH (&s->conns, sc) {
 		http_sconn_close_locked(sc);
 	}
-	nni_cv_wake(&s->cv);
 }
 
 void
@@ -1660,6 +1648,21 @@ nni_http_server_get_tls(nni_http_server *s, nng_tls_config **tp)
 #endif
 }
 
+void
+nni_http_server_fini(nni_http_server *s)
+{
+	nni_mtx_lock(&http_servers_lk);
+	s->refcnt--;
+	if (s->refcnt == 0) {
+		nni_mtx_lock(&s->mtx);
+		http_server_stop(s);
+		nni_mtx_unlock(&s->mtx);
+		nni_list_remove(&http_servers, s);
+		nni_reap(&s->reap, (nni_cb) http_server_fini, s);
+	}
+	nni_mtx_unlock(&http_servers_lk);
+}
+
 static int
 http_server_sys_init(void)
 {
@@ -1671,5 +1674,6 @@ http_server_sys_init(void)
 static void
 http_server_sys_fini(void)
 {
+	nni_reap_drain();
 	nni_mtx_fini(&http_servers_lk);
 }