Skip to content

prevent exceptions in callback from locking up the postgres client - #332

Closed
twobitfool wants to merge 1 commit into
brianc:masterfrom
twobitfool:master
Closed

prevent exceptions in callback from locking up the postgres client#332
twobitfool wants to merge 1 commit into
brianc:masterfrom
twobitfool:master

Conversation

@twobitfool

Copy link
Copy Markdown

When the code in the callback raises an exception (e.g. an assert in test code) the next query will never fire. This prevents that issue by firing the callback on nextTick.

Note: Tests lifted from https://github.com/NiKnight/node-postgres/commit/d4b99763bc7760bf9add414da2d38510596c69df on #282

@brianc

brianc commented Apr 19, 2013

Copy link
Copy Markdown
Owner

When the query emits 'end' the client which owns the query checks it's internal queue of queries to see about issuing another one. If the client has no more queries it will emit a 'drain'. This would have been a much more difficult problem in the past with the weird pauseDrain and resumeDrain stuff I recently removed...but I still think this might introduce weird timing bugs? Basically if you issue a second query to the same client from the callback of the first query the client will have emitted the 'drain' event in between. For example...this:

var client = new Client('blalba');

//automatically close the client when its empty
client.on('drain', client.end.bind(client));

client.query('SELECT * FROM whatever', function() {
  //client has at this point emitted drain and is in the process of closing
  //so this next query will error out
  client.query('SELECT * FROM something_else', function() {

  });
});

I'm not saying the above code example is good code, but I'm using it to demonstrate moving the callback into process.nextTick introduces some subtle backwards compatibile breaks. There are a few options...

  1. if your code throws an exception according to node you're supposed to consider the process in an "undetermined state" and close it ASAP. Since your test threw an exception...it failed...so...

  2. Run the pulseQueryQueue within the client also in a process.nextTick. I'm wary this not introducing other subtle timing bugs.

  3. Put a try/catch/rethrow around the callback. I will have to benchmark this to see the perf impact. Not entirely out of the question, but feels dirty.

@twobitfool

Copy link
Copy Markdown
Author

I kind of like the "dirty" try/catch idea. It's direct, and it may perform better than putting the callback and pulseQueryQueue on nextTick. Plus, it seems to do a better job (than nextTick) of expressing the intent: to protect the rest of the code from an exception in the callback.

@brianc

brianc commented Apr 19, 2013

Copy link
Copy Markdown
Owner

fixed in pg@1.0.2

Thanks for your code - normally I accept & don't reimplement but there were some tweaks I wanted to put in.

@brianc brianc closed this Apr 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants