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

now resilient to exceptions thrown from callbacks #282

Closed
wants to merge 1 commit into
base: master
from

Conversation

3 participants
@scottnonnenberg
Copy link

scottnonnenberg commented Feb 24, 2013

Because query and client both have logic they need to run, in order, after the user's callback, they are vulnerable to exceptions thrown from those callbacks. This change catches any exception thrown from user code and saves it until all necessary pg logic is complete before throwing again.

Yes, it's true that in production you should probably just restart the process if you have a top-level unhandled exception (that's what I do in production), but this is still a major pain for testing at the very least. And the existing behavior certainly prevents any kind of final app failover logic from hitting the database.

node test/integration/callback-crash-test.js can be run to verify that the changes work. Because all test frameworks install their own top-level uncaught exception handlers, this had to be a standalone file.

now resilient to exceptions thrown from callbacks
Because query and client both have logic they need to run, in order, after the user's callback, they are vulnerable to exceptions thrown from those callbacks. This change catches any exception thrown from user code and saves it until all necessary pg logic is complete before throwing again.

Yes, it's true that in production you should probably just restart the process if you have a top-level unhandled exception (that's what I do in production), but this is still a major pain for testing at the very least. And the existing behavior certainly prevents any kind of final app failover logic from hitting the database.

node test/integration/callback-crash-test.js <connectionString> can be run to verify that the changes work. Because all test frameworks install their own top-level uncaught exception handlers, this had to be a standalone file.
@brianc

This comment has been minimized.

Copy link
Owner

brianc commented Feb 25, 2013

I am not sure about this one. Node core doesn't try/catch around all of its callbacks and events. If you throw an exception in your callback its up to you to handle it. I realize some cleanup code will be skipped but it seems to be non-idiomatic to try/catch/rethrow

@scottnonnenberg

This comment has been minimized.

Copy link

scottnonnenberg commented Feb 25, 2013

I agree, this is odd code. My initial solution was to run the callback inside a process.nextTick() - that's much more idiomatic node, and resulted in the right behavior. But because all the tests are synchronous, it made them all fail. With a pretty big test overhaul, all of that post-callback logic could be wrapped in process.nextTick() as well, preserving order.

I did some thinking about why this all seems so weird to me. It's because pretty much all of the callback-related code I've seen in my experience has the callback as the last call in the function, or like 'return cb().' There's no additional code afterwards. An exception doesn't take anything down but current call tree.

Case in point: this is only situation where, in my testing, I've seen timeouts in every test after an initial failure.

@twobitfool

This comment has been minimized.

Copy link

twobitfool commented Apr 18, 2013

I ran into this issue as well. How about this: #332

@brianc

This comment has been minimized.

Copy link
Owner

brianc commented Apr 19, 2013

I went with the try/catch/rethrow approach. I did my own implementation because I wanted to make sure it was tested. You did supply tests which is great, but I've been bitten so many times by setTimeout based tests I wanted to see if I could test more deterministically.

This should be fixed in pg@1.0.2

@brianc brianc closed this Apr 19, 2013

@twobitfool

This comment has been minimized.

Copy link

twobitfool commented Apr 19, 2013

Awesome. Thanks.

@brianc

This comment has been minimized.

Copy link
Owner

brianc commented Apr 19, 2013

No prob - sorry this one took so long to get to. I wanted to have
benchmarks in place first so I could see if the try/catch would do
something weird to V8. The try/catch/rethrow makes 0 noticeable speed
impact soo that's good 👍

On Fri, Apr 19, 2013 at 9:30 AM, Matt Smith notifications@github.comwrote:

Awesome. Thanks.


Reply to this email directly or view it on GitHubhttps://github.com//pull/282#issuecomment-16656018
.

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