Bugfix: Sockets not properly reattached if reconnected during disconnect event.

The problem was that Connection::on_close was calling Detach() directly
after executing the "disconnect" event. Since we had a boolean attach count,
this was leaving sockets detached even if they had reattached in during the
event.

 * Added many asserts in http.cc and net.cc to ensure that sockets are
   connected when they should be.

 * Changed ObjectWrap to use a reference count instead of boolean attached_
   value.

 * Fixed similar bug in Timer.

src/http.cc

@@ -79,6 +79,7 @@ HTTPConnection::NewServer (const Arguments& args)
 void
 HTTPConnection::OnReceive (const void *buf, size_t len)
 {
+  assert(attached_);
   http_parser_execute(&parser_, static_cast<const char*>(buf), len);
   if (http_parser_has_error(&parser_)) ForceClose();
 }
@@ -87,6 +88,7 @@ int
 HTTPConnection::on_message_begin (http_parser *parser)
 {
   HTTPConnection *connection = static_cast<HTTPConnection*> (parser->data);
+  assert(connection->attached_);
   connection->Emit("message_begin", 0, NULL);
   return 0;
 }
@@ -95,6 +97,7 @@ int
 HTTPConnection::on_message_complete (http_parser *parser)
 {
   HTTPConnection *connection = static_cast<HTTPConnection*> (parser->data);
+  assert(connection->attached_);
   connection->Emit("message_complete", 0, NULL);
   return 0;
 }
@@ -104,6 +107,7 @@ HTTPConnection::on_uri (http_parser *parser, const char *buf, size_t len)
 {
   HandleScope scope;
   HTTPConnection *connection = static_cast<HTTPConnection*>(parser->data);
+  assert(connection->attached_);
   Local<Value> argv[1] = { String::New(buf, len) };
   connection->Emit("uri", 1, argv);
   return 0;
@@ -114,6 +118,7 @@ HTTPConnection::on_header_field (http_parser *parser, const char *buf, size_t le
 {
   HandleScope scope;
   HTTPConnection *connection = static_cast<HTTPConnection*>(parser->data);
+  assert(connection->attached_);
   Local<Value> argv[1] = { String::New(buf, len) };
   connection->Emit("header_field", 1, argv);
   return 0;
@@ -124,6 +129,7 @@ HTTPConnection::on_header_value (http_parser *parser, const char *buf, size_t le
 {
   HandleScope scope;
   HTTPConnection *connection = static_cast<HTTPConnection*>(parser->data);
+  assert(connection->attached_);
   Local<Value> argv[1] = { String::New(buf, len) };
   connection->Emit("header_value", 1, argv);
   return 0;
@@ -155,6 +161,7 @@ int
 HTTPConnection::on_headers_complete (http_parser *parser)
 {
   HTTPConnection *connection = static_cast<HTTPConnection*> (parser->data);
+  assert(connection->attached_);
   HandleScope scope;
 
   Local<Object> message_info = Object::New();
@@ -194,6 +201,7 @@ HTTPConnection::on_body (http_parser *parser, const char *buf, size_t len)
   assert(len != 0);
 
   HTTPConnection *connection = static_cast<HTTPConnection*> (parser->data);
+  assert(connection->attached_);
   HandleScope scope;
 
   Handle<Value> argv[1];

src/net.cc

@@ -113,8 +113,8 @@ Connection::Init (void)
 Connection::~Connection ()
 {
   static int i = 0;
-  if(socket_.fd > 0) {
-    printf("garbage collecting open Connection : %d\n", i++);
+  if (socket_.fd > 0) {
+    printf("%d garbage collecting open Connection : %d\n", i++, socket_.fd);
     printf("  socket->read_action: %p\n", socket_.read_action);
     printf("  socket->write_action: %p\n", socket_.write_action);
   }
@@ -160,7 +160,8 @@ Handle<Value>
 Connection::Connect (const Arguments& args)
 {
   Connection *connection = ObjectWrap::Unwrap<Connection>(args.Holder());
-  if (!connection) return Handle<Value>();
+
+  assert(connection);
 
   HandleScope scope;
 
@@ -171,6 +172,10 @@ Connection::Connect (const Arguments& args)
     connection->Init(); // in case we're reusing the socket... ?
   }
 
+  assert(connection->socket_.fd < 0);
+  assert(connection->socket_.read_action == NULL);
+  assert(connection->socket_.write_action == NULL);
+
   if (args.Length() == 0)
     return ThrowException(String::New("Must specify a port."));
 
@@ -218,6 +223,10 @@ Connection::Resolve (eio_req *req)
 {
   Connection *connection = static_cast<Connection*> (req->data);
   struct addrinfo *address = NULL;
+
+  assert(connection->attached_);
+  assert(connection->opening);
+
   req->result = getaddrinfo(connection->host_, connection->port_, 
                             &client_tcp_hints, &address);
   req->ptr2 = address;
@@ -253,6 +262,10 @@ Connection::AfterResolve (eio_req *req)
   ev_unref(EV_DEFAULT_UC);
 
   Connection *connection = static_cast<Connection*> (req->data);
+
+  assert(connection->opening);
+  assert(connection->attached_);
+
   struct addrinfo *address = NULL,
                   *address_list = static_cast<struct addrinfo *>(req->ptr2);
 
@@ -538,8 +551,6 @@ Server::OnConnection (struct sockaddr *addr)
   Connection *connection = UnwrapConnection(js_connection);
   if (!connection) return NULL;
 
-  connection->Attach();
-
   Handle<Value> argv[1] = { js_connection };
 
   Emit("connection", 1, argv);

src/net.h

@@ -68,11 +68,13 @@ private:
   /* liboi callbacks */
   static void on_connect (evnet_socket *s) {
     Connection *connection = static_cast<Connection*> (s->data);
+    connection->Attach();
     connection->OnConnect();
   }
 
   static void on_read (evnet_socket *s, const void *buf, size_t len) {
     Connection *connection = static_cast<Connection*> (s->data);
+    assert(connection->attached_);
     if (len == 0)
       connection->OnEOF();
     else
@@ -88,11 +90,10 @@ private:
     Connection *connection = static_cast<Connection*> (s->data);
     connection->OnDisconnect();
 
-    /*
     if (s->errorno)
       printf("socket died with error %d\n", s->errorno);
-    */
 
+    assert(connection->attached_);
     connection->Detach();
   }
 

src/object_wrap.h

@@ -10,7 +10,7 @@ class ObjectWrap {
  public:
   ObjectWrap ( ) {
     weak_ = false;
-    attached_ = false;
+    attached_ = 0;
   }
 
   virtual ~ObjectWrap ( ) {
@@ -46,7 +46,7 @@ class ObjectWrap {
    */
   void Attach() {
     assert(!handle_.IsEmpty());
-    attached_ = true;
+    attached_++;
   }
   
   /* Detach() marks an object as detached from the event loop.  This is its
@@ -60,12 +60,13 @@ class ObjectWrap {
    */
   void Detach() {
     assert(!handle_.IsEmpty());
-    attached_ = false;
-    if (weak_) delete this;
+    assert(attached_ > 0);
+    attached_--;
+    if (attached_ == 0 && weak_) delete this;
   }
 
-  v8::Persistent<v8::Object> handle_;
-
+  v8::Persistent<v8::Object> handle_; // ro
+  int attached_; // ro
  private:
   static void MakeWeak (v8::Persistent<v8::Value> value, void *data) {
     ObjectWrap *obj = static_cast<ObjectWrap*>(data);
@@ -73,7 +74,6 @@ class ObjectWrap {
     obj->weak_ = true;
     if (!obj->attached_) delete obj;
   }
-  bool attached_;
   bool weak_;
 };
 

src/timer.cc

@@ -63,12 +63,7 @@ Timer::OnTimeout (EV_P_ ev_timer *watcher, int revents)
 
   timer->Emit("timeout", 0, NULL);
 
-  /* XXX i'm a bit worried if this is the correct test? 
-   * it's rather crutial for memory leaks the conditional here test to see
-   * if the watcher will make another callback.
-   */ 
-  if (!ev_is_active(&timer->watcher_))
-    timer->Detach();
+  if (timer->watcher_.repeat == 0) timer->Detach();
 }
 
 Timer::~Timer ()

test/mjsunit/test-tcp-reconnect.js

@@ -33,11 +33,13 @@ function onLoad () {
 
   client.addListener("receive", function (chunk) {
     client_recv_count += 1;
+    puts("client_recv_count " + client_recv_count);
     assertEquals("hello\r\n", chunk);
     client.fullClose();
   });
 
   client.addListener("disconnect", function (had_error) {
+    puts("disconnect");
     assertFalse(had_error);
     if (disconnect_count++ < N) 
       client.connect(port); // reconnect