fixes #1064 Potential deadlock in statistics code

fixes #1063 Include sanitizer runs in CI fixes #1068 Wssfile test sometimes fails with wrong error code While here, addressed a number of clang-tidy items, and some light cleanup of code we were already in.
nanomsg · Dec 29, 2019 · 7d3bac8 · 7d3bac8
1 parent e457590
commit 7d3bac8
Show file tree

Hide file tree

Showing 11 changed files with 177 additions and 146 deletions.
diff --git a/.github/workflows/sanitizer.yml b/.github/workflows/sanitizer.yml
@@ -0,0 +1,37 @@
+name: coverage
+on: [push]
+jobs:
+  sanitize:
+
+    strategy:
+      matrix:
+        sanitizer: [ address, undefined, thread ]
+        os: [ ubuntu-latest ]
+
+    steps:
+      - uses: actions/checkout@v1
+
+      - name: Install Dependencies (Linux)
+        run: sudo apt-get install ninja-build libmbedtls-dev
+        if: runner.os == Linux
+
+      - name: Install Dependencies (macOS)
+        run: brew install ninja mbedtls
+        if: runner.os == macOS
+
+      - name: Configure
+        run: |
+          mkdir build
+          cd build
+          cmake -G Ninja -DNNG_SANITIZER=${{ matrix.sanitizer }} -DNNG_ENABLE_TLS=ON -DNNG_TOOLS=OFF ..
+        env:
+          CC: clang
+          CXX: clang++
+      - name: Build
+        run: |
+          cd build
+          ninja
+      - name: Test
+        run: |
+          cd build
+          ninja test
diff --git a/src/core/dialer.c b/src/core/dialer.c
@@ -13,7 +13,6 @@
 #include "sockimpl.h"
 
 #include <stdio.h>
-#include <stdlib.h>
 #include <string.h>
 
 // Functionality related to dialers.
@@ -24,7 +23,7 @@ static void dialer_timer_cb(void *);
 static nni_idhash *dialers;
 static nni_mtx     dialers_lk;
 
-#define BUMPSTAT(x) nni_stat_inc_atomic(x, 1)
+#define BUMP_STAT(x) nni_stat_inc_atomic(x, 1)
 
 int
 nni_dialer_sys_init(void)
@@ -81,49 +80,49 @@ dialer_stats_init(nni_dialer *d)
 	nni_stat_init_scope(root, st->s_scope, "dialer statistics");
 
 	nni_stat_init_id(&st->s_id, "id", "dialer id", d->d_id);
-	nni_stat_append(root, &st->s_id);
+	nni_stat_add(root, &st->s_id);
 
 	nni_stat_init_id(&st->s_sock, "socket", "socket for dialer",
 	    nni_sock_id(d->d_sock));
-	nni_stat_append(root, &st->s_sock);
+	nni_stat_add(root, &st->s_sock);
 
 	nni_stat_init_string(
 	    &st->s_url, "url", "dialer url", d->d_url->u_rawurl);
-	nni_stat_append(root, &st->s_url);
+	nni_stat_add(root, &st->s_url);
 
 	nni_stat_init_atomic(&st->s_npipes, "npipes", "open pipes");
-	nni_stat_append(root, &st->s_npipes);
+	nni_stat_add(root, &st->s_npipes);
 
 	nni_stat_init_atomic(
 	    &st->s_connok, "connect", "connections established");
-	nni_stat_append(root, &st->s_connok);
+	nni_stat_add(root, &st->s_connok);
 
 	nni_stat_init_atomic(&st->s_refused, "refused", "connections refused");
-	nni_stat_append(root, &st->s_refused);
+	nni_stat_add(root, &st->s_refused);
 
 	nni_stat_init_atomic(&st->s_discon, "discon", "remote disconnects");
-	nni_stat_append(root, &st->s_discon);
+	nni_stat_add(root, &st->s_discon);
 
 	nni_stat_init_atomic(&st->s_canceled, "canceled", "canceled");
-	nni_stat_append(root, &st->s_canceled);
+	nni_stat_add(root, &st->s_canceled);
 
 	nni_stat_init_atomic(&st->s_othererr, "othererr", "other errors");
-	nni_stat_append(root, &st->s_othererr);
+	nni_stat_add(root, &st->s_othererr);
 
 	nni_stat_init_atomic(&st->s_etimedout, "timedout", "timed out");
-	nni_stat_append(root, &st->s_etimedout);
+	nni_stat_add(root, &st->s_etimedout);
 
 	nni_stat_init_atomic(&st->s_eproto, "protoerr", "protcol errors");
-	nni_stat_append(root, &st->s_eproto);
+	nni_stat_add(root, &st->s_eproto);
 
 	nni_stat_init_atomic(&st->s_eauth, "autherr", "auth errors");
-	nni_stat_append(root, &st->s_eauth);
+	nni_stat_add(root, &st->s_eauth);
 
 	nni_stat_init_atomic(&st->s_enomem, "nomem", "out of memory");
-	nni_stat_append(root, &st->s_enomem);
+	nni_stat_add(root, &st->s_enomem);
 
 	nni_stat_init_atomic(&st->s_reject, "reject", "pipes rejected");
-	nni_stat_append(root, &st->s_reject);
+	nni_stat_add(root, &st->s_reject);
 }
 
 void
@@ -132,29 +131,29 @@ nni_dialer_bump_error(nni_dialer *d, int err)
 	switch (err) {
 	case NNG_ECONNABORTED:
 	case NNG_ECONNRESET:
-		BUMPSTAT(&d->d_stats.s_discon);
+		BUMP_STAT(&d->d_stats.s_discon);
 		break;
 	case NNG_ECONNREFUSED:
-		BUMPSTAT(&d->d_stats.s_refused);
+		BUMP_STAT(&d->d_stats.s_refused);
 		break;
 	case NNG_ECANCELED:
-		BUMPSTAT(&d->d_stats.s_canceled);
+		BUMP_STAT(&d->d_stats.s_canceled);
 		break;
 	case NNG_ETIMEDOUT:
-		BUMPSTAT(&d->d_stats.s_etimedout);
+		BUMP_STAT(&d->d_stats.s_etimedout);
 		break;
 	case NNG_EPROTO:
-		BUMPSTAT(&d->d_stats.s_eproto);
+		BUMP_STAT(&d->d_stats.s_eproto);
 		break;
 	case NNG_EPEERAUTH:
 	case NNG_ECRYPTO:
-		BUMPSTAT(&d->d_stats.s_eauth);
+		BUMP_STAT(&d->d_stats.s_eauth);
 		break;
 	case NNG_ENOMEM:
-		BUMPSTAT(&d->d_stats.s_enomem);
+		BUMP_STAT(&d->d_stats.s_enomem);
 		break;
 	default:
-		BUMPSTAT(&d->d_stats.s_othererr);
+		BUMP_STAT(&d->d_stats.s_othererr);
 		break;
 	}
 }
@@ -212,7 +211,7 @@ nni_dialer_create(nni_dialer **dp, nni_sock *s, const char *urlstr)
 	snprintf(d->d_stats.s_scope, sizeof(d->d_stats.s_scope), "dialer%u",
 	    d->d_id);
 	nni_stat_set_value(&d->d_stats.s_id, d->d_id);
-	nni_stat_append(NULL, &d->d_stats.s_root);
+	nni_stat_register(&d->d_stats.s_root);
 	*dp = d;
 	return (0);
 }
@@ -261,7 +260,6 @@ nni_dialer_rele(nni_dialer *d)
 	nni_mtx_lock(&dialers_lk);
 	d->d_refcnt--;
 	if ((d->d_refcnt == 0) && (d->d_closed)) {
-		nni_stat_remove(&d->d_stats.s_root);
 		nni_reap(&d->d_reap, (nni_cb) nni_dialer_reap, d);
 	}
 	nni_mtx_unlock(&dialers_lk);
@@ -327,48 +325,34 @@ dialer_connect_cb(void *arg)
 {
 	nni_dialer *d   = arg;
 	nni_aio *   aio = d->d_con_aio;
-	nni_aio *   uaio;
+	nni_aio *   user_aio;
 	int         rv;
 
 	nni_mtx_lock(&d->d_mtx);
-	uaio          = d->d_user_aio;
+	user_aio      = d->d_user_aio;
 	d->d_user_aio = NULL;
 	nni_mtx_unlock(&d->d_mtx);
 
 	switch ((rv = nni_aio_result(aio))) {
 	case 0:
-		BUMPSTAT(&d->d_stats.s_connok);
+		BUMP_STAT(&d->d_stats.s_connok);
 		nni_dialer_add_pipe(d, nni_aio_get_output(aio, 0));
 		break;
 	case NNG_ECLOSED:   // No further action.
 	case NNG_ECANCELED: // No further action.
 		break;
 	case NNG_ECONNREFUSED:
-		if (uaio == NULL) {
-			nni_dialer_timer_start(d);
-		} else {
-			nni_atomic_flag_reset(&d->d_started);
-		}
-		break;
-
 	case NNG_ETIMEDOUT:
-		if (uaio == NULL) {
-			nni_dialer_timer_start(d);
-		} else {
-			nni_atomic_flag_reset(&d->d_started);
-		}
-		break;
-
 	default:
-		if (uaio == NULL) {
+		if (user_aio == NULL) {
 			nni_dialer_timer_start(d);
 		} else {
 			nni_atomic_flag_reset(&d->d_started);
 		}
 		break;
 	}
-	if (uaio != NULL) {
-		nni_aio_finish(uaio, rv, 0);
+	if (user_aio != NULL) {
+		nni_aio_finish(user_aio, rv, 0);
 	}
 }
 
@@ -518,5 +502,5 @@ nni_dialer_getopt(
 void
 nni_dialer_add_stat(nni_dialer *d, nni_stat_item *stat)
 {
-	nni_stat_append(&d->d_stats.s_root, stat);
+	nni_stat_add(&d->d_stats.s_root, stat);
 }
diff --git a/src/core/list.h b/src/core/list.h
@@ -30,9 +30,9 @@ extern void nni_list_init_offset(nni_list *list, size_t offset);
 	nni_list_init_offset(list, offsetof(type, field))
 
 #define NNI_LIST_NODE_INIT(node)                       \
-	{                                              \
+	do {                                           \
 		(node)->ln_prev = (node)->ln_next = 0; \
-	}
+	} while (0)
 
 extern void *nni_list_first(const nni_list *);
 extern void *nni_list_last(const nni_list *);