Skip to content

Commit

Permalink
Requiring native bindings polutes 'global' (brianc#984)
Browse files Browse the repository at this point in the history 
There was some nasty global-ish variable reference updating happening when the native module 'initializes' after its require with `require('pg').native`

This fixes the issue by making sure both `require('pg')` and `require('pg').native` each initialize their own context in isolation and no weird global-ish references are used & subsequently stomped on.
  • Loading branch information
@brianc
brianc committed Apr 8, 2016
1 parent beb8eef commit a8bd44a
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 92 deletions.
4 changes: 2 additions & 2 deletions lib/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ var Connection = require('./connection');
var PG = function(clientConstructor) {
EventEmitter.call(this);
this.defaults = defaults;
this.Client = pool.Client = clientConstructor;
this.Client = clientConstructor;
this.Query = this.Client.Query;
this.pools = pool;
this.pools = pool(clientConstructor);
this.Connection = Connection;
this.types = require('pg-types');
};
Expand Down
162 changes: 82 additions & 80 deletions lib/pool.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,93 +3,95 @@ var EventEmitter = require('events').EventEmitter;
var defaults = require('./defaults');
var genericPool = require('generic-pool');

var pools = {
//dictionary of all key:pool pairs
all: {},
//reference to the client constructor - can override in tests or for require('pg').native
Client: require('./client'),
getOrCreate: function(clientConfig) {
clientConfig = clientConfig || {};
var name = JSON.stringify(clientConfig);
var pool = pools.all[name];
if(pool) {
return pool;
}
pool = genericPool.Pool({
name: name,
max: clientConfig.poolSize || defaults.poolSize,
idleTimeoutMillis: clientConfig.poolIdleTimeout || defaults.poolIdleTimeout,
reapIntervalMillis: clientConfig.reapIntervalMillis || defaults.reapIntervalMillis,
log: clientConfig.poolLog || defaults.poolLog,
create: function(cb) {
var client = new pools.Client(clientConfig);
// Ignore errors on pooled clients until they are connected.
client.on('error', Function.prototype);
client.connect(function(err) {
if(err) return cb(err, null);

// Remove the noop error handler after a connection has been established.
client.removeListener('error', Function.prototype);
module.exports = function(Client) {
var pools = {
//dictionary of all key:pool pairs
all: {},
//reference to the client constructor - can override in tests or for require('pg').native
getOrCreate: function(clientConfig) {
clientConfig = clientConfig || {};
var name = JSON.stringify(clientConfig);
var pool = pools.all[name];
if(pool) {
return pool;
}
pool = genericPool.Pool({
name: name,
max: clientConfig.poolSize || defaults.poolSize,
idleTimeoutMillis: clientConfig.poolIdleTimeout || defaults.poolIdleTimeout,
reapIntervalMillis: clientConfig.reapIntervalMillis || defaults.reapIntervalMillis,
log: clientConfig.poolLog || defaults.poolLog,
create: function(cb) {
var client = new Client(clientConfig);
// Ignore errors on pooled clients until they are connected.
client.on('error', Function.prototype);
client.connect(function(err) {
if(err) return cb(err, null);

//handle connected client background errors by emitting event
//via the pg object and then removing errored client from the pool
client.on('error', function(e) {
pool.emit('error', e, client);
// Remove the noop error handler after a connection has been established.
client.removeListener('error', Function.prototype);

// If the client is already being destroyed, the error
// occurred during stream ending. Do not attempt to destroy
// the client again.
if (!client._destroying) {
pool.destroy(client);
}
});
//handle connected client background errors by emitting event
//via the pg object and then removing errored client from the pool
client.on('error', function(e) {
pool.emit('error', e, client);

// Remove connection from pool on disconnect
client.on('end', function(e) {
// Do not enter infinite loop between pool.destroy
// and client 'end' event...
if ( ! client._destroying ) {
// If the client is already being destroyed, the error
// occurred during stream ending. Do not attempt to destroy
// the client again.
if (!client._destroying) {
pool.destroy(client);
}
});

// Remove connection from pool on disconnect
client.on('end', function(e) {
// Do not enter infinite loop between pool.destroy
// and client 'end' event...
if ( ! client._destroying ) {
pool.destroy(client);
}
});
client.poolCount = 0;
return cb(null, client);
});
},
destroy: function(client) {
client._destroying = true;
client.poolCount = undefined;
client.end();
}
});
pools.all[name] = pool;
//mixin EventEmitter to pool
EventEmitter.call(pool);
for(var key in EventEmitter.prototype) {
if(EventEmitter.prototype.hasOwnProperty(key)) {
pool[key] = EventEmitter.prototype[key];
}
}
//monkey-patch with connect method
pool.connect = function(cb) {
var domain = process.domain;
pool.acquire(function(err, client) {
if(domain) {
cb = domain.bind(cb);
}
if(err) return cb(err, null, function() {/*NOOP*/});
client.poolCount++;
cb(null, client, function(err) {
if(err) {
pool.destroy(client);
} else {
pool.release(client);
}
});
client.poolCount = 0;
return cb(null, client);
});
},
destroy: function(client) {
client._destroying = true;
client.poolCount = undefined;
client.end();
}
});
pools.all[name] = pool;
//mixin EventEmitter to pool
EventEmitter.call(pool);
for(var key in EventEmitter.prototype) {
if(EventEmitter.prototype.hasOwnProperty(key)) {
pool[key] = EventEmitter.prototype[key];
}
};
return pool;
}
//monkey-patch with connect method
pool.connect = function(cb) {
var domain = process.domain;
pool.acquire(function(err, client) {
if(domain) {
cb = domain.bind(cb);
}
if(err) return cb(err, null, function() {/*NOOP*/});
client.poolCount++;
cb(null, client, function(err) {
if(err) {
pool.destroy(client);
} else {
pool.release(client);
}
});
});
};
return pool;
}
};
};

module.exports = pools;
return pools;
};
27 changes: 27 additions & 0 deletions test/integration/gh-issues/981-tests.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
var helper = require(__dirname + '/../test-helper');

//native bindings are only installed for native tests
if(!helper.args.native) {
return;
}

var assert = require('assert')
var pg = require('../../../lib')
var native = require('../../../lib').native

var JsClient = require('../../../lib/client')
var NativeClient = require('../../../lib/native')

assert(pg.Client === JsClient);
assert(native.Client === NativeClient);

pg.connect(function(err, client, done) {
assert(client instanceof JsClient);
client.end();

native.connect(function(err, client, done) {
assert(client instanceof NativeClient);
client.end();
});
});

15 changes: 7 additions & 8 deletions test/unit/pool/basic-tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ var util = require('util');
var EventEmitter = require('events').EventEmitter;

var libDir = __dirname + '/../../../lib';
var poolsFactory = require(libDir + '/pool')
var defaults = require(libDir + '/defaults');
var pools = require(libDir + '/pool');
var poolId = 0;

require(__dirname + '/../../test-helper');
Expand All @@ -21,6 +21,7 @@ FakeClient.prototype.connect = function(cb) {
FakeClient.prototype.end = function() {
this.endCalled = true;
}
var pools = poolsFactory(FakeClient);

//Hangs the event loop until 'end' is called on client
var HangingClient = function(config) {
Expand All @@ -41,8 +42,6 @@ HangingClient.prototype.end = function() {
clearInterval(this.intervalId);
}

pools.Client = FakeClient;

test('no pools exist', function() {
assert.empty(Object.keys(pools.all));
});
Expand Down Expand Up @@ -115,7 +114,7 @@ test('on client error, client is removed from pool', function() {
});

test('pool with connection error on connection', function() {
pools.Client = function() {
var errorPools = poolsFactory(function() {
return {
connect: function(cb) {
process.nextTick(function() {
Expand All @@ -124,9 +123,10 @@ test('pool with connection error on connection', function() {
},
on: Function.prototype
};
};
})

test('two parameters', function() {
var p = pools.getOrCreate(poolId++);
var p = errorPools.getOrCreate(poolId++);
p.connect(assert.calls(function(err, client) {
assert.ok(err);
assert.equal(client, null);
Expand All @@ -136,7 +136,7 @@ test('pool with connection error on connection', function() {
}));
});
test('three parameters', function() {
var p = pools.getOrCreate(poolId++);
var p = errorPools.getOrCreate(poolId++);
var tid = setTimeout(function() {
assert.fail('Did not call connect callback');
}, 100);
Expand All @@ -155,7 +155,6 @@ test('pool with connection error on connection', function() {

test('returnning an error to done()', function() {
var p = pools.getOrCreate(poolId++);
pools.Client = FakeClient;
p.connect(function(err, client, done) {
assert.equal(err, null);
assert(client);
Expand Down
5 changes: 3 additions & 2 deletions test/unit/pool/timeout-tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ var EventEmitter = require('events').EventEmitter;

var libDir = __dirname + '/../../../lib';
var defaults = require(libDir + '/defaults');
var pools = require(libDir + '/pool');
var poolsFactory = require(libDir + '/pool');
var poolId = 0;

require(__dirname + '/../../test-helper');
Expand All @@ -25,8 +25,9 @@ FakeClient.prototype.end = function() {
defaults.poolIdleTimeout = 10;
defaults.reapIntervalMillis = 10;

var pools = poolsFactory(FakeClient)

test('client times out from idle', function() {
pools.Client = FakeClient;
var p = pools.getOrCreate(poolId++);
p.connect(function(err, client, done) {
done();
Expand Down

0 comments on commit a8bd44a

Please sign in to comment.