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

Route handler still runs after returning response for earlier uncaught exception #1974

Open
3 tasks done
hashtagchris opened this issue Jul 10, 2024 · 1 comment
Open
3 tasks done

Comments

@hashtagchris
Copy link

hashtagchris commented Jul 10, 2024

  • Used appropriate template for the issue type
  • Searched both open and closed issues for duplicates of this issue
  • Title adequately and concisely reflects the feature or the bug

Restify Version: 11.1.0 (also 10.0.0, 9.1.0, 8.6.1, 7.7.0, but not earlier versions)
Node.js Version: v20.15.1

Expected behaviour

I would expect the handler registered for a given route to not execute if an (error) response has already been sent to the client.

I understand middleware handlers possibly running regardless of a response being sent, but I would expect "normal", route-specific handlers to be skipped if there's already been an uncaught exception, and the response was completed.

Actual behaviour

The handler still runs. Unless the handler defensively checks res.headersSent, there's unnecessary processing and an ERR_HTTP_HEADERS_SENT error when the handler attempts to send a response.

Repro case

const restify = require('restify')
const axios = require('axios')

const server = restify.createServer({handleUncaughtExceptions: true})
server.use(restify.plugins.bodyParser())

server.on('uncaughtException', function (req, res, route, err) {
  console.log('restify uncaughtException:', err.code, err.message)
  if (!res.headersSent) {
    res.send(500, {error: err.message})

    // Q. How can I end request processing here, and prevent the registered hello handler from running?
  }
})

server.get('/hello', function (req, res, next) {
  console.log('hello handler called')

  // // I'd prefer not to add defensive code like to this to individual route handlers
  // if (res.headersSent) {
  //   console.log('weird, response already sent, skipping execution')
  //   return
  // }

  res.send(200, {hello: 'world'})
})

server.listen(9595, function () {
  console.log(`${server.name} listening at ${server.url}`)
})

// intentionally send a request with an invalid (empty) gzip body
axios({
  method: 'get',
  url: 'http://localhost:9595/hello',
  headers: {'Content-encoding': 'gzip', 'Content-type': 'application/json'},
  validateStatus: () => true
}).then(response => {
  console.log('server response', response.status, response.data)

  server.close()
})

Output

restify listening at http://[::]:9595
restify uncaughtException: Z_BUF_ERROR unexpected end of file
hello handler called
restify uncaughtException: ERR_HTTP_HEADERS_SENT Cannot set headers after they are sent to the client
server response 500 { error: 'unexpected end of file' }

Cause

Seems to be related to the uncaught exception occurring for an early middleware handler (readBody). This doesn't prevent later handlers from executing.

Are you willing and able to fix this?

No

@hashtagchris
Copy link
Author

Filed #1975 for the behavior when handleUncaughtExceptions is false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant