win, signal: flag to disable signal control handler #1168

bzoz · 2016-12-13T12:59:36Z

Trying to unregister signal handler from the handler itself can lead to deadlock. If it is the last handle, and uv__signal_control_handler_refs fail to zero a call to SetConsoleCtrlHandler is made, which blocks until all control handlers exits.

This PR adds flags that marks uv__signal_control_handler as disabled instead of removing it when uv__signal_control_handler_refs falls to zero.

Fixes part of: nodejs/node#10165

saghul · 2016-12-13T15:00:55Z

AFAIS we'd never reset the signal control handler. Is that what we want? If so, I'd like to suggest a different approach :-)

Completely get rid of uv__signal_control_handler_refs and use a uv_once_t for the initialization of the signal handler. As for checking if the signal needs to be dispatched or not, getting the lock and checking if the RB tree is empty should do. I think this would make it simpler, WDYT?

bzoz · 2016-12-16T09:06:45Z

I simplified it further: I've moved SetConsoleCtrlHandler to uv_signals_init. PTAL

saghul · 2016-12-16T09:28:38Z

src/win/signal.c

-
-    default:
-      /* Invalid signal. */
-      return ERROR_INVALID_PARAMETER;


This function served also to validate input. Now this would not complain on invalid input.

saghul · 2016-12-16T09:32:20Z

Can we have a test for this?

santigimeno · 2016-12-16T09:36:50Z

One question: is it fine having a signal handler even though no signal watchers have registered?

bzoz · 2016-12-16T10:43:35Z

Added testing if signum is correct and a test.

@santigimeno Yes. We return FALSE to indicate that we did not handle the signal.

saghul · 2016-12-16T10:47:08Z

src/win/signal.c

  }
+  /* Test for other invalid signum values. */
+  if (signum != SIGWINCH && (signum < 0 || signum >= NSIG)) {
+    return ERROR_INVALID_PARAMETER;


You need to return a libuv error here. UV_EINVAL. Actually, you can probably combine this if with the one above.

saghul · 2016-12-16T10:47:54Z

test/test-signal.c

+static void signal_cb(uv_signal_t* handle, int signum) { }
+
+TEST_IMPL(win32_signum_number) {
+  uv_signal_t signal;


Use some other name for the handle please.

saghul · 2016-12-16T10:48:27Z

test/test-signal.c

+#ifdef _WIN32
+static void signal_cb(uv_signal_t* handle, int signum) { }
+
+TEST_IMPL(win32_signum_number) {


This test is OK, but we need one which tests the whole reason behind this patch. Can that be done?

I don't think so, at least nothing easy comes to my mind.

santigimeno · 2016-12-16T10:57:35Z

@bzoz Sorry, I'm not sure I follow and probably I'm wrong but if I understand the code correctly, the signal handler is now always set on initialization whereas before this change was only set on the first watcher registration. Isn't this a change of behavior?

saghul · 2016-12-16T11:13:15Z

@santigimeno I see what you mean. "Since there are no registered handles and the signal is ignored, will it affect applications in any way?" Is that it? I'd also like to know :-)

santigimeno · 2016-12-16T11:19:48Z

@saghul I think so. With this change it looks like the signals are always handled in uv__signal_control_handle whereas before if there were no watchers registered via uv_signal_start they weren't. How is it going to affect us?

bzoz · 2016-12-19T15:17:51Z

libuv supports SIGHUP and SIGINT on Windows by registering one handler. If we get SIGHUP and the only user-registered handler is for SIGINT we return FALSE meaning that we did not handle this signal. Same thing will happen if user registered SIGHUP handler and we get SIGINT: we also return FALSE.

Now, with this change the same thing will happen if no user registered handler is present .

saghul · 2016-12-21T22:04:01Z

src/win/signal.c

  InitializeCriticalSection(&uv__signal_lock);
+  if (!SetConsoleCtrlHandler(uv__signal_control_handler, TRUE)) {
+    abort();
+  }


drop braces for single statement conditionals

saghul · 2016-12-21T22:04:51Z

test/test-signal.c

+  ASSERT(uv_signal_start(&signal, signal_cb, SIGSEGV) == 0);
+  ASSERT(uv_signal_start(&signal, signal_cb, SIGTERM) == 0);
+  ASSERT(uv_signal_start(&signal, signal_cb, SIGABRT) == 0);
+  ASSERT(uv_signal_start(&signal, signal_cb, -1) == ERROR_INVALID_PARAMETER);


these need to return UV_EINVAL

saghul · 2016-12-21T22:05:16Z

test/test-signal.c

+  ASSERT(uv_signal_start(&signal, signal_cb, 1024) == ERROR_INVALID_PARAMETER);
+  return 0;
+}
+#else _WIN32


santigimeno · 2016-12-22T11:11:51Z

Now, with this change the same thing will happen if no user registered handler is present

@bzoz sorry to bring this up again. After your last comment I'm not sure that, when no watchers are registered for any signal, the behavior will be the same with these changes as without them.

mika-fischer · 2016-12-22T12:13:41Z

@santigimeno It depends on what you mean by "behavior". There's a difference, in that SetConsoleCtrlHandler will be called uncoditionally now, where before it was called only when a signal handler was registered.

However, there's no difference in behavior. The registered handler returns false as long as no libuv signal handlers are registered, which tells Windows that we didn't handle this signal, and the normal signal handling process should continue (which by default just terminates the process). I.e. the behavior is exactly the same as if no handler was installed at all.

Also note that, as @bzoz mentioned above, this is what was already happening in the past if a libuv handler for say SIGINT was installed and signals other than CTRL_C_EVENT were received. Our handler would be called, but since no libuv handler was registered, we returned false and the normal Windows signal handling process continued.

So there is no effective change in behavor.

mika-fischer · 2016-12-22T12:28:36Z

One thing I would like to note is that with this change, uv__signal_control_handler and thus uv__signal_dispatch can be called at absolutely any time, most significantly during and even after libuv shuts down and cleans up after itself. I don't know enough to tell if this could lead to any issues, for instance if there are still handlers registered for a signal while libuv is shut down.

santigimeno · 2016-12-22T12:29:36Z

@mika-fischer thanks a lot for the thorough explanation! I think I get it now (my Windows knowledge is almost zero).

bzoz · 2016-12-27T11:12:27Z

Updated, PTAL

@mika-fischer uv__signal_dispatch will walk uv__signal_tree and look for registered handlers. The uv__signal_tree does not seem to be destroyed or made invalid by libuv deinitialization, I think it should be OK.

saghul · 2017-01-02T07:38:37Z

test/test-signal.c


+/* For Windows we test only signum handling */
+#ifdef _WIN32
+static void signum_test_cb(uv_signal_t* handle, int signum) { }


add a "fatal" assert here, as we do in some other tests

bzoz · 2017-01-02T11:09:38Z

Updated, PTAL

saghul · 2017-01-04T08:19:16Z

test/test-signal.c

+  ASSERT(uv_signal_start(&signal, signum_test_cb, -1) == UV_EINVAL);
+  ASSERT(uv_signal_start(&signal, signum_test_cb, NSIG) == UV_EINVAL);
+  ASSERT(uv_signal_start(&signal, signum_test_cb, 1024) == UV_EINVAL);
+  return 0;


add a MAKE_VALGRIND_HAPPY before returning 0, that will cleanup the loop.

saghul

One last nit, oterwise LGTM.

Adds flags that marks uv__signal_control_handler as disabled instead of removing it when uv__signal_control_handler_refs falls to zero. Trying to remove the controller from the controller handle itself leads to deadlock. Ref: nodejs/node#10165

bzoz · 2017-01-04T11:24:51Z

Updated and squashed, PTAL

bzoz · 2017-01-17T15:18:20Z

Ping?

saghul · 2017-01-17T15:49:36Z

Sorry for the delay @bzoz I'll merge it later today.

Trying to remove the controller from the controller handle itself leads to deadlock. Fix it by always having the global signal handler engaged. Ref: nodejs/node#10165 PR-URL: #1168 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

saghul · 2017-01-18T03:26:31Z

Sorry for the delay, landed in c66f265. Thanks @bzoz, I really like how it turned out!

 3 files changed, 38 insertions(+), 117 deletions(-)

piscisaureus · 2017-01-26T21:06:59Z

Post-closing comment: the original, (now-removed) logic to install and remove the console control handler existed because of the philosophy that libuv should limit the amount of global application state it affects unconditionally.

For example, the application might opt not use use uv_signal but have it's own control handler instead.

From that perspective I consider this patch a minor step back.

Of course the fact that this patch fixes an actual problem that's affecting real applications trumps this academic concern of mine, and I also really like the significant reduction of complexity that this patch brings about.

If the unconditional installation of the control handler does create problems for other users, I would suggest making it 'lazy' again but without reinstating the logic to remove the control handler.

bzoz mentioned this pull request Dec 13, 2016

win, watchdog: add flag to mark handler as disabled nodejs/node#10248

Closed

2 tasks

saghul reviewed Dec 16, 2016

View reviewed changes

saghul requested changes Dec 21, 2016

View reviewed changes

saghul requested changes Jan 2, 2017

View reviewed changes

saghul reviewed Jan 4, 2017

View reviewed changes

bzoz force-pushed the bartek-fix-10165 branch from b54e730 to 015922f Compare January 4, 2017 11:24

saghul approved these changes Jan 17, 2017

View reviewed changes

saghul closed this Jan 18, 2017

piscisaureus mentioned this pull request Jan 26, 2017

CTRL_LOGOFF_EVENT and CTRL_SHUTDOWN_EVENT should be mapped to SIGHUP on Windows #1174

Closed

win, signal: flag to disable signal control handler #1168

win, signal: flag to disable signal control handler #1168

Conversation

bzoz commented Dec 13, 2016

Uh oh!

saghul commented Dec 13, 2016

Uh oh!

bzoz commented Dec 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saghul commented Dec 16, 2016

Uh oh!

santigimeno commented Dec 16, 2016

Uh oh!

bzoz commented Dec 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

santigimeno commented Dec 16, 2016

Uh oh!

saghul commented Dec 16, 2016

Uh oh!

santigimeno commented Dec 16, 2016

Uh oh!

bzoz commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

santigimeno commented Dec 22, 2016

Uh oh!

mika-fischer commented Dec 22, 2016

Uh oh!

mika-fischer commented Dec 22, 2016

Uh oh!

santigimeno commented Dec 22, 2016

Uh oh!

bzoz commented Dec 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Jan 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saghul left a comment

Choose a reason for hiding this comment

Uh oh!

bzoz commented Jan 4, 2017

Uh oh!

bzoz commented Jan 17, 2017

Uh oh!

saghul commented Jan 17, 2017

Uh oh!

saghul commented Jan 18, 2017

Uh oh!

piscisaureus commented Jan 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants