Skip to content
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

12.16/13.8: http response listener throwing does not result in emit of uncaughtException #31796

Closed
michaelgoin opened this issue Feb 14, 2020 · 6 comments
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem.

Comments

@michaelgoin
Copy link

  • Version: 12.16.0 | 13.8.0
  • Platform: Darwin Kernel Version 18.6.0
  • Subsystem:

What steps will reproduce the bug?

Throwing from a response listener (callback) to http.get() will not trigger process.once('uncaughtException', () => {}). Interestingly, throwing from a request listener (callback) to http.createServer() will still.

This behavior changed with 12.16.0, I'm guessing likely due to the porting of the emit changes?

const http = require('http')

let server
let request

process.once('uncaughtException', function() {
  // never gets here from response listener in 12.16, works fine < 12.16.
  console.log('in uncaughtException handler')

  server.close(done)
})

server = http.createServer(function cb_createServer(request, response) {
  // Throw from request listener will result in uncaughtException
  //throw new Error('wat')
  response.writeHead(200, {'Content-Type': 'text/plain'})
  response.end()
})

server.listen(8183, function() {
  request = http.get({host: 'localhost', port: 8183}, function() {
    // Throw from response listener will not result in uncaughtException
    throw new Error('whoah')
  })
})

How often does it reproduce? Is there a required condition?

Consistently does not trigger uncaughtException / does not allow handling via process.on('uncaughtException', ...).

What is the expected behavior?

Should be able to notice the uncaught exception thrown from the handler.

What do you see instead?

Additional information

@addaleax
Copy link
Member

Is there anything else you’re doing besides running the above script with node <filename> to reproduce? It works for me on x64 Linux (after removing the undefined done callback) on the versions you provided.

@addaleax addaleax added the http Issues or PRs related to the http subsystem. label Feb 14, 2020
@michaelgoin
Copy link
Author

Well that's embarrassing. Let me dig around a bit. It was failing in tests and then I ported out and ran via debugger and saw the same issues, but admittedly didn't run straight via script at that point.

@michaelgoin
Copy link
Author

I think I let myself get tricked by the debugger. I'm back to thinking it is something we are doing that is incompatible with the new 12.16 code. Going to go ahead and close this out, can always re-open if proves real.

@michaelgoin
Copy link
Author

Looks like something about creating an async hook is causing this.

@michaelgoin
Copy link
Author

Specifically, looks like the existance of an after handler that changes the overall behavior.

Is this expected?

Adding this to the top results in the breaking behavior...

const asyncHooks = require('async_hooks')

var hook = asyncHooks.createHook({
  // init: () => {},
  // before: () => {},
  after: () => {},
  //destroy: () => {}
}).enable()

@addaleax addaleax added the confirmed-bug Issues with confirmed bugs. label Feb 14, 2020
@addaleax addaleax reopened this Feb 14, 2020
addaleax added a commit to addaleax/node that referenced this issue Feb 14, 2020
@addaleax
Copy link
Member

This was caused by 08e55e3; a fix is in #31801.

I’m assuming it’s too late for the fix to get into #31781 unless the release gets pushed out to next week, though.

MylesBorins pushed a commit that referenced this issue Feb 18, 2020
Refs: 4aca277
Refs: #30236
Fixes: #31796

PR-URL: #31801
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
codebytere pushed a commit that referenced this issue Feb 27, 2020
Refs: 4aca277
Refs: #30236
Fixes: #31796

PR-URL: #31801
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants