Memory leak in http-proxy dependency when client closes connection prematurely

lbeschastny commented 2 years ago

Checks

[X] I understand project setup issues should be asked on StackOverflow or in GitHub Discussions.
[X] I updated to latest http-proxy-middleware.

Describe the bug (be clear and concise)

This is an issue of http-proxy, but I decided to report it here as well because:

http-proxy-middleware is affected by this issue
last http-proxy version was released in 2020, so I don't expect them to fix it soon
this issue could be fixed on http-proxy-middleware side

Here is a complete description of this issue: https://github.com/http-party/node-http-proxy/issues/1586

Step-by-step reproduction instructions

Here is a minimal example to reproduce the above-mentioned memory leak:

const express = require('express');
const { createProxyMiddleware } = require('http-proxy-middleware');

const server = express()
    .use(createProxyMiddleware({ target: 'http://127.0.0.1:5000' }))
    .listen(7000);

server.once('listening', () => {
    console.log('server is listenning');
});

I used http-server to create an upstream server:

npx http-server . -p 5000

Memory leak appears when client sends a http request, but then closes connection prematurely after receiving HTTP headers without waiting for HTTP body.

I used cURL to simulate this behavior:

curl http://localhost:7000/15142e1388c685f57c58e0babbede1f1.jpg

Memory leak could be reproduces with cURL when requesting any binary file from a TTY terminal, since cURL doesn't support printing binary data to terminals and just closes the connection.

Expected behavior (be clear and concise)

Memory should not leak

How is http-proxy-middleware used in your project?

↪ npm ls http-proxy-middleware
culture-ru-gateway@1.0.6-alpha.0 /home/leonid/Documents/culture/gateway
└── http-proxy-middleware@2.0.6

What http-proxy-middleware configuration are you using?

{
    retry: {
        factor: 2,
        randomize: true,
        retries: 30,
        minTimeout: 10,
        maxTimeout: 500
    },
    xfwd: true,
    logLevel: 'warn'
}

What OS/version and node/version are you seeing the problem?

System:
    OS: Linux 5.13 Ubuntu 20.04.4 LTS (Focal Fossa)
    CPU: (8) x64 Intel(R) Core(TM) i7-10510U CPU @ 1.80GHz
    Memory: 5.11 GB / 15.34 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 16.15.0 - ~/.nvm/versions/node/v16.15.0/bin/node
    npm: 8.5.5 - ~/.nvm/versions/node/v16.15.0/bin/npm
  Managers:
    Apt: 2.0.8 - /usr/bin/apt
    Cargo: 1.49.0 - ~/.cargo/bin/cargo
    pip3: 20.0.2 - /usr/bin/pip3
    RubyGems: 3.1.2 - /usr/bin/gem
  Utilities:
    CMake: 3.16.3 - /usr/bin/cmake
    Make: 4.2.1 - /usr/bin/make
    GCC: 9.4.0 - /usr/bin/gcc
    Git: 2.25.1 - /usr/bin/git
    Ninja: 1.10.0 - /usr/bin/ninja
    FFmpeg: 4.2.4 - /usr/bin/ffmpeg
  Servers:
    Nginx: 1.18.0 - /usr/sbin/nginx
  Virtualization:
    Docker: 20.10.12 - /usr/bin/docker
  IDEs:
    Nano: 4.8 - /usr/bin/nano
    Sublime Text: 4126 - /usr/bin/subl
    VSCode: 1.67.1 - /usr/bin/code
  Languages:
    Bash: 5.0.17 - /usr/bin/bash
    Perl: 5.30.0 - /usr/bin/perl
    Python3: 3.8.10 - /usr/bin/python3
    Ruby: 2.7.0 - /usr/bin/ruby
    Rust: 1.49.0 - /home/leonid/.cargo/bin/rustc
  Databases:
    MongoDB: 4.4.14 - /usr/bin/mongo
  Browsers:
    Chrome: 101.0.4951.64

Additional context (optional)

Here is a code I used to fix http-proxy memory leak in my project:

proxy.on('proxyRes', (proxyRes, req, res) => {
    const cleanup = (err) => {
        // cleanup event listeners to allow clean garbage collection
        proxyRes.removeListener('error', cleanup);
        proxyRes.removeListener('close', cleanup);
        res.removeListener('error', cleanup);
        res.removeListener('close', cleanup);

        // destroy all source streams to propagate the caught event backward
        req.destroy(err);
        proxyRes.destroy(err);
    };

    proxyRes.once('error', cleanup);
    proxyRes.once('close', cleanup);
    res.once('error', cleanup);
    res.once('close', cleanup);
});

The actual code could be simpler since:

it should be enough to listen only for 'close' events since they are always emitted after 'error' events (unless emitClose is false)
it's probably not necessary to call removeListener manually, because those listeners should not prevent streams from being garbage collected (but who knows, I certainly don't want to test it)

audifaxdev commented 2 years ago

Unable to reproduce locally and in staging/production. using node 14 LTS AND HPM 1.0.3

We have a software in production that has been running OK without any leak for weeks now, with millions request / day.

lbeschastny commented 2 years ago

@audifaxdev I prepared a reproduction example here: https://github.com/lbeschastny/http-proxy-memory-leak-example

You may run it locally or using docker-compose. Or you could check my logs in Readme here.

Also, keep in mind that memory leak doesn't happen when all http requests are properly closed by clients. That's why it may be difficult to reproduce this issue. But I initially came upon it in our production environment, so it definitely can affect production servers.

lbeschastny commented 2 years ago

I just added a hotfix branch to my reproduction example repository to demonstrate that it fixes the memory leak in question

lbeschastny commented 2 years ago

Ideally, this issue should be addressed in http-proxy repository, not here. But I don't see any activity there, so it may not happen anytime soon.

alxlchnr commented 1 year ago

I observed a similar problem and this issue helped me to get a clue of the reason behind leaking sockets. I would love a fix for this issue.

Denoder commented 1 year ago

Would you guys perhaps want to try to use my package? https://github.com/refactorjs/http-proxy https://www.npmjs.com/package/@refactorjs/http-proxy

it's basically http-proxy but incorporating most of the pull requests that looked useful.

nestdimon commented 1 year ago

Oh man, for a couple of days I struggled with a hanging SSE connection on browser tab refresh. Thanks for hotfix, it definitely should be merged!

lbeschastny commented 1 year ago

Here is another fix for this memory leak: http-party/node-http-proxy#1559.

It was published to npm as http-proxy-node16.

Muhammad-Ahmad-Rai commented 1 year ago

@lbeschastny is this only an issue with node 15.15+, or is there memory leak in the older versions of node(node14) as well?

lbeschastny commented 1 year ago

@Muhammad-Ahmad-Rai I just run my reproduction example lbeschastny/http-proxy-memory-leak-example using Node.js v14.15 and it worked fine (no memory leaks).

According to https://github.com/http-party/node-http-proxy/pull/1559 this issue affects Node.js v15.5.0 and above. I just run my reproduction example using Node.js v15.5 and it crushed with a memory leak.

I'm not sure about Node.js v15.x prior to v15.5.0, though.

weeeziyuanli commented 1 month ago

Ideally, this issue should be addressed in http-proxy repository, not here. But I don't see any activity there, so it may not happen anytime soon.

    // cleanup code to prevent memory leaks
    const cleanup = (err) => {
        // cleanup event listeners to allow clean garbage collection
        proxyRes.removeListener('error', cleanup);
        proxyRes.removeListener('close', cleanup);
        res.removeListener('error', cleanup);
        res.removeListener('close', cleanup);

        // destroy all source streams to propagate the caught event backward
        req.destroy(err);
        proxyRes.destroy(err);
    };

    proxyRes.once('error', cleanup);
    proxyRes.once('close', cleanup);
    res.once('error', cleanup);
    res.once('close', cleanup);
};

req.destroy(err); should be res.destroy(err);? @lbeschastny

chimurai / http-proxy-middleware