Skip to content

Commit

Permalink
ws, bugfix: WebSocket object in the closure was incorrectly released.
Browse files Browse the repository at this point in the history
  • Loading branch information
xicilion committed May 16, 2018
1 parent a3373d2 commit f78492f
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 3 deletions.
2 changes: 2 additions & 0 deletions fibjs/include/WebSocket.h
Expand Up @@ -110,6 +110,8 @@ class WebSocket : public WebSocket_base {

int32_t m_code;
exlib::string m_reason;

obj_ptr<ValueHolder> m_holder;
};

} /* namespace fibjs */
Expand Down
6 changes: 5 additions & 1 deletion fibjs/src/websocket/WebSocket.cpp
Expand Up @@ -311,6 +311,7 @@ result_t WebSocket_base::_new(exlib::string url, exlib::string protocol, exlib::

virtual int32_t error(int32_t v)
{
m_this->m_holder.Release();
m_this->isolate_unref();
m_this->endConnect(1002, "");
return v;
Expand All @@ -325,7 +326,7 @@ result_t WebSocket_base::_new(exlib::string url, exlib::string protocol, exlib::
};

obj_ptr<WebSocket> sock = new WebSocket(url, protocol, origin);
sock->wrap(This);
sock->m_holder = new ValueHolder(sock->wrap(This));

(new asyncConnect(sock, sock->holder()))->apost(0);

Expand All @@ -342,12 +343,15 @@ void WebSocket::startRecv(Isolate* isolate)
: AsyncState(NULL)
, m_this(pThis)
{
if (m_this->m_holder == NULL)
m_this->m_holder = new ValueHolder(m_this->wrap());
m_isolate = isolate;
set(recv);
}

~asyncRead()
{
m_this->m_holder.Release();
m_this->isolate_unref();
}

Expand Down
2 changes: 0 additions & 2 deletions fibjs/src/websocket/WebSocketHandler.cpp
Expand Up @@ -134,8 +134,6 @@ result_t WebSocketHandler::invoke(object_base* v, obj_ptr<Handler_base>& retVal,
vs[1] = pThis->m_httpreq;
pHandler->_emit("accept", vs, 2);

// sock->startRecv();

return CALL_E_PENDDING;
}

Expand Down
76 changes: 76 additions & 0 deletions test/ws_test.js
Expand Up @@ -6,6 +6,7 @@ var io = require('io');
var http = require('http');
var mq = require('mq');
var coroutine = require('coroutine');
var test_util = require('./test_util');

var base_port = coroutine.vmid * 10000;

Expand Down Expand Up @@ -763,6 +764,81 @@ describe('ws', () => {
});
});

describe('gc', () => {
it("gc on close", () => {
var t = false;
GC();

var no1 = test_util.countObject('WebSocket');
var httpd = new http.Server(8816 + base_port, {
"/ws": ws.upgrade((s, req) => {
s.onmessage = e => {}
})
});

ss.push(httpd.socket);
httpd.run(() => {});

assert.equal(test_util.countObject('WebSocket'), no1);

var s = new ws.Socket("ws://127.0.0.1:" + (8816 + base_port) + "/ws", "test");

s.onopen = () => {
t = true;
};

s.onmessage = e => {}

for (var i = 0; i < 1000 && !t; i++)
coroutine.sleep(1);

assert.equal(test_util.countObject('WebSocket'), no1 + 2);

s.close();
s = undefined;
GC();
coroutine.sleep(10);
GC();
assert.equal(test_util.countObject('WebSocket'), no1);
});

it("not gc in closure", () => {
var t = false;
GC();

var no1 = test_util.countObject('WebSocket');
var httpd = new http.Server(8817 + base_port, {
"/ws": ws.upgrade((s, req) => {
ss.push(s);
s.send(new Date());
})
});

ss.push(httpd.socket);
httpd.run(() => {});

GC();
assert.equal(test_util.countObject('WebSocket'), no1);

function test() {
var s = new ws.Socket("ws://127.0.0.1:" + (8817 + base_port) + "/ws", "test");
s.onmessage = e => {
t = true;
s.close();
}
}

test();
GC();

coroutine.sleep(10);
assert.equal(test_util.countObject('WebSocket'), no1 + 2);
assert.isTrue(t);

GC();
assert.equal(test_util.countObject('WebSocket'), no1 + 1);
});
});
});
});

Expand Down

0 comments on commit f78492f

Please sign in to comment.