Creating a second poll handle over the same fd is not supported. Currently we do give an error if the handle is running, but we cannot detect it if the handle was stopped (maybe we could iterate the loop handle list?) so you are dancing in undefined behavior I think. Nothing prevents you from uv_poll_start -ing the old poll handle after you stopped it (this is allowed) so things would break.

The typical recommendattion here is to keep a map of fd-poll-handle at the application layer and rreuse the handle for the same fd.

Refs: #1172 (comment) with a description of possible solutions to the issue.

Thanks for the PR but the scenario you describe is a bug in libuv usage: you're not supposed to have two uv_poll_t handles with overlapping lifetimes that reference the same file descriptor.

I can see how your change "fixes" the immediate issue but it really just hides an application bug. it'd be better to catch it in uv_poll_start().

uv_poll_init() already has an "is this fd in use?" check but that only catches bugs when the other poll handle is already started. Can you check if this patch works for your use case?

diff --git a/src/unix/poll.c b/src/unix/poll.c
index 3d5022b2..e43f4ec9 100644
--- a/src/unix/poll.c
+++ b/src/unix/poll.c
@@ -116,12 +116,22 @@ int uv_poll_stop(uv_poll_t* handle) {
 
 
 int uv_poll_start(uv_poll_t* handle, int pevents, uv_poll_cb poll_cb) {
+  uv__io_t** watchers;
+  uv__io_t* w;
   int events;
 
   assert((pevents & ~(UV_READABLE | UV_WRITABLE | UV_DISCONNECT |
                       UV_PRIORITIZED)) == 0);
   assert(!uv__is_closing(handle));
 
+  watchers = handle->loop->watchers;
+  w = &handle->io_watcher;
+
+  if ((unsigned) w->fd < handle->loop->nwatchers)
+    if (watchers[w->fd] != NULL)
+      if (watchers[w->fd] != w)
+        return UV_EEXIST;
+
   uv__poll_stop(handle);
 
   if (pevents == 0)

@saghul Which is sort of okay, if it wasn't for file descriptor reuse from the OS. I.e. the cycle collector closes the fd, a destructor creates a new fd and watches it. The cycle collector closes the poll handle and it explodes.
It's nothing users of PHP can have an influence on and thus if this is not handled, just makes using libuv a ticking time bomb in PHP.

@bnoordhuis This does not help - after all the original file descriptor has been stopped, just not closed yet. But it really does not help with the edge case where a file descriptor gets reused by bad luck.

The true alternative for me would be guarding the explicit uv_close by a destructor with a check whether the pevents == 0. But it's private API, so I cannot rely on that...

But it really does not help with the edge case where a file descriptor gets reused by bad luck.

I don't think that's libuv job to fix. You'll have to make php-uv conform to libuv's API contract.

What really is unsupported, judging from the code, is having two poll handles active at the same fd at the same time. Everything else can be easily allowed without breaking anything.

It is not okay to have multiple active poll handles for the same socket, this can cause libuv to busyloop or otherwise malfunction. ~ http://docs.libuv.org/en/v1.x/poll.html

@bnoordhuis In this case the docs are at best misleading. Docs explicitly state active poll handles. Nothing about just having two initialized poll handles of which one is inactive. (Which, I see no reason for to malfunction.)

Additionally, the docs state:

However the fd can be safely closed immediately after a call to uv_poll_stop() or uv_close(). ~ http://docs.libuv.org/en/v1.x/poll.html

But it turns out I cannot, due to the behavior exposed. The current behavior requires me to first uv_close() before a close(2) call; uv_poll_stop() is not enough. (After all the docs tell me to always call uv_close() as well, if I don't want to leak memory.)

FWIW @saghul I cannot keep a file descriptor to handle map in PHP, because PHP does not expose file descriptors natively. I only can keep a map to a resource (which we do). But the resource number is not related to the fd number.

Valid observation. Is this the behavior you're after?

diff --git a/src/unix/core.c b/src/unix/core.c
index 1e50f798..2315105e 100644
--- a/src/unix/core.c
+++ b/src/unix/core.c
@@ -899,13 +899,12 @@ void uv__io_stop(uv_loop_t* loop, uv__io_t* w, unsigned int events) {
   if (w->pevents == 0) {
     QUEUE_REMOVE(&w->watcher_queue);
     QUEUE_INIT(&w->watcher_queue);
+    w->events = 0;
 
-    if (loop->watchers[w->fd] != NULL) {
-      assert(loop->watchers[w->fd] == w);
+    if (w == loop->watchers[w->fd]) {
       assert(loop->nfds > 0);
       loop->watchers[w->fd] = NULL;
       loop->nfds--;
-      w->events = 0;
     }
   }
   else if (QUEUE_EMPTY(&w->watcher_queue))

FWIW @saghul I cannot keep a file descriptor to handle map in PHP, because PHP does not expose file descriptors natively. I only can keep a map to a resource (which we do). But the resource number is not related to the fd number.

But yiou could expose it in your libuv wrapper can't you? Someone must know it in order to passs it along to uv_poll_init.

@bnoordhuis Yes, looks good.

I think we should additionally add your uv_poll_start() change. So that the safeguard is still in place (and checking it at poll start is a much better time than at poll stop).

@saghul I could add a function to get the fd, yes. But the point of the PHP I/O layer is abstracting file descriptors away. Currently the fd is extracted from the PHP resource within the uv_poll_init() function of the PHP bindings, opaquely to the user.

@bnoordhuis Shall I still do anything about it / change my PR to incorporate your patch suggestions? Or will you handle it fully from here on?

Anyway, thanks for the quick and interactive replies :-)

It's going to need regression tests so if you're up for writing those you're welcome to take my patches. If not, I'll get to it eventually but probably not today.

I'll give it a go at writing some regression test.

@bnoordhuis Applied your patches and added a regression test for it.

@bnoordhuis bump ?

@bnoordhuis Applied/fixed your suggestions and comments - should be fine now I hope :-)

@bnoordhuis fixed the issue on linux, on windows apparently close_cb() is not called at all?
Is that a bug (in libuv in general) or expected windows behavior?

I was under the assumption that close gets always called on a next tick for polls, but I have no knowledge about the windows internals… Seems to be a bit delayed as windows seems to force a wait before closing? Can you please have a look how to best handle that in the test? Or just make the whole test unix only? EDIT: Or issue uv_stop() in close cb and not use run_once?

Sorry, missed the notification. Here's a CI with this PR rebased against v1.x's tip, let's see how it fares: https://ci.nodejs.org/job/libuv-test-commit/1924/

Seems there was some infra issue. New run: https://ci.nodejs.org/job/libuv-test-commit/1927/

what's the status here?

@vtjnash Is there any chance to get help from you libuv maintainers to fix the CI for windows?

Rebase and I can re-run CI. I try to sweep up old issues every 6 months, if I can get the time. But often depend on others doing the work, so review can be quicker.

@vtjnash Thanks for the quick reply: rebased it now.

@vtjnash Applied your suggested fix - can you please re-run CI now?