Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support TCP_NODELAY option #1240

Merged
merged 3 commits into from
Jun 26, 2017
Merged

support TCP_NODELAY option #1240

merged 3 commits into from
Jun 26, 2017

Conversation

xiamengyu
Copy link
Contributor

@xiamengyu xiamengyu commented Jun 23, 2017

Add setNoDelay method for Request, this method can enable TCP_NODELAY option.

@kornelski
Copy link
Contributor

I don't see a reason to add buffering delay for requests. Why do you need this?

@xiamengyu
Copy link
Contributor Author

xiamengyu commented Jun 24, 2017

This is not add a delay for a request, this is support TCP_NODELAY option to disable TCP Nagle algorithm.
We found some TCP packages are delayed because of TCP Nagle algorithm.
This commit can support under layer TCP_NODELAY option to disable TCP Nagle algorithm.

@kornelski
Copy link
Contributor

  1. In Node Nagle's algorithm is off by default, so without this change it's always off (good). With this change it's on or off.

  2. I don't see default set for this._noDelay anywhere, so it's undefined, and !!this._noDelay makes false, so your change makes it slow by default.

@kornelski
Copy link
Contributor

kornelski commented Jun 25, 2017

https://nodejs.org/api/net.html#net_socket_setnodelay_nodelay
noDelay defaults to true.

So if true is the default used by node, and therefore used by superagent, the only use for this function is to set it to false. Why would you need to set nodelay to false?

@xiamengyu
Copy link
Contributor Author

xiamengyu commented Jun 26, 2017

noDelay defaults to true

This means the param noDelay is true when you call setNoDelay without pass a param.
The same web page says 'By default TCP connections use the Nagle algorithm'. So I need a option to disable Nagle algorithm(enable TCP_NODELAY option).

@xiamengyu
Copy link
Contributor Author

xiamengyu commented Jun 26, 2017

Here is the source code of nodejs socket.setNoDelay(): lib/net.js

Socket.prototype.setNoDelay = function(enable) {
  if (!this._handle) {
    this.once('connect',
              enable ? this.setNoDelay : () => this.setNoDelay(enable));
    return this;
  }

  // backwards compatibility: assume true when enable is omitted
  if (this._handle.setNoDelay)
    this._handle.setNoDelay(enable === undefined ? true : !!enable);

  return this;
};

@xiamengyu xiamengyu changed the title support tcp NO_DELAY option support TCP_NODELAY option Jun 26, 2017
@kornelski
Copy link
Contributor

Ah! I see. I'm sorry I misread that documentation page.

I've tried this change and I tried to see how much it affects the speed, but I'm not seeing any effect:

describe('benchmark', function(){
    it('nodelay', function(){
      this.timeout(1000000);
      const https = require('https');
      return Array.from(Array(100)).reduce(prev => prev.then(() => {
        return request
          .head('https://geekhood.net/robots.txt') // remote serv in case localhost is special
          .agent(new https.Agent({ keepAlive: false }))
          .set('connection', 'close')
      }), Promise.resolve());
    });
  });

With and without nodelay I'm seeing about the same speed (tested on macOS).

@kornelski kornelski merged commit 72babfb into ladjs:master Jun 26, 2017
@xiamengyu
Copy link
Contributor Author

xiamengyu commented Jun 26, 2017

Thank you for you merged this commit and added benchmark for it.
The delay of Nagle algorithm happens on some special situations, that not always happen.
Here is the Algorithm:

if there is new data to send
  if the window size >= MSS and available data is >= MSS
    send complete MSS segment now
  else
    if there is unconfirmed data still in the pipe
      enqueue data in the buffer until an acknowledge is received
    else
      send data immediately
    end if
  end if
end if

By the way, may be it's a good choice for user (who use superagent) to choose enable TCP_NODELAY or not.
I see TCP_NODELAY is enabled by default in superagent now, and user can not disable it.
Nagle's algorithm sometimes can save network resource, may be some situations need Nagle's algorithm, but user can not disable TCP_NODELAY in superagent.

@kornelski
Copy link
Contributor

kornelski commented Jun 26, 2017

Yes, since I didn't see any negative side effect, I've enabled it all the time just in case.

As far as I understand cost/benefit of Nagle's algorithm depends on how the data is sent — whether it's sent all at once, or sent bit by bit.

In case of superagent user has absolutely no control over this. All of the sending happens in Node's underlying HTTP request library. Since nodelay doesn't seem to affect anything, I presume Node already sends all headers at once.

So I don't see why user would need to have control over this, since it doesn't depend on the user, but depends on Node's implementation. In case of superagent there should clearly exist a right and wrong setting, and there shouldn't be a need to let user set it to a wrong value.

kornelski added a commit that referenced this pull request Aug 8, 2017
* commit '4e21f1c509c3ee09ea4031c27855ed8e9ddc0d35':
  Documented FormData support in .send() (#1260)
  Update supported node version to >= 4.0 (#1248)
  Keep nodelay always on
  support TCP_NODELAY option (#1240)
  timeout options.read property is not used.
  grammar misstype (#1234)
  Fix spelling mistake in the docs (#1232)
  Revert test 'fixes' - see PR #1227
  Support passphrase with pfx certificate
  Fix build errors
  Send payload in query string for GET and HEAD shorthand API
  Fix xml tests that broke with mime update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants