feat: provide callback to uncaughtException handler #1766
Conversation
misterdjules
changed the title
|
@cprussin I think this approach looks good to me. Two things I'd like to see as part of this PR and that are currently missing:
|
test/server.test.js
Outdated
| t.end(); | ||
| }); | ||
|
|
||
| SERVER.on('uncaughtException', function(req, res, route, err, next) { |
There was a problem hiding this comment.
In the documentation, I would suggest not calling that callback next, so that users don't confuse this callback with middlewares' next callbacks and their specific semantics/API.
test/server.test.js
Outdated
| // client to timeout | ||
| setTimeout(function() { | ||
| throw new Error('foo'); | ||
| }, 800); |
There was a problem hiding this comment.
Can we rely on a 'timeout' event on FAST_CLIENT instead of a timer duration to make sure that we throw after the client times out and abort the request? Or maybe even relying on an abort event on the client would be better? Or even better on the 'abort' event of the request object?
Otherwise it seems this test could be broken easily by changes in the client's implementation details.
There was a problem hiding this comment.
Could do, I mostly copied this pattern from existing tests so I'm not sure if you'd prefer consistency with those tests over stability for this test?
There was a problem hiding this comment.
I prefer stability here. Your stable test can act as a good example of how to correctly write tests and that pattern can then be used for future tests. I think changing the other tests to follow your pattern of stable tests can also be a good first issue for new contributors.
There was a problem hiding this comment.
@cprussin @rajatkumar I refactored the test so that it doesn't rely on any arbitrary timeout, but instead relies on the actual state of the request/response lifecycle. Let me know if that looks good to you!
| res, | ||
| req.route, | ||
| err, | ||
| function uncaughtExceptionCompleted() { |
There was a problem hiding this comment.
Please put a comment about the why here and describe what will happen with and without domains.
There was a problem hiding this comment.
@hekike @rajatkumar Added some comments here, let me know if that addresses your feedback!
| res, | ||
| req.route, | ||
| err, | ||
| function uncaughtExceptionCompleted() { |
|
@cprussin @rajatkumar @hekike Updated this PR with a few more commits that I believe address all the comments in this PR. Please take another look! |
Pre-Submission Checklist
make prepushIssues
Closes:
The 'after' event is not called when a request has an uncaught exception (using
handleUncaughtExceptions) if the request has been aborted before the exception.Changes
This PR is a proposal for a mechanism to fix the issue by adding a continuation function to the
uncaughtExceptionhandler. If this continuation function isn't called, then restify can never know that a request has completed all work and call the 'after' handler (becauseabortedis triggered on the request object before work has completed, so neitherfinishnorcloseever get triggered on the response and thus we have no other signal that work has completed). This is actually consistent with how middlewares work--ifnextisn't called then the accounting will leak--so I believe the tradeoff is acceptable.This pattern also allows for a fix without making a breaking API change.