search

apigee / trireme

Embed Node.js inside a Java Virtual Machine
Other
479 stars 48 forks source link

http: Remove event listeners breaking uncaught exception handling #172

Closed whitlockjc closed 7 years ago

whitlockjc commented 7 years ago

The adaptorhttp.js was previously registering process event listeners that would make it where uncaught exceptions outside of the domain-wrapped request handlers would not terminate the process as they should. This commit fixes this but DOES NOT change how requests are wrapped in domains.

whitlockjc commented 7 years ago

Basically, any time you had a script with an HTTP server listening, any process.exit or uncaught exception would be handled differently. Not only that but there was no benefit to doing it this way. This made it where an uncaught exception outside of a domain-wrapped request could not terminate the process when based on node.js, it should be.

gbrail commented 7 years ago

Can we find a test to validate this? Perhaps we can re-enable some of the tests that are disabled in node10/node10tests/simple/excluded-tests.xml starting on line 518. I'm not sure which ones, though...

On Tue, Sep 19, 2017 at 11:51 AM, Jeremy Whitlock notifications@github.com wrote:

Basically, any time you had a script with an HTTP server listening, any process.exit or uncaught exception would be handled differently. Not only that but there was no benefit to doing it this way. This made it where an uncaught exception outside of a domain-wrapped request could not terminate the process when based on node.js, it should be.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/apigee/trireme/pull/172#issuecomment-330635639, or mute the thread https://github.com/notifications/unsubscribe-auth/AAf0a0d3hrWPI2BE7wuUauuse3Upoh42ks5skA0UgaJpZM4Pc19W .

whitlockjc commented 7 years ago

I have added a test that would fail prior to these changes.

gbrail commented 7 years ago

Looks good to me -- thanks!