New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix connection close race condition #31

Merged
merged 4 commits into from May 31, 2012

Conversation

Projects
None yet
3 participants
@robert-chiniquy
Contributor

robert-chiniquy commented May 29, 2012

Fixes race condition in PooledConnection.shutdown()

Previously, if PooledConnection.shutdown() was called before the connection process had completed,
_closeConnections would only call ConnectionInPool.close() on each connection in the pool if that
ConnectionInPool had connected == true. If the connection process was still in-progress, connected
would be set to false, close() would not be called, and the socket would later connect, causing tests
to hang.

This adds two events to ConnectionInPool, 'connected' and 'disconnected' - 'disconnected' is not
actually needed, just added for symmetry. Instead of setting ConnectionInPool.connected directly,
call setConnected() or setDisconnected() and the appropriate event will be emitted.

ConnectionInPool.closeConnections now attaches an on('connected') handler to close the connection
if the connection completes after shutdown() has been called.

robert-chiniquy added some commits May 29, 2012

Fixes race condition in PooledConnection.shutdown()
Previously, if PooledConnection.shutdown() was called before the connection process had completed,
_closeConnections would only call ConnectionInPool.close() on each connection in the pool if that
ConnectionInPool had connected == true. If the connection process was still in-progress, connected
would be set to false, close() would not be called, and the socket would later connect, causing tests
to hang.

This adds two events to ConnectionInPool, 'connected' and 'disconnected' - 'disconnected' is not
actually needed, just added for symmetry. Instead of setting ConnectionInPool.connected directly,
call setConnected() or setDisconnected() and the appropriate event will be emitted.

ConnectionInPool.closeConnections now attaches an on('connected') handler to close the connection
if the connection completes after shutdown() has been called.
Show outdated Hide outdated lib/driver.js Outdated
@@ -808,6 +824,9 @@ PooledConnection.prototype._closeConnections = function(closeCallback) {
if (con.connected) {
con.close(cb);
} else {
con.on('connected', function() {

This comment has been minimized.

@Kami

Kami May 29, 2012

Contributor

This will result in cb being called twice, right?

@Kami

Kami May 29, 2012

Contributor

This will result in cb being called twice, right?

This comment has been minimized.

@robert-chiniquy

robert-chiniquy May 29, 2012

Contributor

Nice catch, thanks.

Fixed in 5b6ff40

@robert-chiniquy

robert-chiniquy May 29, 2012

Contributor

Nice catch, thanks.

Fixed in 5b6ff40

@@ -493,14 +493,30 @@ ConnectionInPool.prototype.connect = function(callback) {
}
Connection.call(this, this._options);
Connection.prototype.connect.call(this, function(err) {
self.connected = !err;
if (err) {
self.connected = false;

This comment has been minimized.

@gdusbabek

gdusbabek May 30, 2012

Contributor

why not self.setDisconnected()?

@gdusbabek

gdusbabek May 30, 2012

Contributor

why not self.setDisconnected()?

This comment has been minimized.

@robert-chiniquy
@robert-chiniquy

robert-chiniquy May 30, 2012

Contributor
var pool = new PooledConnection({'hosts': hosts, 'keyspace': 'Keyspace1'});
pool.connect();
pool.shutdown(function(err) {

This comment has been minimized.

@gdusbabek

gdusbabek May 30, 2012

Contributor

Would this test have failed outright previously or just timed out?

@gdusbabek

gdusbabek May 30, 2012

Contributor

Would this test have failed outright previously or just timed out?

This comment has been minimized.

@robert-chiniquy

robert-chiniquy May 30, 2012

Contributor

It times out. Wasn't sure how to make it fail outright.

@robert-chiniquy

robert-chiniquy May 30, 2012

Contributor

It times out. Wasn't sure how to make it fail outright.

gdusbabek added a commit that referenced this pull request May 31, 2012

Merge pull request #31 from robert-chiniquy/fix-connection-close-race…
…-condition

Fix connection close race condition

@gdusbabek gdusbabek merged commit 7eedb98 into racker:master May 31, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment