Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: brianc/node-postgres
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: master
Choose a base ref
...
head repository: yocontra/node-postgres
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: master
Choose a head ref
  • 13 commits
  • 8 files changed
  • 3 contributors

Commits on Nov 20, 2017

  1. Copy the full SHA
    7edfdf7 View commit details
  2. Ensure callbacks are executed for all queued queries after connection…

    …-level errors
    
    Separates socket errors from error messages, sends socket errors to all queries in the queue, marks clients as unusable after socket errors.
    
    This is not very pleasant but should maintain backwards compatibility…?
    charmander committed Nov 20, 2017
    Copy the full SHA
    6cba93d View commit details
  3. Always call handleError asynchronously

    This doesn’t match the original behaviour of the type errors, but it’s correct.
    charmander committed Nov 20, 2017
    Copy the full SHA
    7b6b7a1 View commit details
  4. Copy the full SHA
    fdf5a4a View commit details
  5. Copy the full SHA
    57bd144 View commit details

Commits on Feb 12, 2018

  1. Handle SSL negotiation errors more robustly

    This commit adds some finer grained detail to handling the postmaster's
    response to SSL negotiation packets, by accounting for the possibility
    of an 'E' byte being sent back, and emitting an appropriate error.
    
    In the naive case, the postmaster will respond with either 'S' (proceed
    with an SSL connection) or 'N' (SSL is not supported). However, the
    current if statement doesn't account for an 'E' byte being returned
    by the postmaster, where an error is encountered (perhaps unable to
    fork due to being out of memory).
    
    By adding this case, we can prevent confusing error messages when SSL is
    enforced and the postmaster returns an error after successful SSL
    connections.
    
    This also brings the connection handling further in line with
    libpq, where 'E' is handled similarly as of this commit:
    
    postgres/postgres@a49fbaa
    
    Given that there are no longer pre-7.0 databases out in the wild, I
    believe this is a safe change to make, and should not break backwards
    compatibility (unless matching on error message content).
    
    * Replace if statement with switch, to catch 'S', 'E' and 'N' bytes
      returned by the postmaster
    * Return an Error for non 'S' or 'N' cases
    * Expand and restructure unit tests for SSL negotiation packets
    mble committed Feb 12, 2018
    Copy the full SHA
    b75adc6 View commit details

Commits on Feb 14, 2018

  1. Copy the full SHA
    f1f3eb5 View commit details

Commits on Feb 16, 2018

  1. Copy the full SHA
    fcfacbd View commit details

Commits on Feb 22, 2018

  1. Copy the full SHA
    66d0581 View commit details

Commits on Apr 2, 2018

  1. Copy the full SHA
    eb77592 View commit details
  2. Copy the full SHA
    26f95f3 View commit details
  3. Copy the full SHA
    03eed3e View commit details
  4. Copy the full SHA
    91a3871 View commit details
Showing with 182 additions and 66 deletions.
  1. +60 −10 lib/client.js
  2. +9 −3 lib/connection.js
  3. +3 −8 lib/query.js
  4. +11 −24 lib/result.js
  5. +0 −1 package.json
  6. +12 −0 test/integration/client/error-handling-tests.js
  7. +43 −2 test/integration/connection-pool/error-tests.js
  8. +44 −18 test/unit/connection/error-tests.js
70 changes: 60 additions & 10 deletions lib/client.js
Original file line number Diff line number Diff line change
@@ -36,6 +36,7 @@ var Client = function (config) {
this._connecting = false
this._connected = false
this._connectionError = false
this._queryable = true

this.connection = c.connection || new Connection({
stream: c.stream,
@@ -126,15 +127,39 @@ Client.prototype.connect = function (callback) {
}

const connectedErrorHandler = (err) => {
this._queryable = false

const enqueueError = (query) => {
process.nextTick(() => {
query.handleError(err, con)
})
}

if (this.activeQuery) {
var activeQuery = self.activeQuery
enqueueError(this.activeQuery)
this.activeQuery = null
return activeQuery.handleError(err, con)
}

this.queryQueue.forEach(enqueueError)
this.queryQueue = []

this.emit('error', err)
}

const connectedErrorMessageHandler = (msg) => {
const activeQuery = this.activeQuery

if (!activeQuery) {
connectedErrorHandler(msg)
return
}

this.activeQuery = null
activeQuery.handleError(msg, con)
}

con.on('error', connectingErrorHandler)
con.on('errorMessage', connectingErrorHandler)

// hook up query handling events to connection
// after the connection initially becomes ready for queries
@@ -143,7 +168,9 @@ Client.prototype.connect = function (callback) {
self._connected = true
self._attachListeners(con)
con.removeListener('error', connectingErrorHandler)
con.removeListener('errorMessage', connectingErrorHandler)
con.on('error', connectedErrorHandler)
con.on('errorMessage', connectedErrorMessageHandler)

// process possible callback argument to Client#connect
if (callback) {
@@ -181,10 +208,10 @@ Client.prototype.connect = function (callback) {
if (callback) {
callback(error)
} else {
this.emit('error', error)
connectedErrorHandler(error)
}
} else if (!this._connectionError) {
this.emit('error', error)
connectedErrorHandler(error)
}
}
this.emit('end')
@@ -353,7 +380,15 @@ Client.prototype._pulseQueryQueue = function () {
if (this.activeQuery) {
this.readyForQuery = false
this.hasExecuted = true
this.activeQuery.submit(this.connection)

const queryError = this.activeQuery.submit(this.connection)
if (queryError) {
process.nextTick(() => {
this.activeQuery.handleError(queryError, this.connection)
this.readyForQuery = true
this._pulseQueryQueue()
})
}
} else if (this.hasExecuted) {
this.activeQuery = null
this.emit('drain')
@@ -389,25 +424,40 @@ Client.prototype.query = function (config, values, callback) {
query._result._getTypeParser = this._types.getTypeParser.bind(this._types)
}

if (!this._queryable) {
process.nextTick(() => {
query.handleError(new Error('Client has encountered a connection error and is not queryable'), this.connection)
})
return result
}

if (this._ending) {
process.nextTick(() => {
query.handleError(new Error('Client was closed and is not queryable'), this.connection)
})
return result
}

this.queryQueue.push(query)
this._pulseQueryQueue()
return result
}

Client.prototype.end = function (cb) {
this._ending = true

if (this.activeQuery) {
// if we have an active query we need to force a disconnect
// on the socket - otherwise a hung query could block end forever
this.connection.stream.destroy(new Error('Connection terminated by user'))
return cb ? cb() : Promise.resolve()
this.connection.stream.destroy()
} else {
this.connection.end()
}

if (cb) {
this.connection.end()
this.connection.once('end', cb)
} else {
return new global.Promise((resolve, reject) => {
this.connection.end()
return new Promise((resolve) => {
this.connection.once('end', resolve)
})
}
12 changes: 9 additions & 3 deletions lib/connection.js
Original file line number Diff line number Diff line change
@@ -82,8 +82,13 @@ Connection.prototype.connect = function (port, host) {

this.stream.once('data', function (buffer) {
var responseCode = buffer.toString('utf8')
if (responseCode !== 'S') {
return self.emit('error', new Error('The server does not support SSL connections'))
switch (responseCode) {
case 'N': // Server does not support SSL connections
return self.emit('error', new Error('The server does not support SSL connections'))
case 'S': // Server supports SSL connections, continue with a secure connection
break
default: // Any other response byte, including 'E' (ErrorResponse) indicating a server error
return self.emit('error', new Error('There was an error establishing an SSL connection'))
}
var tls = require('tls')
self.stream = tls.connect({
@@ -111,10 +116,11 @@ Connection.prototype.attachListeners = function (stream) {
var packet = self._reader.read()
while (packet) {
var msg = self.parseMessage(packet)
var eventName = msg.name === 'error' ? 'errorMessage' : msg.name
if (self._emitMessage) {
self.emit('message', msg)
}
self.emit(msg.name, msg)
self.emit(eventName, msg)
packet = self._reader.read()
}
})
11 changes: 3 additions & 8 deletions lib/query.js
Original file line number Diff line number Diff line change
@@ -147,22 +147,17 @@ Query.prototype.handleError = function (err, connection) {

Query.prototype.submit = function (connection) {
if (typeof this.text !== 'string' && typeof this.name !== 'string') {
const err = new Error('A query must have either text or a name. Supplying neither is unsupported.')
connection.emit('error', err)
connection.emit('readyForQuery')
return
return new Error('A query must have either text or a name. Supplying neither is unsupported.')
}
if (this.values && !Array.isArray(this.values)) {
const err = new Error('Query values must be an array')
connection.emit('error', err)
connection.emit('readyForQuery')
return
return new Error('Query values must be an array')
}
if (this.requiresPreparation()) {
this.prepare(connection)
} else {
connection.query(this.text)
}
return null
}

Query.prototype.hasBeenParsed = function (connection) {
35 changes: 11 additions & 24 deletions lib/result.js
Original file line number Diff line number Diff line change
@@ -8,7 +8,6 @@
*/

var types = require('pg-types')
var escape = require('js-string-escape')

// result object returned from query
// in the 'end' event and also
@@ -65,29 +64,24 @@ Result.prototype._parseRowAsArray = function (rowData) {
return row
}

// rowData is an array of text or binary values
// this turns the row into a JavaScript object
Result.prototype.parseRow = function (rowData) {
return new this.RowCtor(this._parsers, rowData)
var row = {}
for (var i = 0, len = rowData.length; i < len; i++) {
var rawValue = rowData[i]
var field = this.fields[i].name
if (rawValue !== null) {
row[field] = this._parsers[i](rawValue)
} else {
row[field] = null
}
}
return row
}

Result.prototype.addRow = function (row) {
this.rows.push(row)
}

var inlineParser = function (fieldName, i) {
return "\nthis['" +
// fields containing single quotes will break
// the evaluated javascript unless they are escaped
// see https://github.com/brianc/node-postgres/issues/507
// Addendum: However, we need to make sure to replace all
// occurences of apostrophes, not just the first one.
// See https://github.com/brianc/node-postgres/issues/934
escape(fieldName) +
"'] = " +
'rowData[' + i + '] == null ? null : parsers[' + i + '](rowData[' + i + ']);'
}

Result.prototype.addFields = function (fieldDescriptions) {
// clears field definitions
// multiple query statements in 1 action can result in multiple sets
@@ -97,18 +91,11 @@ Result.prototype.addFields = function (fieldDescriptions) {
this.fields = []
this._parsers = []
}
var ctorBody = ''
for (var i = 0; i < fieldDescriptions.length; i++) {
var desc = fieldDescriptions[i]
this.fields.push(desc)
var parser = this._getTypeParser(desc.dataTypeID, desc.format || 'text')
this._parsers.push(parser)
// this is some craziness to compile the row result parsing
// results in ~60% speedup on large query result sets
ctorBody += inlineParser(desc.name, i)
}
if (!this.rowAsArray) {
this.RowCtor = Function('parsers', 'rowData', ctorBody)
}
}

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -20,7 +20,6 @@
"main": "./lib",
"dependencies": {
"buffer-writer": "1.0.1",
"js-string-escape": "1.0.1",
"packet-reader": "0.3.1",
"pg-connection-string": "0.1.3",
"pg-pool": "~2.0.3",
12 changes: 12 additions & 0 deletions test/integration/client/error-handling-tests.js
Original file line number Diff line number Diff line change
@@ -50,6 +50,18 @@ suite.test('re-using connections results in promise rejection', (done) => {
})
})

suite.test('using a client after closing it results in error', (done) => {
const client = new Client()
client.connect(() => {
client.end(assert.calls(() => {
client.query('SELECT 1', assert.calls((err) => {
assert.equal(err.message, 'Client was closed and is not queryable')
done()
}))
}))
})
})

suite.test('query receives error on client shutdown', function (done) {
var client = new Client()
client.connect(assert.success(function () {
45 changes: 43 additions & 2 deletions test/integration/connection-pool/error-tests.js
Original file line number Diff line number Diff line change
@@ -5,15 +5,14 @@ const pg = helper.pg
// first make pool hold 2 clients
pg.defaults.poolSize = 2

const pool = new pg.Pool()

const suite = new helper.Suite()
suite.test('connecting to invalid port', (cb) => {
const pool = new pg.Pool({ port: 13801 })
pool.connect().catch(e => cb())
})

suite.test('errors emitted on pool', (cb) => {
const pool = new pg.Pool()
// get first client
pool.connect(assert.success(function (client, done) {
client.id = 1
@@ -48,3 +47,45 @@ suite.test('errors emitted on pool', (cb) => {
})
}))
})

suite.test('connection-level errors cause queued queries to fail', (cb) => {
const pool = new pg.Pool()
pool.connect(assert.success((client, done) => {
client.query('SELECT pg_terminate_backend(pg_backend_pid())', assert.calls((err) => {
assert.equal(err.code, '57P01')
}))

pool.once('error', assert.calls((err, brokenClient) => {
assert.equal(client, brokenClient)
}))

client.query('SELECT 1', assert.calls((err) => {
assert.equal(err.message, 'Connection terminated unexpectedly')

done()
pool.end()
cb()
}))
}))
})

suite.test('connection-level errors cause future queries to fail', (cb) => {
const pool = new pg.Pool()
pool.connect(assert.success((client, done) => {
client.query('SELECT pg_terminate_backend(pg_backend_pid())', assert.calls((err) => {
assert.equal(err.code, '57P01')
}))

pool.once('error', assert.calls((err, brokenClient) => {
assert.equal(client, brokenClient)

client.query('SELECT 1', assert.calls((err) => {
assert.equal(err.message, 'Client has encountered a connection error and is not queryable')

done()
pool.end()
cb()
}))
}))
}))
})
62 changes: 44 additions & 18 deletions test/unit/connection/error-tests.js
Original file line number Diff line number Diff line change
@@ -37,26 +37,52 @@ suite.test('connection does not emit ECONNRESET errors during disconnect', funct
done()
})

var SSLNegotiationPacketTests = [
{
testName: 'connection does not emit ECONNRESET errors during disconnect also when using SSL',
errorMessage: null,
response: 'S',
responseType: 'sslconnect'
},
{
testName: 'connection emits an error when SSL is not supported',
errorMessage: 'The server does not support SSL connections',
response: 'N',
responseType: 'error'
},
{
testName: 'connection emits an error when postmaster responds to SSL negotiation packet',
errorMessage: 'There was an error establishing an SSL connection',
response: 'E',
responseType: 'error'
}
]

suite.test('connection does not emit ECONNRESET errors during disconnect also when using SSL', function (done) {
// our fake postgres server, which just responds with 'S' to start SSL
var socket
var server = net.createServer(function (c) {
socket = c
c.once('data', function (data) {
c.write(Buffer.from('S'))
for (var i = 0; i < SSLNegotiationPacketTests.length; i++) {
var tc = SSLNegotiationPacketTests[i]
suite.test(tc.testName, function (done) {
// our fake postgres server
var socket
var server = net.createServer(function (c) {
socket = c
c.once('data', function (data) {
c.write(Buffer.from(tc.response))
})
})
})

server.listen(7778, function () {
var con = new Connection({ssl: true})
con.connect(7778, 'localhost')
assert.emits(con, 'sslconnect', function () {
con.end()
socket.destroy()
server.close()
done()
server.listen(7778, function () {
var con = new Connection({ssl: true})
con.connect(7778, 'localhost')
assert.emits(con, tc.responseType, function (err) {
if (tc.errorMessage !== null || err) {
assert.equal(err.message, tc.errorMessage)
}
con.end()
socket.destroy()
server.close()
done()
})
con.requestSsl()
})
con.requestSsl()
})
})
}