XREQ and others race on TTL.

The TTL in these cases should have been atomic.  To facilitate
things we actually introduce an atomic int for convenience.  We
also introduce a convenience nni_msg_must_append_u32() and
nni_msg_header_must_append_u32(), so that we can eliminate some
failure tests that cannot ever happen.  Combined with a new test
for xreq, we have 100% coverage for xreq and more coverage for
the other REQ/REP protocols.

1 files changed, 65 insertions, 65 deletions

diff --git a/src/protocol/reqrep0/xreq.c b/src/protocol/reqrep0/xreq.c
index 15652f4f..ef24010e 100644
--- a/src/protocol/reqrep0/xreq.c
+++ b/src/protocol/reqrep0/xreq.c
@@ -11,7 +11,7 @@
 #include <stdio.h>
 
 #include "core/nng_impl.h"
-#include "nng/protocol/reqrep0/req.h"
+//#include "nng/protocol/reqrep0/req.h"
 
 // Request protocol.  The REQ protocol is the "request" side of a
 // request-reply pair.  This is useful for building RPC clients, for example.
@@ -29,19 +29,19 @@ typedef struct xreq0_sock xreq0_sock;
 
 // An xreq0_sock is our per-socket protocol private structure.
 struct xreq0_sock {
-	nni_msgq *uwq;
-	nni_msgq *urq;
-	int       ttl;
+	nni_msgq *     uwq;
+	nni_msgq *     urq;
+	nni_atomic_int ttl;
 };
 
 // A req0_pipe is our per-pipe protocol private structure.
 struct xreq0_pipe {
 	nni_pipe *  pipe;
 	xreq0_sock *req;
-	nni_aio *   aio_getq;
-	nni_aio *   aio_send;
-	nni_aio *   aio_recv;
-	nni_aio *   aio_putq;
+	nni_aio     aio_getq;
+	nni_aio     aio_send;
+	nni_aio     aio_recv;
+	nni_aio     aio_putq;
 };
 
 static void xreq0_sock_fini(void *);
@@ -55,7 +55,8 @@ xreq0_sock_init(void *arg, nni_sock *sock)
 {
 	xreq0_sock *s = arg;
 
-	s->ttl = 8;
+	nni_atomic_init(&s->ttl);
+	nni_atomic_set(&s->ttl, 8);
 	s->uwq = nni_sock_sendq(sock);
 	s->urq = nni_sock_recvq(sock);
 
@@ -85,10 +86,10 @@ xreq0_pipe_stop(void *arg)
 {
 	xreq0_pipe *p = arg;
 
-	nni_aio_stop(p->aio_getq);
-	nni_aio_stop(p->aio_putq);
-	nni_aio_stop(p->aio_recv);
-	nni_aio_stop(p->aio_send);
+	nni_aio_stop(&p->aio_getq);
+	nni_aio_stop(&p->aio_putq);
+	nni_aio_stop(&p->aio_recv);
+	nni_aio_stop(&p->aio_send);
 }
 
 static void
@@ -96,25 +97,21 @@ xreq0_pipe_fini(void *arg)
 {
 	xreq0_pipe *p = arg;
 
-	nni_aio_free(p->aio_getq);
-	nni_aio_free(p->aio_putq);
-	nni_aio_free(p->aio_recv);
-	nni_aio_free(p->aio_send);
+	nni_aio_fini(&p->aio_getq);
+	nni_aio_fini(&p->aio_putq);
+	nni_aio_fini(&p->aio_recv);
+	nni_aio_fini(&p->aio_send);
 }
 
 static int
 xreq0_pipe_init(void *arg, nni_pipe *pipe, void *s)
 {
 	xreq0_pipe *p = arg;
-	int         rv;
 
-	if (((rv = nni_aio_alloc(&p->aio_getq, xreq0_getq_cb, p)) != 0) ||
-	    ((rv = nni_aio_alloc(&p->aio_putq, xreq0_putq_cb, p)) != 0) ||
-	    ((rv = nni_aio_alloc(&p->aio_recv, xreq0_recv_cb, p)) != 0) ||
-	    ((rv = nni_aio_alloc(&p->aio_send, xreq0_send_cb, p)) != 0)) {
-		xreq0_pipe_fini(p);
-		return (rv);
-	}
+	nni_aio_init(&p->aio_getq, xreq0_getq_cb, p);
+	nni_aio_init(&p->aio_putq, xreq0_putq_cb, p);
+	nni_aio_init(&p->aio_recv, xreq0_recv_cb, p);
+	nni_aio_init(&p->aio_send, xreq0_send_cb, p);
 
 	p->pipe = pipe;
 	p->req  = s;
@@ -131,8 +128,8 @@ xreq0_pipe_start(void *arg)
 		return (NNG_EPROTO);
 	}
 
-	nni_msgq_aio_get(s->uwq, p->aio_getq);
-	nni_pipe_recv(p->pipe, p->aio_recv);
+	nni_msgq_aio_get(s->uwq, &p->aio_getq);
+	nni_pipe_recv(p->pipe, &p->aio_recv);
 	return (0);
 }
 
@@ -141,14 +138,14 @@ xreq0_pipe_close(void *arg)
 {
 	xreq0_pipe *p = arg;
 
-	nni_aio_close(p->aio_getq);
-	nni_aio_close(p->aio_putq);
-	nni_aio_close(p->aio_recv);
-	nni_aio_close(p->aio_send);
+	nni_aio_close(&p->aio_getq);
+	nni_aio_close(&p->aio_putq);
+	nni_aio_close(&p->aio_recv);
+	nni_aio_close(&p->aio_send);
 }
 
-// For raw mode we can just let the pipes "contend" via getq to get a
-// message from the upper write queue.  The msgqueue implementation
+// For raw mode we can just let the pipes "contend" via get queue to get a
+// message from the upper write queue.  The msg queue implementation
 // actually provides ordering, so load will be spread automatically.
 // (NB: We may have to revise this in the future if we want to provide some
 // kind of priority.)
@@ -158,15 +155,15 @@ xreq0_getq_cb(void *arg)
 {
 	xreq0_pipe *p = arg;
 
-	if (nni_aio_result(p->aio_getq) != 0) {
+	if (nni_aio_result(&p->aio_getq) != 0) {
 		nni_pipe_close(p->pipe);
 		return;
 	}
 
-	nni_aio_set_msg(p->aio_send, nni_aio_get_msg(p->aio_getq));
-	nni_aio_set_msg(p->aio_getq, NULL);
+	nni_aio_set_msg(&p->aio_send, nni_aio_get_msg(&p->aio_getq));
+	nni_aio_set_msg(&p->aio_getq, NULL);
 
-	nni_pipe_send(p->pipe, p->aio_send);
+	nni_pipe_send(p->pipe, &p->aio_send);
 }
 
 static void
@@ -174,15 +171,15 @@ xreq0_send_cb(void *arg)
 {
 	xreq0_pipe *p = arg;
 
-	if (nni_aio_result(p->aio_send) != 0) {
-		nni_msg_free(nni_aio_get_msg(p->aio_send));
-		nni_aio_set_msg(p->aio_send, NULL);
+	if (nni_aio_result(&p->aio_send) != 0) {
+		nni_msg_free(nni_aio_get_msg(&p->aio_send));
+		nni_aio_set_msg(&p->aio_send, NULL);
 		nni_pipe_close(p->pipe);
 		return;
 	}
 
 	// Sent a message so we just need to look for another one.
-	nni_msgq_aio_get(p->req->uwq, p->aio_getq);
+	nni_msgq_aio_get(p->req->uwq, &p->aio_getq);
 }
 
 static void
@@ -190,15 +187,15 @@ xreq0_putq_cb(void *arg)
 {
 	xreq0_pipe *p = arg;
 
-	if (nni_aio_result(p->aio_putq) != 0) {
-		nni_msg_free(nni_aio_get_msg(p->aio_putq));
-		nni_aio_set_msg(p->aio_putq, NULL);
+	if (nni_aio_result(&p->aio_putq) != 0) {
+		nni_msg_free(nni_aio_get_msg(&p->aio_putq));
+		nni_aio_set_msg(&p->aio_putq, NULL);
 		nni_pipe_close(p->pipe);
 		return;
 	}
-	nni_aio_set_msg(p->aio_putq, NULL);
+	nni_aio_set_msg(&p->aio_putq, NULL);
 
-	nni_pipe_recv(p->pipe, p->aio_recv);
+	nni_pipe_recv(p->pipe, &p->aio_recv);
 }
 
 static void
@@ -209,13 +206,13 @@ xreq0_recv_cb(void *arg)
 	nni_msg *   msg;
 	uint32_t    id;
 
-	if (nni_aio_result(p->aio_recv) != 0) {
+	if (nni_aio_result(&p->aio_recv) != 0) {
 		nni_pipe_close(p->pipe);
 		return;
 	}
 
-	msg = nni_aio_get_msg(p->aio_recv);
-	nni_aio_set_msg(p->aio_recv, NULL);
+	msg = nni_aio_get_msg(&p->aio_recv);
+	nni_aio_set_msg(&p->aio_recv, NULL);
 	nni_msg_set_pipe(msg, nni_pipe_id(p->pipe));
 
 	// We yank 4 bytes from front of body, and move them to the header.
@@ -226,15 +223,13 @@ xreq0_recv_cb(void *arg)
 		return;
 	}
 	id = nni_msg_trim_u32(msg);
-	if (nni_msg_header_append_u32(msg, id) != 0) {
-		// Probably ENOMEM, discard and carry on.
-		nni_msg_free(msg);
-		nni_pipe_recv(p->pipe, p->aio_recv);
-		return;
-	}
 
-	nni_aio_set_msg(p->aio_putq, msg);
-	nni_msgq_aio_put(sock->urq, p->aio_putq);
+	// Since we got this from the transport, there had better be some
+	// room in the header for our stuff.
+	nni_msg_header_must_append_u32(msg, id);
+
+	nni_aio_set_msg(&p->aio_putq, msg);
+	nni_msgq_aio_put(sock->urq, &p->aio_putq);
 }
 
 static void
@@ -254,17 +249,22 @@ xreq0_sock_recv(void *arg, nni_aio *aio)
 }
 
 static int
-xreq0_sock_set_maxttl(void *arg, const void *buf, size_t sz, nni_opt_type t)
+xreq0_sock_set_max_ttl(void *arg, const void *buf, size_t sz, nni_opt_type t)
 {
 	xreq0_sock *s = arg;
-	return (nni_copyin_int(&s->ttl, buf, sz, 1, 255, t));
+	int         ttl;
+	int         rv;
+	if ((rv = nni_copyin_int(&ttl, buf, sz, 1, 255, t)) == 0) {
+		nni_atomic_set(&s->ttl, ttl);
+	}
+	return (rv);
 }
 
 static int
-xreq0_sock_get_maxttl(void *arg, void *buf, size_t *szp, nni_opt_type t)
+xreq0_sock_get_max_ttl(void *arg, void *buf, size_t *szp, nni_opt_type t)
 {
 	xreq0_sock *s = arg;
-	return (nni_copyout_int(s->ttl, buf, szp, t));
+	return (nni_copyout_int(nni_atomic_get(&s->ttl), buf, szp, t));
 }
 
 static nni_proto_pipe_ops xreq0_pipe_ops = {
@@ -279,8 +279,8 @@ static nni_proto_pipe_ops xreq0_pipe_ops = {
 static nni_option xreq0_sock_options[] = {
 	{
 	    .o_name = NNG_OPT_MAXTTL,
-	    .o_get  = xreq0_sock_get_maxttl,
-	    .o_set  = xreq0_sock_set_maxttl,
+	    .o_get  = xreq0_sock_get_max_ttl,
+	    .o_set  = xreq0_sock_set_max_ttl,
 	},
 	// terminate list
 	{
@@ -310,7 +310,7 @@ static nni_proto xreq0_proto = {
 };
 
 int
-nng_req0_open_raw(nng_socket *sidp)
+nng_req0_open_raw(nng_socket *sock)
 {
-	return (nni_proto_open(sidp, &xreq0_proto));
+	return (nni_proto_open(sock, &xreq0_proto));
 }