ladjs / superagent

Ajax for Node.js and browsers (JS HTTP client). Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
https://ladjs.github.io/superagent/
MIT License
16.58k stars 1.33k forks source link

TCP reuse #1584

Open yigaldviri opened 4 years ago

yigaldviri commented 4 years ago

Hi, I'm using superagent and while inspecting the network I noticed that superagent is creating a new TCP connection for each request. I'm using superagent for a sequence of requests which results in a large number of TCP connections (can reach several hundreds).

I tried to follow this idea and use agentkeepalive package however this approach has some downsides:

  1. While superagent works out-of-the-box with http and https, agentkeepalive needs to be defined per protocol.
  2. As a result of the previous section, in case I do an http request that redirects to https request I get an error of Protocol "https:" not supported. Expected "http:" since the protocol has changed.
  3. As a result of section 2 the app crushes since with an uncaughtException (which happens only upon redirects and not when using the wrong protocol i.e. the http agentkeepalive for https request)

Needless to say that using request.set('Connection', 'keep-alive'); didn't solve it.

My questions are:

  1. Is there un updated solution for the keep-alive problem?
  2. Is there a built-in option in superagent to define the reuse of TCP connections? If not, is there a plan to support one?

Thanks

niftylettuce commented 4 years ago

PR welcome!

yigaldviri commented 4 years ago

Tried to find a solution both in superagent and in agentkeepalive repos but it required such changes in both repos that I don't think someone (including me) would approve.

So, after looking deep into the code with @omrilitov , our workaround was to intercept the request every time and since superagent reuse the same request object when redirecting we reset the agent as well. It looks like this:

 const _oldRequest = request.request;
  request.request = function (...args) {
    // first intercept the request and do our stuff
    const { url } = this;

    if (url.startsWith('https:')) {
      this.agent(sslAgent);
    } else {
      this.agent(agent);
    }

    // then continue with superagent logic
    _oldRequest.apply(this, ...args);
  };

Hope this is helping any other developers who run into this issue.

@omrilitov and I thought about creating some kind of an interceptors for superagent - is that something you guys will consider?

niftylettuce commented 4 years ago

Couldn't you just use xhook? See https://github.com/jpillora/xhook and some examples are in @cabinjs at https://cabinjs.com/?id=automatic-request-logging.

omrilitov commented 4 years ago

xhooks is designed for client-side interceptors, we needed the changes for server-side

niftylettuce commented 4 years ago

Ah, yeah, I even filed this issue regarding that here https://github.com/jpillora/xhook/issues/103. I'd be open to interceptors, I already have them in Frisbee at https://github.com/niftylettuce/frisbee (if you want a Fetch implementation or inspiration).