Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unix: process: prevent exit event before data events #610

Closed
bnoordhuis opened this issue Nov 6, 2015 · 1 comment
Closed

unix: process: prevent exit event before data events #610

bnoordhuis opened this issue Nov 6, 2015 · 1 comment

Comments

@bnoordhuis
Copy link
Member

See nodejs/node#3676 - the executive summary is that the order in which the child process's stdio I/O watchers and the signal handler I/O watcher run is indeterminate. This can result in the exit callback running before the data callbacks, which is confusing. It appears to be an easy fix, @gireeshpunathil reported good luck with the patch below.

diff --git a/deps/uv/src/unix/aix.c b/deps/uv/src/unix/aix.c
index c90b7e5..b129845 100644
--- a/deps/uv/src/unix/aix.c
+++ b/deps/uv/src/unix/aix.c
@@ -100,6 +100,7 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {
   uv__io_t* w;
   uint64_t base;
   uint64_t diff;
+  int have_signals;
   int nevents;
   int count;
   int nfds;
@@ -207,6 +208,7 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {
       goto update_timeout;
     }

+    have_signals = 0;
     nevents = 0;

     assert(loop->watchers != NULL);
@@ -237,10 +239,20 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {
         continue;
       }

-      w->cb(loop, w, pe->revents);
+      /* Run signal watchers last.  This also affects child process watchers
+       * because those are implemented in terms of signal watchers.
+       */
+      if (w == &loop->signal_io_watcher)
+        have_signals = 1;
+      else
+        w->cb(loop, w, pe->revents);
+
       nevents++;
     }

+    if (have_signals != 0)
+      loop->signal_io_watcher.cb(loop, &loop->signal_io_watcher, UV__POLLIN);
+
     loop->watchers[loop->nwatchers] = NULL;
     loop->watchers[loop->nwatchers + 1] = NULL;
@bnoordhuis
Copy link
Member Author

Note that the patch is not 100% correct. You can still get 'exit' before 'data' when uv__io_poll() decides to loop and poll again.

bnoordhuis added a commit to bnoordhuis/libuv that referenced this issue Nov 6, 2015
It was reported that some node.js tests fail on AIX because the exit
event sometimes comes before the final stdio output of a child process.

Work around that by deferring the signal watcher that is used for
process management until after the dispatch of regular i/o watchers.

Fixes: libuv#610
bnoordhuis added a commit to bnoordhuis/libuv that referenced this issue Apr 14, 2016
It was reported that some node.js tests fail on AIX because the exit
event sometimes comes before the final stdio output of a child process.

Work around that by deferring the signal watcher that is used for
process management until after the dispatch of regular i/o watchers.

Fixes: libuv#610
bnoordhuis added a commit to bnoordhuis/libuv that referenced this issue Apr 15, 2016
It was reported that some node.js tests fail on AIX because the exit
event sometimes comes before the final stdio output of a child process.

Work around that by deferring the signal watcher that is used for
process management until after the dispatch of regular i/o watchers.

Fixes: libuv#610
PR-URL: libuv#611
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
kthelgason pushed a commit to kthelgason/libuv that referenced this issue May 7, 2016
It was reported that some node.js tests fail on AIX because the exit
event sometimes comes before the final stdio output of a child process.

Work around that by deferring the signal watcher that is used for
process management until after the dispatch of regular i/o watchers.

Fixes: libuv#610
PR-URL: libuv#611
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant