restify / node-restify

The future of Node.js REST development
http://restify.com
MIT License
10.71k stars 983 forks source link

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

Open hashtagchris opened 2 months ago

hashtagchris commented 2 months ago

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 commented 2 months ago

Filed https://github.com/restify/node-restify/issues/1975 for the behavior when handleUncaughtExceptions is false.