0001-FIX-signal-unregistration.patch

From c2217768411c879539142997438580b3dc475270 Mon Sep 17 00:00:00 2001
From: Jonas Pfenniger <jonas@pfenniger.name>
Date: Thu, 13 May 2010 15:24:50 +0200
Subject: [PATCH] FIX: signal unregistration

* Added process.{add,remove}Listener wrappers for signal handling
* Thinner SignalWatcher, using same design as IdleWatcher
* Re-introduced test-signal-unregister test
---
 src/node.js                             |   54 ++++++++++++++++++------
 src/node_signal_watcher.cc              |   68 +++++++++++++++++++------------
 src/node_signal_watcher.h               |   19 +++++++--
 test/disabled/test-signal-unregister.js |   31 --------------
 test/simple/test-signal-unregister.js   |   31 ++++++++++++++
 5 files changed, 129 insertions(+), 74 deletions(-)
 delete mode 100644 test/disabled/test-signal-unregister.js
 create mode 100644 test/simple/test-signal-unregister.js

diff --git a/src/node.js b/src/node.js
index 4a6ed96..13f9891 100644
--- a/src/node.js
+++ b/src/node.js
@@ -65,20 +65,48 @@ process.compile("(function (exports) {"
 // module.require("events");
 
 // Signal Handlers
-
-function isSignal (event) {
-  return event.slice(0, 3) === 'SIG' && process.hasOwnProperty(event);
-};
-
-process.addListener("newListener", function (event) {
-  if (isSignal(event) && process.listeners(event).length === 0) {
-    var b = process.binding('signal_watcher');
-    var w = new b.SignalWatcher(process[event]);
-    w.addListener("signal", function () {
-      process.emit(event);
-    });
+(function() {
+  var signalWatchers = {};
+    addListener = process.addListener,
+    removeListener = process.removeListener;
+  
+  function isSignal (event) {
+    return event.slice(0, 3) === 'SIG' && process.hasOwnProperty(event);
+  };
+  
+  // Wrap addListener for the special signal types
+  process.addListener = function (type, listener) {
+    var ret = addListener.apply(this, arguments);
+    if (isSignal(type)) {
+      if (!signalWatchers.hasOwnProperty(type)) {
+        var b = process.binding('signal_watcher'),
+          w = new b.SignalWatcher(process[type]);
+          w.callback = function () {
+            process.emit(type);
+          }
+        signalWatchers[type] = w;
+        w.start();
+      } else if (this.listeners(type).length === 1) {
+        signalWatchers[event].start();
+      }
+    }
+    
+    return ret;
   }
-});
+  
+  process.removeListener = function (type, listener) {
+    var ret = removeListener.apply(this, arguments);
+    if (isSignal(type)) {
+      process.assert(signalWatchers.hasOwnProperty(type));
+      
+      if (this.listeners(type).length === 0) {
+        signalWatchers[type].stop();
+      }
+    }
+    
+    return ret;
+  }
+})();
 
 // Timers
 function addTimerListener (callback) {
diff --git a/src/node_signal_watcher.cc b/src/node_signal_watcher.cc
index 1eff3ba..f97341c 100644
--- a/src/node_signal_watcher.cc
+++ b/src/node_signal_watcher.cc
@@ -7,38 +7,46 @@ namespace node {
 using namespace v8;
 
 Persistent<FunctionTemplate> SignalWatcher::constructor_template;
-static Persistent<String> signal_symbol;
+static Persistent<String> callback_symbol;
 
 void SignalWatcher::Initialize(Handle<Object> target) {
   HandleScope scope;
 
   Local<FunctionTemplate> t = FunctionTemplate::New(SignalWatcher::New);
   constructor_template = Persistent<FunctionTemplate>::New(t);
-  constructor_template->Inherit(EventEmitter::constructor_template);
   constructor_template->InstanceTemplate()->SetInternalFieldCount(1);
   constructor_template->SetClassName(String::NewSymbol("SignalWatcher"));
 
+  NODE_SET_PROTOTYPE_METHOD(constructor_template, "start", SignalWatcher::Start);
   NODE_SET_PROTOTYPE_METHOD(constructor_template, "stop", SignalWatcher::Stop);
 
-  signal_symbol = NODE_PSYMBOL("signal");
-
   target->Set(String::NewSymbol("SignalWatcher"),
       constructor_template->GetFunction());
+  
+  callback_symbol = NODE_PSYMBOL("callback");
 }
 
-void SignalWatcher::OnSignal(EV_P_ ev_signal *watcher, int revents) {
+void SignalWatcher::Callback(EV_P_ ev_signal *watcher, int revents) {
   SignalWatcher *w = static_cast<SignalWatcher*>(watcher->data);
+
+  assert(watcher == &w->watcher_);
+
   HandleScope scope;
 
-  assert(revents == EV_SIGNAL);
+  Local<Value> callback_v = w->handle_->Get(callback_symbol);
+  if (!callback_v->IsFunction()) {
+    w->Stop();
+    return;
+  }
 
-  w->Emit(signal_symbol, 0, NULL);
-}
+  Local<Function> callback = Local<Function>::Cast(callback_v);
 
-SignalWatcher::~SignalWatcher() {
-  if (watcher_.active) {
-    ev_ref(EV_DEFAULT_UC);
-    ev_signal_stop(EV_DEFAULT_UC_ &watcher_);
+  TryCatch try_catch;
+
+  callback->Call(w->handle_, 0, NULL);
+
+  if (try_catch.HasCaught()) {
+    FatalException(try_catch);
   }
 }
 
@@ -51,30 +59,38 @@ Handle<Value> SignalWatcher::New(const Arguments& args) {
 
   int sig = args[0]->Int32Value();
 
-  SignalWatcher *w = new SignalWatcher();
+  SignalWatcher *w = new SignalWatcher(sig);
   w->Wrap(args.Holder());
 
-  ev_signal_init(&w->watcher_, SignalWatcher::OnSignal, sig);
-  w->watcher_.data = w;
-  // Give signal handlers very high priority. The only thing that has higher
-  // priority is the garbage collector check.
-  ev_set_priority(&w->watcher_, EV_MAXPRI-1);
-  ev_signal_start(EV_DEFAULT_UC_ &w->watcher_);
-  ev_unref(EV_DEFAULT_UC);
+  return args.This();
+}
 
-  w->Ref();
+Handle<Value> SignalWatcher::Start(const Arguments& args) {
+  HandleScope scope;
+  SignalWatcher *w = ObjectWrap::Unwrap<SignalWatcher>(args.Holder());
+  w->Start();
+  return Undefined();
+}
 
-  return args.This();
+void SignalWatcher::Start () {
+  if (!watcher_.active) {
+    ev_signal_start(EV_DEFAULT_UC_ &watcher_);
+    Ref();
+  }
 }
 
 Handle<Value> SignalWatcher::Stop(const Arguments& args) {
   HandleScope scope;
-
   SignalWatcher *w = ObjectWrap::Unwrap<SignalWatcher>(args.Holder());
-  ev_ref(EV_DEFAULT_UC);
-  ev_signal_stop(EV_DEFAULT_UC_ &w->watcher_);
-  w->Unref();
+  w->Stop();
   return Undefined();
 }
 
+void SignalWatcher::Stop () {
+  if (watcher_.active) {
+    ev_signal_stop(EV_DEFAULT_UC_ &watcher_);
+    Unref();
+  }
+}
+
 }  // namespace node
diff --git a/src/node_signal_watcher.h b/src/node_signal_watcher.h
index dc66e96..a875dd1 100644
--- a/src/node_signal_watcher.h
+++ b/src/node_signal_watcher.h
@@ -10,21 +10,32 @@
 
 namespace node {
 
-class SignalWatcher : EventEmitter {
+class SignalWatcher : ObjectWrap {
  public:
   static void Initialize(v8::Handle<v8::Object> target);
 
  protected:
   static v8::Persistent<v8::FunctionTemplate> constructor_template;
 
-  SignalWatcher() : EventEmitter() { }
-  ~SignalWatcher();
+  SignalWatcher(int sig) : ObjectWrap() {
+    ev_signal_init(&watcher_, SignalWatcher::Callback, sig);
+    watcher_.data = this;
+  }
+  
+  ~SignalWatcher() {
+    ev_signal_stop(EV_DEFAULT_UC_ &watcher_);
+  }
 
   static v8::Handle<v8::Value> New(const v8::Arguments& args);
+  static v8::Handle<v8::Value> Start(const v8::Arguments& args);
   static v8::Handle<v8::Value> Stop(const v8::Arguments& args);
 
  private:
-  static void OnSignal(EV_P_ ev_signal *watcher, int revents);
+  static void Callback(EV_P_ ev_signal *watcher, int revents);
+  
+  void Start();
+  void Stop();
+  
   ev_signal watcher_;
 };
 
diff --git a/test/disabled/test-signal-unregister.js b/test/disabled/test-signal-unregister.js
deleted file mode 100644
index 29ce09d..0000000
--- a/test/disabled/test-signal-unregister.js
+++ /dev/null
@@ -1,31 +0,0 @@
-require("../common");
-
-var childKilled = false, done = false,
-    spawn = require('child_process').spawn,
-    sys = require("sys"),
-    child;
-
-var join = require('path').join;
-
-child = spawn(process.argv[0], [join(fixturesDir, 'should_exit.js')]);
-child.addListener('exit', function () {
-  if (!done) childKilled = true;
-});
-
-setTimeout(function () {
-  sys.puts("Sending SIGINT");
-  child.kill("SIGINT");
-  setTimeout(function () {
-    sys.puts("Chance has been given to die");
-    done = true;
-    if (!childKilled) {
-      // Cleanup
-      sys.puts("Child did not die on SIGINT, sending SIGTERM");
-      child.kill("SIGTERM");
-    }
-  }, 200);
-}, 200);
-
-process.addListener("exit", function () {
-  assert.ok(childKilled);
-});
diff --git a/test/simple/test-signal-unregister.js b/test/simple/test-signal-unregister.js
new file mode 100644
index 0000000..29ce09d
--- /dev/null
+++ b/test/simple/test-signal-unregister.js
@@ -0,0 +1,31 @@
+require("../common");
+
+var childKilled = false, done = false,
+    spawn = require('child_process').spawn,
+    sys = require("sys"),
+    child;
+
+var join = require('path').join;
+
+child = spawn(process.argv[0], [join(fixturesDir, 'should_exit.js')]);
+child.addListener('exit', function () {
+  if (!done) childKilled = true;
+});
+
+setTimeout(function () {
+  sys.puts("Sending SIGINT");
+  child.kill("SIGINT");
+  setTimeout(function () {
+    sys.puts("Chance has been given to die");
+    done = true;
+    if (!childKilled) {
+      // Cleanup
+      sys.puts("Child did not die on SIGINT, sending SIGTERM");
+      child.kill("SIGTERM");
+    }
+  }, 200);
+}, 200);
+
+process.addListener("exit", function () {
+  assert.ok(childKilled);
+});
-- 
1.7.1

gistfile2.txt