chimurai / http-proxy-middleware

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

Restify support #802

Open jrsquared opened 2 years ago

jrsquared commented 2 years ago

Describe the feature you'd love to see

Restify support is ALMOST there, it just does not call next() after a successful proxy.

Additional context (optional)

No response

chimurai commented 2 years ago

Could you share which version of http-proxy-middleware you used/tried?

jrsquared commented 2 years ago

I am currently using 2.0.6. I've been using the proxy with express without issue, but recently switched to restify, and get lots of log messages about long-running requests / next() not being called.

Looking at http-proxy-middleware.ts, it appears that next() only gets called if it is not proxied, or there is an error? https://github.com/chimurai/http-proxy-middleware/blob/master/src/http-proxy-middleware.ts#L41-L51

    if (this.shouldProxy(this.proxyOptions.pathFilter, req)) {
      try {
        const activeProxyOptions = await this.prepareProxyRequest(req);
        debug(`proxy request to target: %O`, activeProxyOptions.target);
        this.proxy.web(req, res, activeProxyOptions);
      } catch (err) {
        next && next(err);
      }
    } else {
      next && next();
    }
chimurai commented 2 years ago

Do you have a minimal reproducible repo with the problem?

Tried this setup and I'm able to proxy without log messages you were talking about:

// main.mjs

import { createServer } from 'restify'; // ^8.6.1
import { createProxyMiddleware } from 'http-proxy-middleware'; // ^2.0.6

const server = createServer({
  name: 'my-app',
  version: '1.0.0',
});

server.get('/users', createProxyMiddleware({
  target: 'http://jsonplaceholder.typicode.com',
  changeOrigin: true,
}));

server.listen(3000, () => {
  console.log('server listening on port 3000');
});
curl http://localhost:3000/users
jrsquared commented 2 years ago

Hey, yes, sorry, I log the inflight requests myself. Here is an example building on yours to show the leak

// main.mjs

import { createServer } from 'restify'; // ^8.6.1
import { createProxyMiddleware } from 'http-proxy-middleware'; // ^2.0.6

const server = createServer({
  name: 'my-app',
  version: '1.0.0',
});

server.get('/users', createProxyMiddleware({
  target: 'http://jsonplaceholder.typicode.com',
  changeOrigin: true,
}));

server.get('/inflight', (req, res, next) => {
  res.send(200, server.inflightRequests());
  return next();
})

server.listen(3000, () => {
  console.log('server listening on port 3000');
});
curl http://localhost:3000/users
curl http://localhost:3000/users
curl http://localhost:3000/users
curl http://localhost:3000/inflight
curl http://localhost:3000/users
curl http://localhost:3000/users
curl http://localhost:3000/inflight

As you can see, inflight requests keep increasing since the successful proxied requests never call next()

chimurai commented 2 years ago

Not sure what's behind your server.inflightRequests() implementation

If I substitute:

server.get('/inflight', (req, res, next) => {
  res.send(200, server.inflightRequests());
  return next();
})

with a simple response:

server.get('/inflight', (req, res, next) => {
  res.send(200, 'inflightRequests');
  return next();
})

with the same curl sequence I'm not able to get the error you're getting.

curl http://localhost:3000/users
curl http://localhost:3000/users
curl http://localhost:3000/users
curl http://localhost:3000/inflight
curl http://localhost:3000/users
curl http://localhost:3000/users
curl http://localhost:3000/inflight

Suspecting you're dealing with some async code? If so, this might help:

server.get('/inflight', async (req, res, next) => {
  res.send(200, await server.inflightRequests());
  return next();
})
jrsquared commented 2 years ago

server.inflightRequests() is built into restify to show how many requests are inflight, i.e. not completed yet (next() was never called) http://restify.com/docs/server-api/#inflightrequests

There is no error, the number of inflight requests just keeps growing every time the proxy is used, because next() is not called on successful proxies, like I mentioned in the initial post.

chimurai commented 2 years ago

Looks like next() usage in restify is slightly different from express' implementation. Where it seems mandatory to call next() right after sending a response.

With the following I'm able to proxy requests in restify without long-running requests:

// main.mjs

import { createServer } from 'restify';
import { createProxyMiddleware } from 'http-proxy-middleware';

const server = createServer({
  name: 'my-app',
  version: '1.0.0',
});

const proxy = createProxyMiddleware({
  target: 'http://jsonplaceholder.typicode.com',
  changeOrigin: true,
  selfHandleResponse: true,                 // manually handle response
  onProxyRes(proxyRes, req, res) {
    proxyRes.pipe(res);                     // stream response to client without modifying it
    req.next();                             // call next() when response is received
  }
})

server.get('/users', (req, res, next) => { 
  req.next = next;                          // make next() available so it can be called in onProxyRes
  proxy(req, res, next);
});

server.get('/inflight', async (req, res, next) => {
  res.send(200, server.inflightRequests());
  return next();
})

server.listen(3000, () => {
  console.log('server listening on port 3000');
});

next() is somewhat server implementation specific. (not standardised I believe) Think restify is a "new" category of server which requires you to call next().

  1. no next at all (vanilla http server, next.js server)
  2. optional next, only next on error (express, express-like; e.g. connect, fastify, polka)
  3. mandatory next (restify)

Will have to think if there is an elegant way to support restify out-of-the-box. Maybe you have some ideas?

jrsquared commented 2 years ago

Yeah, the fact that next() is different across different server implementations makes this difficult. I do not have any ideas, unfortunately. I even forked the library and tried to implement it myself, but could not figure it out, that's why I had to ask you 😄

I tried this pattern in my own codebase and am getting weird binary responses now; unsure if that's because it the proxied responses are gzipped or something?

chimurai commented 2 years ago

The weird responses might be related to this gzip issue in http-proxy: https://github.com/http-party/node-http-proxy/issues/795

1) You could try to disable gzip if that's possible.

2) Decompress the response

curl --compressed http://localhost:3000/gzip

3) Alternatively you could use responseInterceptor. Internally this will decompress the response so you could manipulate the response. In your case you can just return it.

import { createProxyMiddleware, responseInterceptor } from 'http-proxy-middleware';

...

const proxy = createProxyMiddleware({
  target: 'http://httpbin.org',
  changeOrigin: true,
  selfHandleResponse: true,
  onProxyRes: responseInterceptor(async (responseBuffer, proxyRes, req, res) => {
    setTimeout(() => req.next())     // call next() after returning `responseBuffer`
    return responseBuffer; 
  });
});

server.get('/gzip', (req, res, next) => { 
  req.next = next;
  proxy(req, res, next);
});
curl http://localhost:3000/gzip