chimurai / http-proxy-middleware

:zap: The one-liner node.js http-proxy middleware for connect, express, next.js and more
MIT License
10.75k stars 840 forks source link

ERR_HTTP_HEADERS_SENT from onProxyReq with keep-alive #472

Open nickwesselman opened 4 years ago

nickwesselman commented 4 years ago

Under load, when using a keep-alive agent, if the # of incoming requests exceeds the max sockets, we are seeing a ERR_HTTP_HEADERS_SENT very quickly.

Is this a bug report?

Yes

Steps to reproduce

  1. Configure proxy middleware with a keep-alive agent
  2. Add an onProxyReq handler that sets a header
  3. Apply load with a larger number of threads/VUs than available sockets.

Expected behavior

No errors, perhaps timeouts due to socket availability.

Actual behavior

Hard crash with ERR_HTTP_HEADERS_SENT.

Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at ClientRequest.setHeader (_http_outgoing.js:485:11)
    at ProxyServer.onProxyReq (C:\dev\nwe-jss-test\jss13.0.1-sc9.3.0\keepalive-middleware-test\index.js:23:18)
    at ProxyServer.emit (C:\dev\nwe-jss-test\jss13.0.1-sc9.3.0\keepalive-middleware-test\node_modules\eventemitter3\index.js:184:35)
    at ClientRequest.<anonymous> (C:\dev\nwe-jss-test\jss13.0.1-sc9.3.0\keepalive-middleware-test\node_modules\http-proxy\lib\http-proxy\passes\web-incoming.js:133:16)
    at ClientRequest.emit (events.js:215:7)
    at tickOnSocket (_http_client.js:684:7)
    at onSocketNT (_http_client.js:723:5)
    at processTicksAndRejections (internal/process/task_queues.js:80:21)

Setup

client info

k6, see below

target server info

IIS

Reproducible Demo

Server

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

const apiHost = 'http://localhost';

const agent = new keepAlive({
    maxSockets: 120,
    maxFreeSockets: 120,
    timeout: 60000,
    freeSocketTimeout: 30000,
});

const server = express();
server.get('/connections', function(req, res, next) {
    res.status(200).send({ sockets: agent.getCurrentStatus() })
  });
  server.use('*', createProxyMiddleware({
    agent,
    target: apiHost,
    changeOrigin: true,
    onProxyReq: (proxyReq, req, res) => {
        proxyReq.setHeader('TEST', 'hello world');
    }
}));
server.listen(3000);

K6 Project

import http from 'k6/http';
import { Counter } from "k6/metrics";

export let options = {
    discardResponseBodies: false,
    scenarios: {
        headRequest: {
            executor: 'constant-vus',
            exec: 'headRequest',
            vus: 150,
            duration: '30s',
        }
    },
    thresholds: {
        "pageError": [{threshold: "count<1", abortOnFail: true}],
    }
};

const pageError = new Counter("pageError");

export function headRequest() {
    var response = http.request('GET', 'http://localhost:3000/');
    if (response.error || response.status !== 200) {
        pageError.add(1);
    }
}
nickwesselman commented 4 years ago

This appears to be an issue with node-http-proxy, and the fact that it uses .on('socket') to fire the proxyReq event. This took a lot of digging into the behavior of HTTP requests in node. When a socket is immediately available to a request, the event fires in the ClientRequest constructor, so the request headers can be modified. But with keepAlive enabled on the agent, a socket may not be immediately available. In that case the request continues on and begins buffering its output, so by the time onSocket is invoked, the request header has been 'sent'.

I was able to reproduce this without use of http-proxy:

const express = require('express');
const http = require('http');

const agent = new http.Agent({
    keepAlive: true,
    maxSockets: 5,
    maxFreeSockets: 5
});

const _originalOnSocket = http.ClientRequest.prototype.onSocket;
http.ClientRequest.prototype.onSocket = function onSocket(socket, err) {
    console.log(`Received socket, are headers sent? ${this.headersSent}`);
    const myObject = {};
    Error.captureStackTrace(myObject);
    console.log(myObject.stack);
    _originalOnSocket.apply(this, arguments);
}

const server = express();

server.get('/', function (req, res, next) {
    var httpOptions = {
        hostname: "localhost",
        path: "/",
        agent
    };

    var proxyReq = http.request(httpOptions);
    proxyReq.on('socket', (socket) => {
        console.log(`Setting header`);
        proxyReq.setHeader('TEST', 'hello world');   
    });

    req.pipe(proxyReq);

    proxyReq.on('response', (proxyRes) => {
        res.status = proxyRes.status;
        proxyRes.pipe(res);
    });

});

server.listen(3000);

Output on failure

Received socket, are headers sent? true
Error
    at ClientRequest.onSocket (C:\dev\nwe-jss-test\jss13.0.1-sc9.3.0\keepalive-middleware-test\just-node.js:14:11)
    at setRequestSocket (_http_agent.js:428:7)
    at Agent.<anonymous> (_http_agent.js:95:7)
    at Agent.emit (events.js:315:20)
    at Socket.onFree (_http_agent.js:315:11)
    at Socket.emit (events.js:315:20)
    at emitFreeNT (_http_client.js:667:10)
    at processTicksAndRejections (internal/process/task_queues.js:83:21)
Setting header
_http_outgoing.js:518
    throw new ERR_HTTP_HEADERS_SENT('set');

So this isn't a problem with http-proxy-middleware, but does anyone perhaps have a suggestion as to an alternate way to set headers dynamically based on values from the incoming request?

nickwesselman commented 4 years ago

Workaround we've found is to instead modify the request headers in separate middleware, before applying the proxy.

server.use((req, res, next) => {
  // change req here
  next();
});
bobalong79 commented 3 years ago

I've been having the same issue and I think it happens when any agent is used, not something specific to keepalive (I'm using global-agent's HttpsProxyAgent). My workaround is to use the custom filter mechanism to alter the headers prior to the proxy request being set up. It looks like this filter mechanism was designed to allow you to implement custom matching logic, but as it provides access to the req: IncomingRequest object before the proxy request is created it is a convenient way to manipulate the headers before they even reach the http-proxy code.

const filter = (_pathname: string, req: http.IncomingMessage): boolean => {
  req.headers['TEST'] = 'hello world';

  return true;
};

server.use('*', createProxyMiddleware(filter, {
  agent,
  target: apiHost,
  changeOrigin: true
}));

I think this is basically the same as what you've done but it keeps all the logic in the same middleware.

I guess one drawback of this approach (and your workaround) is that because the headers are changed on the original incoming request rather than on the proxied request, other downstream routers that rely on "untampered" headers might be compromised. In that case then you'd need to keep a copy of the original set and copy them back after onProxyRes completes. I don't need to do that for my application though.

JounQin commented 3 years ago

I also meet this issue when using fixRequestBody from this middleware.

3rj commented 3 years ago

I also meet this issue when using fixRequestBody from this middleware.

Did you find any workaround to this issue. (using fixRequestBody and agent together)

JounQin commented 3 years ago

I also meet this issue when using fixRequestBody from this middleware.

Did you find any workaround to this issue. (using fixRequestBody and agent together)

Nope, I have to move proxy middlewares to be used firstly.

markcellus commented 2 years ago

Also ran into this issue with fixRequestBody 😞

chestnutchen commented 1 year ago

Also ran into this issue with fixRequestBody

josslzr commented 1 year ago

For those who still facing this issue I've found a workaround. Basically you need to set the headers and buffer properties while creating the proxy middleware:

 getProxyConfig(headers: any, body: string): RequestHandler {
    return createProxyMiddleware({
      ...//other options
      headers,
      buffer: this.getBodyStream(body)
    })
  }

   private getBodyStream(body: string): Readable {
    const stream = new Readable();
    stream.push(body);
    stream.push(null);
    return stream;
  }

Hope this helps! :)