restify / restify/node-restify

Inflight request accounting leaks when error handlers don't flush response

Open
#1,767 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
10.7k
Forks
975
Avg merge
1d 6h
Merged PRs (30d)
5

Description

  • 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

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the server.inflightRequests() entry point and reproduce the leak using the provided InternalServer error-handler example. Trace the request completion path when the handler does not flush the response, then add regression coverage for that case. Done means repeated requests no longer increase the inflight counter when the error handler leaves the response unflushed.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
api, backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.