unix, win: make fs-poll close wait for resource cleanup

Wait until all fs requests spawned by an `uv_fs_poll_t`
have finished and all timers created by it have fully been
closed before calling the close callback.

Fixes: https://github.com/libuv/libuv/issues/1869
PR-URL: https://github.com/libuv/libuv/pull/1875
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

src/fs-poll.c

@@ -22,12 +22,20 @@
 #include "uv.h"
 #include "uv-common.h"
 
+#ifdef _WIN32
+#include "win/internal.h"
+#include "win/handle-inl.h"
+#define uv__make_close_pending(h) uv_want_endgame((h)->loop, (h))
+#else
+#include "unix/internal.h"
+#endif
+
 #include <assert.h>
 #include <stdlib.h>
 #include <string.h>
 
 struct poll_ctx {
-  uv_fs_poll_t* parent_handle; /* NULL if parent has been stopped or closed */
+  uv_fs_poll_t* parent_handle;
   int busy_polling;
   unsigned int interval;
   uint64_t start_time;
@@ -36,6 +44,7 @@ struct poll_ctx {
   uv_timer_t timer_handle;
   uv_fs_t fs_req; /* TODO(bnoordhuis) mark fs_req internal */
   uv_stat_t statbuf;
+  struct poll_ctx* previous; /* context from previous start()..stop() period */
   char path[1]; /* variable length */
 };
 
@@ -49,6 +58,7 @@ static uv_stat_t zero_statbuf;
 
 int uv_fs_poll_init(uv_loop_t* loop, uv_fs_poll_t* handle) {
   uv__handle_init(loop, (uv_handle_t*)handle, UV_FS_POLL);
+  handle->poll_ctx = NULL;
   return 0;
 }
 
@@ -62,7 +72,7 @@ int uv_fs_poll_start(uv_fs_poll_t* handle,
   size_t len;
   int err;
 
-  if (uv__is_active(handle))
+  if (uv_is_active((uv_handle_t*)handle))
     return 0;
 
   loop = handle->loop;
@@ -90,6 +100,8 @@ int uv_fs_poll_start(uv_fs_poll_t* handle,
   if (err < 0)
     goto error;
 
+  if (handle->poll_ctx != NULL)
+    ctx->previous = handle->poll_ctx;
   handle->poll_ctx = ctx;
   uv__handle_start(handle);
 
@@ -104,19 +116,17 @@ error:
 int uv_fs_poll_stop(uv_fs_poll_t* handle) {
   struct poll_ctx* ctx;
 
-  if (!uv__is_active(handle))
+  if (!uv_is_active((uv_handle_t*)handle))
     return 0;
 
   ctx = handle->poll_ctx;
   assert(ctx != NULL);
-  assert(ctx->parent_handle != NULL);
-  ctx->parent_handle = NULL;
-  handle->poll_ctx = NULL;
+  assert(ctx->parent_handle == handle);
 
   /* Close the timer if it's active. If it's inactive, there's a stat request
    * in progress and poll_cb will take care of the cleanup.
    */
-  if (uv__is_active(&ctx->timer_handle))
+  if (uv_is_active((uv_handle_t*)&ctx->timer_handle))
     uv_close((uv_handle_t*)&ctx->timer_handle, timer_close_cb);
 
   uv__handle_stop(handle);
@@ -129,7 +139,7 @@ int uv_fs_poll_getpath(uv_fs_poll_t* handle, char* buffer, size_t* size) {
   struct poll_ctx* ctx;
   size_t required_len;
 
-  if (!uv__is_active(handle)) {
+  if (!uv_is_active((uv_handle_t*)handle)) {
     *size = 0;
     return UV_EINVAL;
   }
@@ -153,6 +163,9 @@ int uv_fs_poll_getpath(uv_fs_poll_t* handle, char* buffer, size_t* size) {
 
 void uv__fs_poll_close(uv_fs_poll_t* handle) {
   uv_fs_poll_stop(handle);
+
+  if (handle->poll_ctx == NULL)
+    uv__make_close_pending((uv_handle_t*)handle);
 }
 
 
@@ -173,14 +186,13 @@ static void poll_cb(uv_fs_t* req) {
   uv_stat_t* statbuf;
   struct poll_ctx* ctx;
   uint64_t interval;
+  uv_fs_poll_t* handle;
 
   ctx = container_of(req, struct poll_ctx, fs_req);
+  handle = ctx->parent_handle;
 
-  if (ctx->parent_handle == NULL) { /* handle has been stopped or closed */
-    uv_close((uv_handle_t*)&ctx->timer_handle, timer_close_cb);
-    uv_fs_req_cleanup(req);
-    return;
-  }
+  if (!uv_is_active((uv_handle_t*)handle) || uv__is_closing(handle))
+    goto out;
 
   if (req->result != 0) {
     if (ctx->busy_polling != req->result) {
@@ -205,7 +217,7 @@ static void poll_cb(uv_fs_t* req) {
 out:
   uv_fs_req_cleanup(req);
 
-  if (ctx->parent_handle == NULL) { /* handle has been stopped by callback */
+  if (!uv_is_active((uv_handle_t*)handle) || uv__is_closing(handle)) {
     uv_close((uv_handle_t*)&ctx->timer_handle, timer_close_cb);
     return;
   }
@@ -219,8 +231,27 @@ out:
 }
 
 
-static void timer_close_cb(uv_handle_t* handle) {
-  uv__free(container_of(handle, struct poll_ctx, timer_handle));
+static void timer_close_cb(uv_handle_t* timer) {
+  struct poll_ctx* ctx;
+  struct poll_ctx* it;
+  struct poll_ctx* last;
+  uv_fs_poll_t* handle;
+
+  ctx = container_of(timer, struct poll_ctx, timer_handle);
+  handle = ctx->parent_handle;
+  if (ctx == handle->poll_ctx) {
+    handle->poll_ctx = ctx->previous;
+    if (handle->poll_ctx == NULL)
+      uv__make_close_pending((uv_handle_t*)handle);
+  } else {
+    for (last = handle->poll_ctx, it = last->previous;
+         it != ctx;
+         last = it, it = it->previous) {
+      assert(last->previous != NULL);
+    }
+    last->previous = ctx->previous;
+  }
+  uv__free(ctx);
 }
 
 

src/unix/core.c

@@ -161,7 +161,9 @@ void uv_close(uv_handle_t* handle, uv_close_cb close_cb) {
 
   case UV_FS_POLL:
     uv__fs_poll_close((uv_fs_poll_t*)handle);
-    break;
+    /* Poll handles use file system requests, and one of them may still be
+     * running. The poll code will call uv__make_close_pending() for us. */
+    return;
 
   case UV_SIGNAL:
     uv__signal_close((uv_signal_t*) handle);

src/win/handle.c

@@ -139,7 +139,6 @@ void uv_close(uv_handle_t* handle, uv_close_cb cb) {
     case UV_FS_POLL:
       uv__fs_poll_close((uv_fs_poll_t*) handle);
       uv__handle_closing(handle);
-      uv_want_endgame(loop, handle);
       return;
 
     default:

test/test-fs-poll.c

@@ -185,3 +185,77 @@ TEST_IMPL(fs_poll_getpath) {
   MAKE_VALGRIND_HAPPY();
   return 0;
 }
+
+
+TEST_IMPL(fs_poll_close_request) {
+  uv_loop_t loop;
+  uv_fs_poll_t poll_handle;
+
+  remove(FIXTURE);
+
+  ASSERT(0 == uv_loop_init(&loop));
+
+  ASSERT(0 == uv_fs_poll_init(&loop, &poll_handle));
+  ASSERT(0 == uv_fs_poll_start(&poll_handle, poll_cb_fail, FIXTURE, 100));
+  uv_close((uv_handle_t*) &poll_handle, close_cb);
+  while (close_cb_called == 0)
+    uv_run(&loop, UV_RUN_ONCE);
+  ASSERT(close_cb_called == 1);
+
+  ASSERT(0 == uv_loop_close(&loop));
+
+  MAKE_VALGRIND_HAPPY();
+  return 0;
+}
+
+TEST_IMPL(fs_poll_close_request_multi_start_stop) {
+  uv_loop_t loop;
+  uv_fs_poll_t poll_handle;
+  int i;
+
+  remove(FIXTURE);
+
+  ASSERT(0 == uv_loop_init(&loop));
+
+  ASSERT(0 == uv_fs_poll_init(&loop, &poll_handle));
+
+  for (i = 0; i < 10; ++i) {
+    ASSERT(0 == uv_fs_poll_start(&poll_handle, poll_cb_fail, FIXTURE, 100));
+    ASSERT(0 == uv_fs_poll_stop(&poll_handle));
+  }
+  uv_close((uv_handle_t*) &poll_handle, close_cb);
+  while (close_cb_called == 0)
+    uv_run(&loop, UV_RUN_ONCE);
+  ASSERT(close_cb_called == 1);
+
+  ASSERT(0 == uv_loop_close(&loop));
+
+  MAKE_VALGRIND_HAPPY();
+  return 0;
+}
+
+TEST_IMPL(fs_poll_close_request_multi_stop_start) {
+  uv_loop_t loop;
+  uv_fs_poll_t poll_handle;
+  int i;
+
+  remove(FIXTURE);
+
+  ASSERT(0 == uv_loop_init(&loop));
+
+  ASSERT(0 == uv_fs_poll_init(&loop, &poll_handle));
+
+  for (i = 0; i < 10; ++i) {
+    ASSERT(0 == uv_fs_poll_stop(&poll_handle));
+    ASSERT(0 == uv_fs_poll_start(&poll_handle, poll_cb_fail, FIXTURE, 100));
+  }
+  uv_close((uv_handle_t*) &poll_handle, close_cb);
+  while (close_cb_called == 0)
+    uv_run(&loop, UV_RUN_ONCE);
+  ASSERT(close_cb_called == 1);
+
+  ASSERT(0 == uv_loop_close(&loop));
+
+  MAKE_VALGRIND_HAPPY();
+  return 0;
+}

test/test-list.h

@@ -280,6 +280,9 @@ TEST_DECLARE   (spawn_quoted_path)
 TEST_DECLARE   (spawn_tcp_server)
 TEST_DECLARE   (fs_poll)
 TEST_DECLARE   (fs_poll_getpath)
+TEST_DECLARE   (fs_poll_close_request)
+TEST_DECLARE   (fs_poll_close_request_multi_start_stop)
+TEST_DECLARE   (fs_poll_close_request_multi_stop_start)
 TEST_DECLARE   (kill)
 TEST_DECLARE   (kill_invalid_signum)
 TEST_DECLARE   (fs_file_noent)
@@ -817,6 +820,9 @@ TASK_LIST_START
   TEST_ENTRY  (spawn_tcp_server)
   TEST_ENTRY  (fs_poll)
   TEST_ENTRY  (fs_poll_getpath)
+  TEST_ENTRY  (fs_poll_close_request)
+  TEST_ENTRY  (fs_poll_close_request_multi_start_stop)
+  TEST_ENTRY  (fs_poll_close_request_multi_stop_start)
   TEST_ENTRY  (kill)
   TEST_ENTRY  (kill_invalid_signum)