Throwing in a callback should kill the process

There is a difference between errors which happen to a socket - like
receiving EPIPE - an exceptional situation but ultimately okay and the
situation where code throws in a callback - which is not okay.

Fixes test/simple/test-http-exceptions.js

TODO: explain this in docs.

lib/http.js

@@ -602,8 +602,11 @@ function connectionListener (socket) {
   });
 
   socket.ondata = function (d, start, end) {
-    var bytesParsed = parser.execute(d, start, end - start);
-    if (parser.incoming && parser.incoming.upgrade) {
+    var ret = parser.execute(d, start, end - start);
+    if (ret instanceof Error) {
+      socket.destroy(ret);
+    } else if (parser.incoming && parser.incoming.upgrade) {
+      var bytesParsed = ret;
       socket.ondata = null;
       socket.onend = null;
 
@@ -624,6 +627,7 @@ function connectionListener (socket) {
 
   socket.onend = function () {
     parser.finish();
+
     // unref the parser for easy gc
     parsers.free(parser);
 
@@ -703,8 +707,11 @@ function Client ( ) {
     if (!parser) {
       throw new Error("parser not initialized prior to Client.ondata call");
     }
-    var bytesParsed = parser.execute(d, start, end - start);
-    if (parser.incoming && parser.incoming.upgrade) {
+    var ret = parser.execute(d, start, end - start);
+    if (ret instanceof Error) {
+      self.destroy(ret);
+    } else if (parser.incoming && parser.incoming.upgrade) {
+      var bytesParsed = ret;
       var upgradeHead = d.slice(start + bytesParsed, end - start);
       parser.incoming.upgradeHead = upgradeHead;
       currentRequest.emit("response", parser.incoming);
@@ -734,7 +741,6 @@ function Client ( ) {
 
   self.onend = function () {
     parser.finish();
-
     debug("self got end closing. readyState = " + self.readyState);
     self.end();
   };

lib/net.js

@@ -266,13 +266,8 @@ function _doFlush () {
   // Stream becomes writeable on connect() but don't flush if there's
   // nothing actually to write
   if (socket.flush()) {
-    try {
     if (socket._events && socket._events['drain']) socket.emit("drain");
     if (socket.ondrain) socket.ondrain(); // Optimization
-    } catch (e) {
-      socket.destroy(e);
-      return;
-    }
   }
 }
 
@@ -358,13 +353,8 @@ function initStream (self) {
       if (!self.writable) self.destroy();
       // Note: 'close' not emitted until nextTick.
 
-      try {
       if (self._events && self._events['end']) self.emit('end');
       if (self.onend) self.onend();
-      } catch (e) {
-        self.destroy(e);
-        return;
-      }
     } else if (bytesRead > 0) {
 
       timeout.active(self);
@@ -373,7 +363,6 @@ function initStream (self) {
       var end = pool.used + bytesRead;
       pool.used += bytesRead;
 
-      try {
       if (!self._encoding) {
         if (self._events && self._events['data']) {
           // emit a slice
@@ -388,10 +377,6 @@ function initStream (self) {
         var string = pool.toString(self._encoding, start, end);
         self.emit('data', string);
       }
-      } catch (e) {
-        self.destroy(e);
-        return;
-      }
     }
   };
   self.readable = false;

src/node_http_parser.cc

@@ -70,7 +70,12 @@ static struct http_parser_settings settings;
     if (!cb_value->IsFunction()) return 0;                               \
     Local<Function> cb = Local<Function>::Cast(cb_value);                \
     Local<Value> ret = cb->Call(parser->handle_, 0, NULL);               \
-    return ret.IsEmpty() ? -1 : 0;                                       \
+    if (ret.IsEmpty()) {                                                 \
+      parser->got_exception_ = true;                                     \
+      return -1;                                                         \
+    } else {                                                             \
+      return 0;                                                          \
+    }                                                                    \
   }
 
 // Callback prototype for http_data_cb
@@ -87,7 +92,12 @@ static struct http_parser_settings settings;
                            };                                            \
     Local<Value> ret = cb->Call(parser->handle_, 3, argv);               \
     assert(parser->buffer_);                                             \
-    return ret.IsEmpty() ? -1 : 0;                                       \
+    if (ret.IsEmpty()) {                                                 \
+      parser->got_exception_ = true;                                     \
+      return -1;                                                         \
+    } else {                                                             \
+      return 0;                                                          \
+    }                                                                    \
   }
 
 
@@ -169,7 +179,12 @@ class Parser : public ObjectWrap {
 
     Local<Value> ret = cb->Call(parser->handle_, 1, argv);
 
-    return ret.IsEmpty() ? -1 : 0;
+    if (ret.IsEmpty()) {
+      parser->got_exception_ = true;
+      return -1;
+    } else {
+      return 0;
+    }
   }
 
   static Handle<Value> New(const Arguments& args) {
@@ -225,10 +240,9 @@ class Parser : public ObjectWrap {
             String::New("Length is extends beyond buffer")));
     }
 
-    TryCatch try_catch;
-
     // Assign 'buffer_' while we parse. The callbacks will access that varible.
     parser->buffer_ = buffer;
+    parser->got_exception_ = false;
 
     size_t nparsed =
       http_parser_execute(&parser->parser_, settings, buffer->data()+off, len);
@@ -238,21 +252,20 @@ class Parser : public ObjectWrap {
     parser->buffer_ = NULL;
 
     // If there was an exception in one of the callbacks
-    if (try_catch.HasCaught()) return try_catch.ReThrow();
+    if (parser->got_exception_) return Local<Value>();
 
     Local<Integer> nparsed_obj = Integer::New(nparsed);
     // If there was a parse error in one of the callbacks 
     // TODO What if there is an error on EOF?
     if (!parser->parser_.upgrade && nparsed != len) {
-      Local<Value> e = Exception::Error(String::New("Parse Error"));
+      Local<Value> e = Exception::Error(String::NewSymbol("Parse Error"));
       Local<Object> obj = e->ToObject();
       obj->Set(String::NewSymbol("bytesParsed"), nparsed_obj);
-      return ThrowException(e);
-    }
-
-    assert(!parser->buffer_);
+      return scope.Close(e);
+    } else {
       return scope.Close(nparsed_obj);
     }
+  }
 
   static Handle<Value> Finish(const Arguments& args) {
     HandleScope scope;
@@ -260,9 +273,12 @@ class Parser : public ObjectWrap {
     Parser *parser = ObjectWrap::Unwrap<Parser>(args.This());
 
     assert(!parser->buffer_);
+    parser->got_exception_ = false;
 
     http_parser_execute(&(parser->parser_), settings, NULL, 0);
 
+    if (parser->got_exception_) return Local<Value>();
+
     return Undefined();
   }
 
@@ -294,6 +310,7 @@ class Parser : public ObjectWrap {
   }
 
   Buffer * buffer_;  // The buffer currently being parsed.
+  bool got_exception_;
   http_parser parser_;
 };
 

test/simple/test-http-upgrade2.js

@@ -10,19 +10,21 @@ server = http.createServer(function (req, res) {
 
 server.addListener('upgrade', function (req, socket, upgradeHead) {
   error('got upgrade event');
-  // test that throwing an error from upgrade gets forworded
-  // to the server'server 'error' event.
+  // test that throwing an error from upgrade gets 
+  // is uncaught
   throw new Error('upgrade error');
 });
 
 gotError = false;
 
-server.addListener('clientError', function (e) {
+process.addListener('uncaughtException', function (e) {
   error('got "clientError" event');
   assert.equal('upgrade error', e.message);
   gotError = true;
+  process.exit(0);
 });
 
+
 server.listen(PORT);