Browse Source

io: fix nasty io_wake corner case.

If we're duplex and one io_always callback makes the other io_always,
we screwed up and hit an assertion later when the conn was in the
always list but didn't actually want to be.

io_wake() uses io_always(), so this is how it happened.  Writing a
test case for this was a bit fun, too.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Rusty Russell 8 years ago
parent
commit
d00c9d1be5
2 changed files with 68 additions and 1 deletions
  1. 14 1
      ccan/io/io.c
  2. 54 0
      ccan/io/test/run-40-wakeup-mutual.c

+ 14 - 1
ccan/io/io.c

@@ -392,12 +392,25 @@ void io_ready(struct io_conn *conn, int pollflags)
 
 
 void io_do_always(struct io_conn *conn)
 void io_do_always(struct io_conn *conn)
 {
 {
+	/* There's a corner case where the in next_plan wakes up the
+	 * out, placing it in IO_ALWAYS and we end up processing it immediately,
+	 * only to leave it in the always list.
+	 *
+	 * Yet we can't just process one, in case they are both supposed
+	 * to be done, so grab state beforehand.
+	 */
+	bool always_out = (conn->plan[IO_OUT].status == IO_ALWAYS);
+
 	if (conn->plan[IO_IN].status == IO_ALWAYS)
 	if (conn->plan[IO_IN].status == IO_ALWAYS)
 		if (!next_plan(conn, &conn->plan[IO_IN]))
 		if (!next_plan(conn, &conn->plan[IO_IN]))
 			return;
 			return;
 
 
-	if (conn->plan[IO_OUT].status == IO_ALWAYS)
+	if (always_out) {
+		/* You can't *unalways* a conn (except by freeing, in which
+		 * case next_plan() returned false */
+		assert(conn->plan[IO_OUT].status == IO_ALWAYS);
 		next_plan(conn, &conn->plan[IO_OUT]);
 		next_plan(conn, &conn->plan[IO_OUT]);
+	}
 }
 }
 
 
 void io_do_wakeup(struct io_conn *conn, enum io_direction dir)
 void io_do_wakeup(struct io_conn *conn, enum io_direction dir)

+ 54 - 0
ccan/io/test/run-40-wakeup-mutual.c

@@ -0,0 +1,54 @@
+/* Test previous issue: in duplex case, we wake reader reader wakes writer. */
+#include <ccan/io/io.h>
+/* Include the C files directly. */
+#include <ccan/io/poll.c>
+#include <ccan/io/io.c>
+#include <ccan/tap/tap.h>
+#include <sys/wait.h>
+#include <stdio.h>
+
+static struct io_plan *block_reading(struct io_conn *conn, void *unused)
+{
+	static char buf[1];
+	return io_read(conn, buf, sizeof(buf), io_never, NULL);
+}
+
+static struct io_plan *writer_woken(struct io_conn *conn, void *unused)
+{
+	pass("Writer woken up");
+	return io_write(conn, "1", 1, io_close_cb, NULL);
+}
+
+static struct io_plan *reader_woken(struct io_conn *conn, void *unused)
+{
+	pass("Reader woken up");
+	/* Wake writer */
+	io_wake(conn);
+	return block_reading(conn, unused);
+}
+
+static struct io_plan *setup_conn(struct io_conn *conn, void *trigger)
+{
+	return io_duplex(conn,
+			 io_wait(conn, trigger, reader_woken, NULL),
+			 io_out_wait(conn, conn, writer_woken, NULL));
+}
+
+int main(void)
+{
+	int fds[2];
+	
+	plan_tests(3);
+	ok1(socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) == 0);
+
+	/* We use 'fds' as pointer to wake writer. */
+	io_new_conn(NULL, fds[0], setup_conn, fds);
+
+	io_wake(fds);
+	io_loop(NULL, NULL);
+
+	close(fds[1]);
+
+	/* This exits depending on whether all tests passed */
+	return exit_status();
+}