inflightRequests() doesn't decrease if an error is thrown in server.pre()

[seoker]

Bug Report

Restify Version

5.2.0

Node.js Version

7.10.1

Expected behaviour

The inflightRequests() should be decreased back to original when something in server.pre() break from executing middlewares.

Actual behaviour

The inflightRequests() keep increasing each time when something break in server.pre() handlers.

Repro case

server.pre((req, res, next) => {
  console.log('inflightRequests:', server.inflightRequests()); // keep increasing
  return next(new errors.ForbiddenError('Invalid API access.'));
  
  // also fall into this case
  // res.send(403);
  // next();
});

Cause

Are you willing and able to fix this?

I am willing to fix this if I have free time to dig into the whole process through a incoming request to the end. For now, I can just point out where the issue is and have no idea where to put _finishReqResCycle().

[retrohacker]

Hey @seoker,

We have been running into this recently as well. In fact, we have stopped using inflightRequestThrottle entirely as the inflightRequests()'s reliability is heavily reliant on the proper usage of next. If you call next before yielding the event loop on every route, inlfightRequests() will always be 1. If you fail to call next for any reason, inflightRequests() will trend upwards towards infinity.

It looks like you've found another case where aborting a middleware stack during the pre handlers will cause inflightRequests() to misbehave.

As of right now, I believe our recommendation is to avoid relying on inflightRequests for anything, as there are too many cases that cause it to be incorrect.

If you need to monitor the number of active requests to the server, you can track them by incrementing on each request and decrementing on req.on('end' (and others). Note though that this metric doesn't include any middleware handlers that run after a response has been sent.

[seoker]

Hi @retrohacker,

Thanks for your kindly explanation. So the inflightRequestThrottle plugin is deprecated as well as inflightRequests(), and is scheduled to be removed, am I right?

[retrohacker]

It isn't deprecated per-say, it is just very broken. We intend to improve it, but it will require a combination of linting (ensuring next is always called not only in this repo, but in our user's projects) and fixing bugs like this one.

At this time, the API isn't reliable so we wouldn't advise using it. Perhaps we should update docs with a warning?

[retrohacker]

Hey @seoker,

I believe this was fixed in #1500

var restify = require('./lib')
var app = restify.createServer()
app.get('*', (req, res, next) => { console.log(app.inflightRequests()); next(new Error()) })
app.listen(8080)

$ node index.js
1
1
1
1
1
1