From 7d3bac8066b3d83f1bc7ce0ebb9eb80cac0c108f Mon Sep 17 00:00:00 2001 From: Garrett D'Amore Date: Sun, 29 Dec 2019 14:39:27 -0800 Subject: [PATCH] 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. --- .github/workflows/sanitizer.yml | 37 +++++++++++ src/core/dialer.c | 78 +++++++++------------- src/core/list.h | 4 +- src/core/listener.c | 81 +++++++++++------------ src/core/pipe.c | 20 +++--- src/core/socket.c | 52 +++++++-------- src/core/stats.c | 20 +++--- src/core/stats.h | 12 +++- src/protocol/pair1/pair.c | 6 +- src/supplemental/websocket/websocket.h | 2 +- src/supplemental/websocket/wssfile_test.c | 11 ++- 11 files changed, 177 insertions(+), 146 deletions(-) create mode 100644 .github/workflows/sanitizer.yml diff --git a/.github/workflows/sanitizer.yml b/.github/workflows/sanitizer.yml new file mode 100644 index 000000000..15fe96d00 --- /dev/null +++ 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 \ No newline at end of file diff --git a/src/core/dialer.c b/src/core/dialer.c index ffbef8a79..91d8082b6 100644 --- a/src/core/dialer.c +++ b/src/core/dialer.c @@ -13,7 +13,6 @@ #include "sockimpl.h" #include -#include #include // 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 index b0007ec0e..204057a2c 100644 --- 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 *); diff --git a/src/core/listener.c b/src/core/listener.c index 58758cf3d..189f89056 100644 --- a/src/core/listener.c +++ b/src/core/listener.c @@ -13,7 +13,6 @@ #include "sockimpl.h" #include -#include #include // Functionality related to listeners. @@ -25,7 +24,7 @@ static void listener_timer_cb(void *); static nni_idhash *listeners; static nni_mtx listeners_lk; -#define BUMPSTAT(x) nni_stat_inc_atomic(x, 1) +#define BUMP_STAT(x) nni_stat_inc_atomic(x, 1) int nni_listener_sys_init(void) @@ -82,45 +81,45 @@ listener_stats_init(nni_listener *l) // NB: This will be updated later. nni_stat_init_id(&st->s_id, "id", "listener id", l->l_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 listener", nni_sock_id(l->l_sock)); - nni_stat_append(root, &st->s_sock); + nni_stat_add(root, &st->s_sock); nni_stat_init_string( &st->s_url, "url", "listener url", l->l_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_accept, "accept", "connections accepted"); - nni_stat_append(root, &st->s_accept); + nni_stat_add(root, &st->s_accept); 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 @@ -129,39 +128,39 @@ nni_listener_bump_error(nni_listener *l, int err) switch (err) { case NNG_ECONNABORTED: case NNG_ECONNRESET: - BUMPSTAT(&l->l_stats.s_discon); + BUMP_STAT(&l->l_stats.s_discon); break; case NNG_ECANCELED: - BUMPSTAT(&l->l_stats.s_canceled); + BUMP_STAT(&l->l_stats.s_canceled); break; case NNG_ETIMEDOUT: - BUMPSTAT(&l->l_stats.s_etimedout); + BUMP_STAT(&l->l_stats.s_etimedout); break; case NNG_EPROTO: - BUMPSTAT(&l->l_stats.s_eproto); + BUMP_STAT(&l->l_stats.s_eproto); break; case NNG_EPEERAUTH: case NNG_ECRYPTO: - BUMPSTAT(&l->l_stats.s_eauth); + BUMP_STAT(&l->l_stats.s_eauth); break; case NNG_ENOMEM: - BUMPSTAT(&l->l_stats.s_enomem); + BUMP_STAT(&l->l_stats.s_enomem); break; default: - BUMPSTAT(&l->l_stats.s_othererr); + BUMP_STAT(&l->l_stats.s_othererr); break; } } int -nni_listener_create(nni_listener **lp, nni_sock *s, const char *urlstr) +nni_listener_create(nni_listener **lp, nni_sock *s, const char *url_str) { nni_tran * tran; nni_listener *l; int rv; nni_url * url; - if ((rv = nni_url_parse(&url, urlstr)) != 0) { + if ((rv = nni_url_parse(&url, url_str)) != 0) { return (rv); } if (((tran = nni_tran_find(url)) == NULL) || @@ -205,7 +204,7 @@ nni_listener_create(nni_listener **lp, nni_sock *s, const char *urlstr) snprintf(l->l_stats.s_scope, sizeof(l->l_stats.s_scope), "listener%u", l->l_id); nni_stat_set_value(&l->l_stats.s_id, l->l_id); - nni_stat_append(NULL, &l->l_stats.s_root); + nni_stat_register(&l->l_stats.s_root); *lp = l; return (0); @@ -255,7 +254,6 @@ nni_listener_rele(nni_listener *l) nni_mtx_lock(&listeners_lk); l->l_refcnt--; if ((l->l_refcnt == 0) && (l->l_closed)) { - nni_stat_remove(&l->l_stats.s_root); nni_reap(&l->l_reap, (nni_cb) nni_listener_reap, l); } nni_mtx_unlock(&listeners_lk); @@ -324,30 +322,25 @@ listener_accept_cb(void *arg) switch (nni_aio_result(aio)) { case 0: - BUMPSTAT(&l->l_stats.s_accept); + BUMP_STAT(&l->l_stats.s_accept); nni_listener_add_pipe(l, nni_aio_get_output(aio, 0)); listener_accept_start(l); break; - case NNG_ECONNABORTED: // remote condition, no cooldown - case NNG_ECONNRESET: // remote condition, no cooldown - listener_accept_start(l); - break; - case NNG_ETIMEDOUT: - // No need to sleep since we timed out already. - listener_accept_start(l); - break; - case NNG_EPEERAUTH: // peer validation failure + case NNG_ECONNABORTED: // remote condition, no cool down + case NNG_ECONNRESET: // remote condition, no cool down + case NNG_ETIMEDOUT: // No need to sleep, we timed out already. + case NNG_EPEERAUTH: // peer validation failure listener_accept_start(l); break; case NNG_ECLOSED: // no further action case NNG_ECANCELED: // no further action break; default: - // We don't really know why we failed, but we backoff + // We don't really know why we failed, but we back off // here. This is because errors here are probably due // to system failures (resource exhaustion) and we hope // by not thrashing we give the system a chance to - // recover. 100 msec is enough to cool down. + // recover. 100 ms is enough to cool down. nni_sleep_aio(100, l->l_tmo_aio); break; } @@ -421,12 +414,12 @@ nni_listener_setopt( int nni_listener_getopt( - nni_listener *l, const char *name, void *valp, size_t *szp, nni_type t) + nni_listener *l, const char *name, void *val, size_t *szp, nni_type t) { nni_option *o; if (l->l_ops.l_getopt != NULL) { - int rv = l->l_ops.l_getopt(l->l_data, name, valp, szp, t); + int rv = l->l_ops.l_getopt(l->l_data, name, val, szp, t); if (rv != NNG_ENOTSUP) { return (rv); } @@ -439,21 +432,21 @@ nni_listener_getopt( if (o->o_get == NULL) { return (NNG_EWRITEONLY); } - return (o->o_get(l->l_data, valp, szp, t)); + return (o->o_get(l->l_data, val, szp, t)); } // We provide a fallback on the URL, but let the implementation // override. This allows the URL to be created with wildcards, // that are resolved later. if (strcmp(name, NNG_OPT_URL) == 0) { - return (nni_copyout_str(l->l_url->u_rawurl, valp, szp, t)); + return (nni_copyout_str(l->l_url->u_rawurl, val, szp, t)); } - return (nni_sock_getopt(l->l_sock, name, valp, szp, t)); + return (nni_sock_getopt(l->l_sock, name, val, szp, t)); } void nni_listener_add_stat(nni_listener *l, nni_stat_item *stat) { - nni_stat_append(&l->l_stats.s_root, stat); + nni_stat_add(&l->l_stats.s_root, stat); } diff --git a/src/core/pipe.c b/src/core/pipe.c index 44957defd..7a632b0f6 100644 --- a/src/core/pipe.c +++ b/src/core/pipe.c @@ -40,7 +40,7 @@ nni_pipe_sys_init(void) // value "1" has a bias -- its roughly twice as likely to be // chosen as any other value. This does not mater.) nni_idhash_set_limits( - nni_pipes, 1, 0x7fffffff, nni_random() & 0x7fffffff); + nni_pipes, 1, 0x7fffffff, nni_random() & 0x7fffffffu); return (0); } @@ -84,7 +84,7 @@ pipe_destroy(nni_pipe *p) p->p_tran_ops.p_stop(p->p_tran_data); } - nni_stat_remove(&p->p_stats.s_root); + nni_stat_unregister(&p->p_stats.s_root); nni_pipe_remove(p); if (p->p_proto_data != NULL) { @@ -223,23 +223,23 @@ pipe_create(nni_pipe **pp, nni_sock *sock, nni_tran *tran, void *tdata) nni_stat_init_scope(&st->s_root, st->s_scope, "pipe statistics"); nni_stat_init_id(&st->s_id, "id", "pipe id", p->p_id); - nni_stat_append(&st->s_root, &st->s_id); + nni_stat_add(&st->s_root, &st->s_id); nni_stat_init_id(&st->s_sock_id, "socket", "socket for pipe", nni_sock_id(p->p_sock)); - nni_stat_append(&st->s_root, &st->s_sock_id); + nni_stat_add(&st->s_root, &st->s_sock_id); nni_stat_init_atomic(&st->s_rxmsgs, "rxmsgs", "messages received"); nni_stat_set_unit(&st->s_rxmsgs, NNG_UNIT_MESSAGES); - nni_stat_append(&st->s_root, &st->s_rxmsgs); + nni_stat_add(&st->s_root, &st->s_rxmsgs); nni_stat_init_atomic(&st->s_txmsgs, "txmsgs", "messages sent"); nni_stat_set_unit(&st->s_txmsgs, NNG_UNIT_MESSAGES); - nni_stat_append(&st->s_root, &st->s_txmsgs); + nni_stat_add(&st->s_root, &st->s_txmsgs); nni_stat_init_atomic(&st->s_rxbytes, "rxbytes", "bytes received"); nni_stat_set_unit(&st->s_rxbytes, NNG_UNIT_BYTES); - nni_stat_append(&st->s_root, &st->s_rxbytes); + nni_stat_add(&st->s_root, &st->s_rxbytes); nni_stat_init_atomic(&st->s_txbytes, "txbytes", "bytes sent"); nni_stat_set_unit(&st->s_txbytes, NNG_UNIT_BYTES); - nni_stat_append(&st->s_root, &st->s_txbytes); + nni_stat_add(&st->s_root, &st->s_txbytes); if ((rv != 0) || ((rv = p->p_tran_ops.p_init(tdata, p)) != 0) || ((rv = pops->pipe_init(&p->p_proto_data, p, sdata)) != 0)) { @@ -270,7 +270,6 @@ nni_pipe_create_dialer(nni_pipe **pp, nni_dialer *d, void *tdata) p->p_dialer = d; nni_stat_init_id(st, "dialer", "dialer for pipe", id); nni_pipe_add_stat(p, st); - nni_stat_append(NULL, &p->p_stats.s_root); *pp = p; return (0); } @@ -293,7 +292,6 @@ nni_pipe_create_listener(nni_pipe **pp, nni_listener *l, void *tdata) p->p_listener = l; nni_stat_init_id(st, "listener", "listener for pipe", id); nni_pipe_add_stat(p, st); - nni_stat_append(NULL, &p->p_stats.s_root); *pp = p; return (0); } @@ -347,7 +345,7 @@ nni_pipe_dialer_id(nni_pipe *p) void nni_pipe_add_stat(nni_pipe *p, nni_stat_item *item) { - nni_stat_append(&p->p_stats.s_root, item); + nni_stat_add(&p->p_stats.s_root, item); } void diff --git a/src/core/socket.c b/src/core/socket.c index c8f3329f9..13d88253b 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -132,8 +132,7 @@ sock_get_fd(void *s, int flag, int *fdp) rv = nni_msgq_get_recvable(SOCK(s)->s_urq, &p); break; default: - rv = NNG_EINVAL; - break; + return (NNG_EINVAL); } if (rv == 0) { @@ -406,7 +405,7 @@ sock_stats_fini(nni_sock *s) { #ifdef NNG_ENABLE_STATS sock_stats *st = &s->s_stats; - nni_stat_remove(&st->s_root); + nni_stat_unregister(&st->s_root); #else NNI_ARG_UNUSED(s); #endif @@ -425,47 +424,47 @@ sock_stats_init(nni_sock *s) nni_stat_init_scope(root, s->s_scope, "socket statistics"); nni_stat_init_id(&st->s_id, "id", "socket id", s->s_id); - nni_stat_append(root, &st->s_id); + nni_stat_add(root, &st->s_id); nni_stat_init_string(&st->s_name, "name", "socket name", s->s_name); nni_stat_set_lock(&st->s_name, &s->s_mx); - nni_stat_append(root, &st->s_name); + nni_stat_add(root, &st->s_name); nni_stat_init_string(&st->s_protocol, "protocol", "socket protocol", nni_sock_proto_name(s)); - nni_stat_append(root, &st->s_protocol); + nni_stat_add(root, &st->s_protocol); nni_stat_init_atomic(&st->s_ndialers, "ndialers", "open dialers"); nni_stat_set_type(&st->s_ndialers, NNG_STAT_LEVEL); - nni_stat_append(root, &st->s_ndialers); + nni_stat_add(root, &st->s_ndialers); nni_stat_init_atomic( &st->s_nlisteners, "nlisteners", "open listeners"); nni_stat_set_type(&st->s_nlisteners, NNG_STAT_LEVEL); - nni_stat_append(root, &st->s_nlisteners); + nni_stat_add(root, &st->s_nlisteners); nni_stat_init_atomic(&st->s_npipes, "npipes", "open pipes"); nni_stat_set_type(&st->s_npipes, NNG_STAT_LEVEL); - nni_stat_append(root, &st->s_npipes); + nni_stat_add(root, &st->s_npipes); nni_stat_init_atomic(&st->s_rxbytes, "rxbytes", "bytes received"); nni_stat_set_unit(&st->s_rxbytes, NNG_UNIT_BYTES); - nni_stat_append(root, &st->s_rxbytes); + nni_stat_add(root, &st->s_rxbytes); nni_stat_init_atomic(&st->s_txbytes, "txbytes", "bytes sent"); nni_stat_set_unit(&st->s_txbytes, NNG_UNIT_BYTES); - nni_stat_append(root, &st->s_txbytes); + nni_stat_add(root, &st->s_txbytes); nni_stat_init_atomic(&st->s_rxmsgs, "rxmsgs", "messages received"); nni_stat_set_unit(&st->s_rxmsgs, NNG_UNIT_MESSAGES); - nni_stat_append(root, &st->s_rxmsgs); + nni_stat_add(root, &st->s_rxmsgs); nni_stat_init_atomic(&st->s_txmsgs, "txmsgs", "messages sent"); nni_stat_set_unit(&st->s_txmsgs, NNG_UNIT_MESSAGES); - nni_stat_append(root, &st->s_txmsgs); + nni_stat_add(root, &st->s_txmsgs); nni_stat_init_atomic(&st->s_reject, "reject", "pipes rejected"); - nni_stat_append(root, &st->s_reject); + nni_stat_add(root, &st->s_reject); #else NNI_ARG_UNUSED(s); #endif @@ -624,7 +623,7 @@ nni_sock_open(nni_sock **sockp, const nni_proto *proto) } nni_mtx_lock(&sock_lk); - if ((rv = nni_idhash_alloc32(sock_hash, &s->s_id, s)) != 0) { + if (nni_idhash_alloc32(sock_hash, &s->s_id, s) != 0) { sock_destroy(s); } else { nni_list_append(&sock_list, s); @@ -633,7 +632,7 @@ nni_sock_open(nni_sock **sockp, const nni_proto *proto) } nni_mtx_unlock(&sock_lk); - // Set the sockname. + // Set the socket name. (void) snprintf(s->s_name, sizeof(s->s_name), "%u", s->s_id); // Set up basic stat values. @@ -641,7 +640,7 @@ nni_sock_open(nni_sock **sockp, const nni_proto *proto) nni_stat_set_value(&s->s_stats.s_id, s->s_id); // Add our stats chain. - nni_stat_append(NULL, &s->s_stats.s_root); + nni_stat_register(&s->s_stats.s_root); return (0); } @@ -775,7 +774,7 @@ nni_sock_close(nni_sock *s) // is idempotent. nni_sock_shutdown(s); - nni_stat_remove(&s->s_stats.s_root); + nni_stat_unregister(&s->s_stats.s_root); nni_mtx_lock(&sock_lk); if (s->s_closed) { @@ -977,11 +976,6 @@ nni_sock_setopt( } nni_mtx_unlock(&s->s_mx); - // If the option was already handled one way or the other, - if (rv != NNG_ENOTSUP) { - return (rv); - } - // Validation of generic and transport options. This is stateless, so // transports should not fail to set an option later if they // passed it here. @@ -1078,7 +1072,7 @@ int nni_sock_getopt( nni_sock *s, const char *name, void *val, size_t *szp, nni_type t) { - int rv = NNG_ENOTSUP; + int rv; nni_sockopt *sopt; nni_mtx_lock(&s->s_mx); @@ -1444,7 +1438,7 @@ nni_dialer_add_pipe(nni_dialer *d, void *tpipe) return; } nni_mtx_unlock(&s->s_mx); - + nni_stat_register(&p->p_stats.s_root); nni_pipe_run_cb(p, NNG_PIPE_EV_ADD_POST); nni_pipe_rele(p); } @@ -1492,6 +1486,8 @@ nni_dialer_reap(nni_dialer *d) nni_aio_stop(d->d_tmo_aio); nni_aio_stop(d->d_con_aio); + nni_stat_unregister(&d->d_stats.s_root); + nni_mtx_lock(&s->s_mx); if (!nni_list_empty(&d->d_pipes)) { nni_pipe *p; @@ -1553,7 +1549,7 @@ nni_listener_add_pipe(nni_listener *l, void *tpipe) return; } nni_mtx_unlock(&s->s_mx); - + nni_stat_register(&p->p_stats.s_root); nni_pipe_run_cb(p, NNG_PIPE_EV_ADD_POST); nni_pipe_rele(p); } @@ -1602,6 +1598,8 @@ nni_listener_reap(nni_listener *l) nni_aio_stop(l->l_tmo_aio); nni_aio_stop(l->l_acc_aio); + nni_stat_unregister(&l->l_stats.s_root); + nni_mtx_lock(&s->s_mx); if (!nni_list_empty(&l->l_pipes)) { nni_pipe *p; @@ -1687,7 +1685,7 @@ void nni_sock_add_stat(nni_sock *s, nni_stat_item *stat) { #ifdef NNG_ENABLE_STATS - nni_stat_append(&s->s_stats.s_root, stat); + nni_stat_add(&s->s_stats.s_root, stat); #else NNI_ARG_UNUSED(s); NNI_ARG_UNUSED(stat); diff --git a/src/core/stats.c b/src/core/stats.c index 247e7ab4e..74b14f594 100644 --- a/src/core/stats.c +++ b/src/core/stats.c @@ -36,13 +36,9 @@ static nni_mtx * stats_held = NULL; #endif void -nni_stat_append(nni_stat_item *parent, nni_stat_item *child) +nni_stat_add(nni_stat_item *parent, nni_stat_item *child) { #ifdef NNG_ENABLE_STATS - if (parent == NULL) { - parent = &stats_root; - } - nni_mtx_lock(&stats_lock); // Make sure that the lists for both children and parents // are correctly initialized. if (parent->si_children.ll_head.ln_next == NULL) { @@ -53,15 +49,24 @@ nni_stat_append(nni_stat_item *parent, nni_stat_item *child) } nni_list_append(&parent->si_children, child); child->si_parent = parent; - nni_mtx_unlock(&stats_lock); #else NNI_ARG_UNUSED(parent); NNI_ARG_UNUSED(child); #endif } +// nni_stat_register registers a stat tree, acquiring the lock +// on the stats structures before doing so. +void +nni_stat_register(nni_stat_item *child) +{ + nni_mtx_lock(&stats_lock); + nni_stat_add(&stats_root, child); + nni_mtx_unlock(&stats_lock); +} + void -nni_stat_remove(nni_stat_item *child) +nni_stat_unregister(nni_stat_item *child) { #ifdef NNG_ENABLE_STATS nni_stat_item *parent; @@ -141,7 +146,6 @@ stat_atomic_update(nni_stat_item *stat, void *notused) void nni_stat_init_atomic(nni_stat_item *stat, const char *name, const char *desc) { - nni_stat_init(stat, name, desc); stat->si_number = 0; stat->si_private = NULL; diff --git a/src/core/stats.h b/src/core/stats.h index 215e07d81..12dbbbdd7 100644 --- a/src/core/stats.h +++ b/src/core/stats.h @@ -54,8 +54,16 @@ struct nni_stat_item { #endif }; -void nni_stat_append(nni_stat_item *, nni_stat_item *); -void nni_stat_remove(nni_stat_item *); +// nni_stat_add adds a statistic, but the operation is unlocked, and the +// add is to an unregistered stats tree. +void nni_stat_add(nni_stat_item *, nni_stat_item *); + +// nni_stat_register registers a statistic tree into the global tree. +// The tree is rooted at the root. This is a locked operation. +void nni_stat_register(nni_stat_item *); + +// nni_stat_unregister removes the entire tree. This is a locked operation. +void nni_stat_unregister(nni_stat_item *); void nni_stat_set_value(nni_stat_item *, uint64_t); void nni_stat_set_string(nni_stat_item *, const char *); diff --git a/src/protocol/pair1/pair.c b/src/protocol/pair1/pair.c index 7adb8bd80..70654d6db 100644 --- a/src/protocol/pair1/pair.c +++ b/src/protocol/pair1/pair.c @@ -22,7 +22,7 @@ #define NNI_PROTO_PAIR_V1 NNI_PROTO(1, 1) #endif -#define BUMPSTAT(x) nni_stat_inc_atomic(x, 1) +#define BUMP_STAT(x) nni_stat_inc_atomic(x, 1) typedef struct pair1_pipe pair1_pipe; typedef struct pair1_sock pair1_sock; @@ -199,7 +199,7 @@ pair1_pipe_start(void *arg) nni_mtx_lock(&s->mtx); if (nni_pipe_peer(p->npipe) != NNI_PROTO_PAIR_V1) { nni_mtx_unlock(&s->mtx); - BUMPSTAT(&s->stat_rejmismatch); + BUMP_STAT(&s->stat_rejmismatch); // Peer protocol mismatch. return (NNG_EPROTO); } @@ -213,7 +213,7 @@ pair1_pipe_start(void *arg) if (!nni_list_empty(&s->plist)) { nni_idhash_remove(s->pipes, id); nni_mtx_unlock(&s->mtx); - BUMPSTAT(&s->stat_rejinuse); + BUMP_STAT(&s->stat_rejinuse); return (NNG_EBUSY); } } else { diff --git a/src/supplemental/websocket/websocket.h b/src/supplemental/websocket/websocket.h index bbc58d305..1ecb46188 100644 --- a/src/supplemental/websocket/websocket.h +++ b/src/supplemental/websocket/websocket.h @@ -29,7 +29,7 @@ typedef struct nni_ws_dialer nni_ws_dialer; // on INADDR_ANY port 80, with path "/". For connect side, INADDR_ANY // makes no sense. (TBD: return NNG_EADDRINVAL, or try loopback?) -// Much of the websocket API is still "private", meeaning you should not +// Much of the websocket API is still "private", meaning you should not // rely upon it being around. extern int nni_ws_listener_alloc(nng_stream_listener **, const nni_url *); extern int nni_ws_dialer_alloc(nng_stream_dialer **, const nni_url *); diff --git a/src/supplemental/websocket/wssfile_test.c b/src/supplemental/websocket/wssfile_test.c index f55a9e84a..8595c0adc 100644 --- a/src/supplemental/websocket/wssfile_test.c +++ b/src/supplemental/websocket/wssfile_test.c @@ -257,7 +257,16 @@ test_invalid_verify(void) TEST_NNG_PASS(nng_setopt_int( s2, NNG_OPT_TLS_AUTH_MODE, NNG_TLS_AUTH_MODE_REQUIRED)); - TEST_NNG_FAIL(nng_dial(s2, addr, NULL, 0), NNG_EPEERAUTH); + // We find that sometimes this fails due to NNG_EPEERAUTH, but it + // can also fail due to NNG_ECLOSED. This seems to be timing + // dependent, based on receive vs. send timing most likely. + // Applications shouldn't really depend that much on this. + int rv; + rv = nng_dial(s2, addr, NULL, 0); + TEST_CHECK(rv != 0); + TEST_CHECK_((rv == NNG_EPEERAUTH) || (rv == NNG_ECLOSED) || + (rv == NNG_ECRYPTO), + "result from dial: %d %s", rv, nng_strerror(rv)); TEST_NNG_PASS(nng_close(s1)); TEST_NNG_PASS(nng_close(s2));