Memory leak

[KidkArolis]

We've experience a serious memory leak issue when using this package. This was only introduced in some version >=0.8, with ^0.7 it used to be fine.

We haven't found the root of cause and I don't have a reproducible example, if it helps, we've used the following options:

reqBodyEncoding: null,
limit: '10mb',
memoizeHost: false,
proxyReqOptDecorator
proxyReqPathResolver
userResDecorator
proxyErrorHandler

We've fixed it by replacing the package with the following code (in case it helps anyone else):

const url = require('url')
const Boom = require('boom')
const http = require('http')
const https = require('https')

const ENV = process.env.NODE_ENV || 'development'

module.exports = function proxy (host) {
  return (req, res, next) => {
    const targetUrl = url.resolve(req.host, req.originalUrl)
    const parsed = url.parse(targetUrl)

    const reqOpts = {
      host: parsed.hostname,
      port: parsed.port,
      path: parsed.path,
      method: req.method,
      headers: { ...req.headers }
    }
    delete reqOpts.headers.host

    const agent = parsed.protocol === 'https:' ? https : http

    const proxyReq = agent.request(reqOpts, async (proxyRes) => {
      const isBadStatusCode = proxyRes.statusCode >= 500
      if (ENV === 'production' && isBadStatusCode) {
        // don't leak information in prod
        return next(new Boom(new Error('Bad gateway'), { statusCode: proxyRes.statusCode }))
      }

      res.status(proxyRes.statusCode)
      Object.keys(proxyRes.headers)
        // https://github.com/CoreFiling/express-http-proxy/pull/1
        .filter(header => header !== 'transfer-encoding')
        .forEach(header => res.set(header, proxyRes.headers[header]))

      proxyRes.pipe(res)
      proxyRes.on('error', next)
    })
    proxyReq.on('error', next)

    req.pipe(proxyReq)
    req.on('aborted', () => proxyReq.abort())
  }
}

[monkpow]

@KidkArolis Thanks for this. I'll dig in.

[ptrwllrt]

We also observe bigger memory issues since using this module. Right now we can't rule out our code to cause the trouble.

@KidkArolis What kind of app are you running there? How may requests are going through the proxy? Are You just proxying or also changeing the response?

[KidkArolis]

@cainvommars It was an api gateway, can't say for sure but probably around 100,000 requests in 2 hours would fill up the memory and the process would crash. We were only using userResDecorator for intercepting errors and changing the response body, not for succesful response modification, but that meant buffering and synchronously unzipping and rezipping each proxied response. As you can see in the snippet above we stopped doing that in the new version of the proxying code.

[ptrwllrt]

@KidkArolis Thanks for the insight. Our use case is different. We're using the proxy to integrate a page build with third party tools into our main app. For this we have to replace some elements of the third party page. But we're talking a couple of thousand requests a day, however, we might have the same issue here.

[alexandervain]

For us using this lib in a combination with a custom Node's domain-based middleware triggered an insane memory leak (seems to be domain's fault though - could be related to https://github.com/nodejs/node/issues/23862). We used:

• Node: 10.15.3
• express-http-proxy: 1.5.1
• express: 4.16.4

This code reproduces the issue:

const proxy = require('express-http-proxy');
const express = require('express');
const domain = require('domain');

const domainBasedMiddleware = () => {
  return function handleErrorsMiddleware(req, res, next) {
    const current = domain.create();
    current.add(req);
    current.add(res);
    current.run(next);
    current.once('error', err => next(err));

    // Uncomment the following code to apply workaround:
    // res.once('finish', () => {
    //   current.remove(req);
    //   current.remove(res);
    //   current.removeAllListeners()
    // });
  };
};

const PROXY_APP_PORT = 8808;
const TARGET_APP_PORT = 8809;

function startProxyApp() {
  const app = express()

  app.use(domainBasedMiddleware())

  app.use('/proxy', proxy(`http://localhost:${TARGET_APP_PORT}`));
  app.use('/direct', (req, res) => res.send('OK'));

  app.listen(PROXY_APP_PORT, () => console.log(`Example app listening on port ${PROXY_APP_PORT}!`));
}

function startTargetApp() {
  const targetApp = express();
  targetApp.get('/', (req, res) => res.send('OK from Target'))
  targetApp.listen(TARGET_APP_PORT, () => console.log(`Target app listening on port ${TARGET_APP_PORT}!`));
}

startProxyApp();
startTargetApp();

// To reproduce memleak run: "ab -n 50000 -c 2 http://localhost:8808/proxy"
// No/less memleak without express-http-proxy: "ab -n 50000 -c 2 http://localhost:8808/direct"

Workaround (simplified) that worked for us - the commented out code:

res.once('finish', () => {
    current.remove(req);
    current.remove(res);
    current.removeAllListeners()
});

Memory usage before applying the fix (with ~4K RPM):

Memory usage after applying the fix:

[Then eventually we stopped using that domain-based middleware at all] Hope it will help someone with the same issue.

[andrelegault]

A memory leak was fixed when removing the usage of this package according to https://kentcdodds.com/blog/fixing-a-memory-leak-in-a-production-node-js-app