-
-
Notifications
You must be signed in to change notification settings - Fork 23
Fix: Log unhandled exceptions in Node.js #30
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
Conversation
if (message !== null) { | ||
client.submitLog(BEFORE_EXIT, message, 'Error') | ||
client.submitLog(EXIT, message, 'Error') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above says only sync code can run but this will trigger a pipeline to run (various chained plugins) which will also enqueue the event). Does this work okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see, all plugins run synchronously (the run-signature gets a context object and no callback). If we ever run code here that depends on callbacks, this code would have to know the isApplicationExiting-state and run synchronously.
But as long as submitEvent(E); queue.process(); work, this should work too.
Thanks for the contribution! |
Thank you very much for the pull request! Awesome job man! I know it was a lot of hard work. I don't follow one part of of the child process workflow and was wondering if we could meet up and you could give me a quick tour. I'd just like to follow 100% how it works so I can help support it and help maintain it :). I was also wondering about your exit controller and wanted your feedback ( @frankebersoll @ejsmith). I was wondering if there is ever a time we would want more behaviors:
I'm not sure if we'd need these or not. If you can think of any other behaviors we might need to support we might want to refactor the exit controller into a behavior that covers on exit. Do you think we would ever need this, and if so do you think we should future proof it?? |
Okay, now I know what "rename branch" in Tower actually does... 😔 |
Okay, now we have:
|
port: parsedHost.port && parseInt(parsedHost.port), | ||
path: path | ||
}; | ||
private sendRequestSync(request: NodeSubmissionRequest, callback: NodeSubmissionCallback): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not an expert with Node. Guessing this is the only way to do a synchronous http request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Node.js, there is only a single thread. As long as there are any open pending callbacks (either timers or I/O), the application will continue. As soon as all callbacks in the queue are processed, the application exits.
There are exceptions to this rule: In case of an uncaught exception or a call to process.exit()
, no other handlers are processed. Instead, the process.on("exit")
handlers run, but immediately after the application will exit.
The http API in Node.js is completely asynchronous, meaning that requests are sent using a callback to wait for the response. As we can't wait for callbacks inside process.on("exit")
we have to use a child process, as spawnSync
actually allows us to wait for the child process to finish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Nice hack. :-)
- Removed node.fix.d.ts (merged in DefinitelyTyped) and updated tsconfig - Updated tsd.json for current node typings - Updated TypeScript to 1.6.2 and tsproject to 1.0.1 - Fixed lots of relative paths in import statements
- Replaced uncaught handler by shim - Added support for SIGINT
expect(response.message).toBe('Unable to connect to server.'); | ||
} | ||
var description = { | ||
email_address: '[email protected]', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noticed a typo with noreply
Ship it |
Fix: Log unhandled exceptions in Node.js
This is the first pull request for issue #29.
It adds an
IExitController
which is responsible for behavior when the application is exiting. IfisApplicationExiting
is true,NodeSubmissionClient
will use synchronous event submission. A child process will be spawned synchronously, and pipes are used to transmit the request and response data.Try it out using the following test: