node-restify
node-restify copied to clipboard
Inflight request accounting leaks when error handlers don't flush response
- [x] Used appropriate template for the issue type
- [x] Searched both open and closed issues for duplicates of this issue
- [x] Title adequately and concisely reflects the feature or the bug
Bug Report
Restify Version
Proven reproducible on 8.2.0, 7.3.0, and on my fork in the cp_fix-inflight-request-counter-leak
branch.
Node.js Version
Proven reproducible on v8.15.0 and v10.15.1
Expected behaviour
This section details what you expected restify to do based on the code that you wrote
Restify inflight counter should not leak
Inflight requests: 1
Inflight requests: 1
Inflight requests: 1
Inflight requests: 1
...
Actual behaviour
Restify inflight counter leaks
Inflight requests: 1
Inflight requests: 2
Inflight requests: 3
Inflight requests: 4
...
This section details what restify actually did when you ran your code
Repro case
const restify = require('restify');
const { InternalServerError } = require('restify-errors');
const http = require('http');
const server = restify.createServer();
const delay = 100;
const port = 8080;
server.pre((req, res, next) => {
console.log(`Inflight requests: ${server.inflightRequests()}`);
next();
});
server.get('/', (req, res, next) => next(new InternalServerError('foo')));
server.on('InternalServer', () => {});
server.listen(
8080,
() => setInterval(() => http.get(`http://localhost:8080/`), delay)
);
Cause
This is caused because the response has not been flushed. That is, changing the InternalServer
handler to this will resolve the issue:
server.on('InternalServer', (req, res) => res.send('foo'));
But there are cases wher an error handler fails to flush a response. Since no processing can happen after an error handler completes, restify should be smart enough to automatically flush the response in these cases.
Note that this is very similar to, but not the same issue (or same underlying cause) as https://github.com/restify/node-restify/issues/1765.
Are you willing and able to fix this?
Yes.
IIUC, I think this is related to the breaking change in restify 3.x where error listeners now require callbacks: http://restify.com/docs/server-api/#errors
If I update your repro to call the cb, it no longer leaks. Can you confirm? I suspect this is similar to the cases where next isn't called.