http-proxy-middleware icon indicating copy to clipboard operation
http-proxy-middleware copied to clipboard

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

Open lbeschastny opened this issue 3 years ago • 11 comments

Checks

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
[email protected] /home/leonid/Documents/culture/gateway
└── [email protected]

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)

lbeschastny avatar May 24 '22 13:05 lbeschastny

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.

audifaxdev avatar Sep 07 '22 19:09 audifaxdev

@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 avatar Sep 14 '22 14:09 lbeschastny

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

lbeschastny avatar Sep 14 '22 19:09 lbeschastny

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.

lbeschastny avatar Sep 14 '22 19:09 lbeschastny

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.

alxlchnr avatar Sep 19 '22 09:09 alxlchnr

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.

Denoder avatar Oct 02 '22 19:10 Denoder

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!

nestdimon avatar Oct 16 '22 10:10 nestdimon

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

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

lbeschastny avatar Nov 10 '22 21:11 lbeschastny

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

Muhammad-Ahmad-Rai avatar Jun 10 '23 18:06 Muhammad-Ahmad-Rai

@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.

lbeschastny avatar Jun 11 '23 15:06 lbeschastny