From 041af82b8c4d7655ee238e09259c4a69ed296583 Mon Sep 17 00:00:00 2001 From: Ryan Date: Sun, 12 Jul 2009 15:02:13 +0200 Subject: [PATCH] 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 | 8 ++++++++ src/net.cc | 21 ++++++++++++++++----- src/net.h | 5 +++-- src/object_wrap.h | 14 +++++++------- src/timer.cc | 7 +------ test/mjsunit/test-tcp-reconnect.js | 2 ++ 6 files changed, 37 insertions(+), 20 deletions(-) diff --git a/src/http.cc b/src/http.cc index 958ff991ef..60cfda38c3 100644 --- a/src/http.cc +++ b/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(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 (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 (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(parser->data); + assert(connection->attached_); Local 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(parser->data); + assert(connection->attached_); Local 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(parser->data); + assert(connection->attached_); Local 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 (parser->data); + assert(connection->attached_); HandleScope scope; Local 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 (parser->data); + assert(connection->attached_); HandleScope scope; Handle argv[1]; diff --git a/src/net.cc b/src/net.cc index 0eeb6265f6..de1c3ac59f 100644 --- a/src/net.cc +++ b/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 Connection::Connect (const Arguments& args) { Connection *connection = ObjectWrap::Unwrap(args.Holder()); - if (!connection) return Handle(); + + 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 (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 (req->data); + + assert(connection->opening); + assert(connection->attached_); + struct addrinfo *address = NULL, *address_list = static_cast(req->ptr2); @@ -538,8 +551,6 @@ Server::OnConnection (struct sockaddr *addr) Connection *connection = UnwrapConnection(js_connection); if (!connection) return NULL; - connection->Attach(); - Handle argv[1] = { js_connection }; Emit("connection", 1, argv); diff --git a/src/net.h b/src/net.h index 2862d64d85..02e53a3644 100644 --- a/src/net.h +++ b/src/net.h @@ -68,11 +68,13 @@ private: /* liboi callbacks */ static void on_connect (evnet_socket *s) { Connection *connection = static_cast (s->data); + connection->Attach(); connection->OnConnect(); } static void on_read (evnet_socket *s, const void *buf, size_t len) { Connection *connection = static_cast (s->data); + assert(connection->attached_); if (len == 0) connection->OnEOF(); else @@ -88,11 +90,10 @@ private: Connection *connection = static_cast (s->data); connection->OnDisconnect(); - /* if (s->errorno) printf("socket died with error %d\n", s->errorno); - */ + assert(connection->attached_); connection->Detach(); } diff --git a/src/object_wrap.h b/src/object_wrap.h index 4fcc304acd..362dd6af50 100644 --- a/src/object_wrap.h +++ b/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 handle_; - + v8::Persistent handle_; // ro + int attached_; // ro private: static void MakeWeak (v8::Persistent value, void *data) { ObjectWrap *obj = static_cast(data); @@ -73,7 +74,6 @@ class ObjectWrap { obj->weak_ = true; if (!obj->attached_) delete obj; } - bool attached_; bool weak_; }; diff --git a/src/timer.cc b/src/timer.cc index 14be30dea2..81f1d5375e 100644 --- a/src/timer.cc +++ b/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 () diff --git a/test/mjsunit/test-tcp-reconnect.js b/test/mjsunit/test-tcp-reconnect.js index dcdf4363df..b2ba3d4ac9 100644 --- a/test/mjsunit/test-tcp-reconnect.js +++ b/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