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

request with agent, timeout not work #21185

Closed
killagu opened this issue Jun 7, 2018 · 2 comments
Closed

request with agent, timeout not work #21185

killagu opened this issue Jun 7, 2018 · 2 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@killagu
Copy link
Contributor

killagu commented Jun 7, 2018

  • Version: 10.3
  • Platform: all
  • Subsystem: http

When request with agent, set the request timeout not work, timeout is still
agent timeout.

The code:

'use strict';

const http = require('http');
const port = 8000;
const AGENT_TIMEOUT = 3000;
const HTTP_TIMEOUT = 5000;

const server = http
  .createServer((req, res) => {
    console.log('never response');
  })
  .listen({ port }, () => doRequest());

function doRequest() {
  const agent = new http.Agent({ timeout: AGENT_TIMEOUT });
  const request = http.request({
    host: 'localhost',
    port,
    agent,
    timeout: HTTP_TIMEOUT, // The HTTP_TIMEOUT will not work.
  }, res => {});

  request.end();

  const start = Date.now();
  request.on('timeout', () => {
    console.log('timeout:' + (Date.now() - start)); // It print 3000, not 5000
    request.abort();
    server.close();
  });

  request.on('error', () => {});
}

I expect request timeout will overwrite the agent time out.

If it's a bug, I can make a PR to fix it.

@bnoordhuis bnoordhuis added the http Issues or PRs related to the http subsystem. label Jun 7, 2018
@bnoordhuis
Copy link
Member

That's a bug for more reasons than one because timeout isn't a documented http.Agent option and shouldn't have worked in the first place. It'd be good to document it and ensure overriding works.

Thanks for the bug report, pull request welcome!

@killagu
Copy link
Contributor Author

killagu commented Jun 7, 2018

I'll make a PR to fix it.

killagu added a commit to killagu/node that referenced this issue Jun 8, 2018
When request with both timeout and agent, timeout not
work. This patch will fix it, socket timeout will set
to request timeout before socket is connected, and
socket timeout will reset to agent timeout after
response end.

Update agent doc, add timeout option.

Fixes: nodejs#21185
targos pushed a commit that referenced this issue Jul 14, 2018
When request with both timeout and agent, timeout not
work. This patch will fix it, socket timeout will set
to request timeout before socket is connected, and
socket timeout will reset to agent timeout after
response end.

Fixes: #21185

PR-URL: #21204
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants