API Change: uv_close only called by user - never automatically

Add test that failing on_connect callback does not trigger on_close.

test/benchmark-ping-pongs.c

@@ -47,7 +47,7 @@ typedef struct buf_s {
 static char PING[] = "PING\n";
 
 static buf_t* buf_freelist = NULL;
-
+static int pinger_shutdown_cb_called;
 static int completed_pingers = 0;
 static int64_t start_time;
 
@@ -117,6 +117,13 @@ static void pinger_write_ping(pinger_t* pinger) {
 
 static void pinger_shutdown_cb(uv_handle_t* handle, int status) {
   ASSERT(status == 0);
+  pinger_shutdown_cb_called++;
+
+  /*
+   * The close callback has not been triggered yet. We must wait for EOF
+   * until we close the connection.
+   */
+  ASSERT(completed_pingers == 0);
 }
 
 
@@ -133,6 +140,9 @@ static void pinger_read_cb(uv_tcp_t* tcp, int nread, uv_buf_t buf) {
       buf_free(buf);
     }
 
+    ASSERT(pinger_shutdown_cb_called == 1);
+    uv_close((uv_handle_t*)tcp);
+
     return;
   }
 
@@ -146,7 +156,6 @@ static void pinger_read_cb(uv_tcp_t* tcp, int nread, uv_buf_t buf) {
         uv_req_init(&pinger->shutdown_req, (uv_handle_t*)tcp, pinger_shutdown_cb);
         uv_shutdown(&pinger->shutdown_req);
         break;
-        return;
       } else {
         pinger_write_ping(pinger);
       }

test/echo-server.c

@@ -59,7 +59,7 @@ static void after_write(uv_req_t* req, int status) {
 
 
 static void after_shutdown(uv_req_t* req, int status) {
-  uv_close(req->handle); /* XXX Is the uv_close necessary??? */
+  uv_close(req->handle);
   free(req);
 }
 

test/test-connection-fail.c

@@ -31,6 +31,10 @@ static uv_req_t req;
 static int connect_cb_calls;
 static int close_cb_calls;
 
+static uv_timer_t timer;
+static int timer_close_cb_calls;
+static int timer_cb_calls;
+
 
 static void on_close(uv_handle_t* handle, int status) {
   ASSERT(status == 0);
@@ -38,20 +42,57 @@ static void on_close(uv_handle_t* handle, int status) {
 }
 
 
-static void on_connect(uv_req_t *req, int status) {
+static void timer_close_cb(uv_handle_t* handle, int status) {
+  ASSERT(status == 0);
+  timer_close_cb_calls++;
+}
+
+
+static void timer_cb(uv_handle_t* handle, int status) {
+  ASSERT(status == 0);
+  timer_cb_calls++;
+
+  /*
+   * These are the important asserts. The connection callback has been made,
+   * but libuv hasn't automatically closed the socket. The user must
+   * uv_close the handle manually.
+   */
+  ASSERT(close_cb_calls == 0);
+  ASSERT(connect_cb_calls == 1);
+
+  /* Close the tcp handle. */
+  uv_close((uv_handle_t*)&tcp);
+
+  /* Close the timer. */
+  uv_close(handle);
+}
+
+
+static void on_connect_with_close(uv_req_t *req, int status) {
   ASSERT(status == -1);
   ASSERT(uv_last_error().code == UV_ECONNREFUSED);
   connect_cb_calls++;
+
+  ASSERT(close_cb_calls == 0);
   uv_close(req->handle);
 }
 
 
-TEST_IMPL(connection_fail) {
+static void on_connect_without_close(uv_req_t *req, int status) {
+  ASSERT(status == -1);
+  ASSERT(uv_last_error().code == UV_ECONNREFUSED);
+  connect_cb_calls++;
+
+  uv_timer_start(&timer, timer_cb, 100, 0);
+
+  ASSERT(close_cb_calls == 0);
+}
+
+
+void connection_fail(uv_connect_cb connect_cb) {
   struct sockaddr_in client_addr, server_addr;
   int r;
 
-  uv_init();
-
   client_addr = uv_ip4_addr("0.0.0.0", 0);
 
   /* There should be no servers listening on this port. */
@@ -63,7 +104,7 @@ TEST_IMPL(connection_fail) {
 
   /* We are never doing multiple reads/connects at a time anyway. */
   /* so these handles can be pre-initialized. */
-  uv_req_init(&req, (uv_handle_t*)&tcp, on_connect);
+  uv_req_init(&req, (uv_handle_t*)&tcp, connect_cb);
 
   uv_bind(&tcp, client_addr);
   r = uv_connect(&req, server_addr);
@@ -73,6 +114,39 @@ TEST_IMPL(connection_fail) {
 
   ASSERT(connect_cb_calls == 1);
   ASSERT(close_cb_calls == 1);
+}
+
+
+/*
+ * This test attempts to connect to a port where no server is running. We
+ * expect an error.
+ */
+TEST_IMPL(connection_fail) {
+  uv_init();
+
+  connection_fail(on_connect_with_close);
+
+  ASSERT(timer_close_cb_calls == 0);
+  ASSERT(timer_cb_calls == 0);
+
+  return 0;
+}
+
+
+/*
+ * This test is the same as the first except it check that the close
+ * callback of the tcp handle hasn't been made after the failed connection
+ * attempt.
+ */
+TEST_IMPL(connection_fail_doesnt_auto_close) {
+  uv_init();
+
+  uv_timer_init(&timer, timer_close_cb, NULL);
+
+  connection_fail(on_connect_without_close);
+
+  ASSERT(timer_close_cb_calls == 1);
+  ASSERT(timer_cb_calls == 1);
 
   return 0;
 }

test/test-list.h

@@ -28,6 +28,7 @@ TEST_DECLARE   (bind_error_addrnotavail_2)
 TEST_DECLARE   (bind_error_fault)
 TEST_DECLARE   (bind_error_inval)
 TEST_DECLARE   (connection_fail)
+TEST_DECLARE   (connection_fail_doesnt_auto_close)
 TEST_DECLARE   (callback_stack)
 TEST_DECLARE   (timer)
 TEST_DECLARE   (timer_again)
@@ -63,6 +64,7 @@ TASK_LIST_START
   TEST_ENTRY  (bind_error_inval)
 
   TEST_ENTRY  (connection_fail)
+  TEST_ENTRY  (connection_fail_doesnt_auto_close)
 
   TEST_ENTRY  (callback_stack)
   TEST_HELPER (callback_stack, echo_server)

uv-unix.c

@@ -343,7 +343,10 @@ void uv__server_io(EV_P_ ev_io* watcher, int revents) {
         return;
       } else {
         uv_err_new((uv_handle_t*)tcp, errno);
-        uv_close((uv_handle_t*)tcp);
+        /*
+         * XXX TODO how do we report this error to the user?  What errors
+         * are possible here?
+         */
       }
 
     } else {
@@ -516,9 +519,8 @@ static void uv__drain(uv_tcp_t* tcp) {
     uv_shutdown_cb cb = req->cb;
 
     if (shutdown(tcp->fd, SHUT_WR)) {
-      /* Error. Nothing we can do, close the tcp. */
+      /* Error. Report it. User should call uv_close(). */
       uv_err_new((uv_handle_t*)tcp, errno);
-      uv_close((uv_handle_t*)tcp);
       if (cb) cb(req, -1);
     } else {
       uv_err_new((uv_handle_t*)tcp, 0);
@@ -563,9 +565,6 @@ void uv__write(uv_tcp_t* tcp) {
     if (errno != EAGAIN) {
       uv_err_t err = uv_err_new((uv_handle_t*)tcp, errno);
 
-      /* XXX How do we tcp the error? Need test coverage here. */
-      uv_close((uv_handle_t*)tcp);
-
       if (cb) {
         cb(req, -1);
       }
@@ -662,8 +661,8 @@ void uv__read(uv_tcp_t* tcp) {
         tcp->read_cb(tcp, 0, buf);
         return;
       } else {
+        /* Error. User should call uv_close(). */
         uv_err_new((uv_handle_t*)tcp, errno);
-        uv_close((uv_handle_t*)tcp);
         tcp->read_cb(tcp, -1, buf);
         assert(!ev_is_active(&tcp->read_watcher));
         return;
@@ -673,10 +672,6 @@ void uv__read(uv_tcp_t* tcp) {
       uv_err_new_artificial((uv_handle_t*)tcp, UV_EOF);
       ev_io_stop(EV_DEFAULT_UC_ &tcp->read_watcher);
       tcp->read_cb(tcp, -1, buf);
-
-      if (uv_flag_is_set((uv_handle_t*)tcp, UV_SHUT)) {
-        uv_close((uv_handle_t*)tcp);
-      }
       return;
     } else {
       /* Successful read */
@@ -780,8 +775,6 @@ static void uv__tcp_connect(uv_tcp_t* tcp) {
     if (connect_cb) {
       connect_cb(req, -1);
     }
-
-    uv_close((uv_handle_t*)tcp);
   }
 }