Skip to content

difference in timeout behavior between versions 0.18.1 and 0.19.0-2 #2710

Closed
@fresidue

Description

@fresidue

Describe the bug

Using axios version 1.18.0, running the following mocha test, where the timeout is specified, results with:

Got the expected error:  timeout of 200ms exceeded
  ✓ test axios (210ms)

  1 passing (217ms)

Using axios version 1.19.x, running the same mocha test would result with

  1) test axios

  0 passing (1s)
  1 failing

  1) test axios:
     Error: drp. this happens instead
      at Timeout._onTimeout (src/subscriptions-manager/axios-test.js:24:14)
      at listOnTimeout (internal/timers.js:531:17)
      at processTimers (internal/timers.js:475:7)

To Reproduce
run with mocha

const axios = require('axios');

const invalidUrl = 'http://12.12.12.12:123/arf/arf';

const postToInvalidUrl = async () => {

  return new Promise((resolve, reject) => {
    axios({
      url: invalidUrl,
      method: 'post',
      data: {not: 'relevant'},
      timeout: 200,
    })
      .then(() => {
        reject(new Error('success should"t happen'));
      })
      .catch(err => {
        console.log('Got the expected error: ', err.message);
        resolve();
      });

    setTimeout(function () {
      reject(new Error('drp. this happens instead'));
    }, 1000);
  });
};

it('test axios', async () => {
  await postToInvalidUrl();
});

Expected behavior
that the behavior from 0.18.0 would be preserved

Activity

chinesedfan

chinesedfan commented on Feb 4, 2020

@chinesedfan
Collaborator

Don't remember we changed something related to timeout. I saw it once in codesandbox and it can't be reproducible any more, no matter in local test or https://codesandbox.io/s/axios2710-bgphy. Can you confirm it is always the same result with different timeout values?

fresidue

fresidue commented on Feb 4, 2020

@fresidue
Author

Yes it is consistent for different values of timeout. However, the break in behavior seems to occur between 0.19.0 and 0.19.1 (i.e. the timeout value is respected for 0.19.0)

chinesedfan

chinesedfan commented on Feb 4, 2020

@chinesedfan
Collaborator

@fresidue Can you provide the required information in the issue template? My codesandbox confirms your conclusion, which uses Node.js 10.17.0. But the local environment(OSX 10.14.5, Node.js 10.17.0) always works well.

tblasche

tblasche commented on Mar 16, 2020

@tblasche

The timeout functionality breaks with https://github.com/axios/axios/pull/1752/files in axios v0.19.1

This is a pretty problematic bug. If URL is an IP address which is not reachable, the timeout will not be respected.

fishcharlie

fishcharlie commented on Mar 23, 2020

@fishcharlie

@chinesedfan I'm running into this as well. I tried to provide the requested information below. Let me know if there is anything I can do to help get this fixed.

  • Axios Version: 0.19.2 works as expected in 0.18.1
  • OS: macOS 10.15.4 (19E264b)
  • Browser: Node.js
  • Browser Version: [e.g. 22] v10.16.0
  • Additional Library Versions: NA
chinesedfan

chinesedfan commented on Mar 28, 2020

@chinesedfan
Collaborator

The timeout functionality breaks with #1752 (files) in axios v0.19.1

@fishcharlie According to @tblasche's discovery, it seems to be due to req.setTimeout. So this is an upstream problem. Can you or someone to check issues of Node.js?

fishcharlie

fishcharlie commented on Mar 28, 2020

@fishcharlie

@chinesedfan I'm not finding anything on the Node.js issue section regarding this. I found a few similar things, but they didn't look like the same thing.

I'd be happy to submit an issue and help try to move this forward. My only concern is that I don't have as much knowledge as some of the others here in terms of the story behind this issue an associated PR.

I'm also not sure that we have a concrete failing example that is simple and easy to reproduce. Which makes it harder since anyone looking at the issue will have to dig through the axios codebase.

Any thoughts on how to proceed? Would love to help get this fixed ASAP, but just want to make sure we are all on the same page, and I know I'm slightly more limited in my knowledge level about this.

chinesedfan

chinesedfan commented on Mar 29, 2020

@chinesedfan
Collaborator

I'm also not sure that we have a concrete failing example that is simple and easy to reproduce.

@fishcharlie That's really what we need. Once confirmed, we can open an issue to Node.js.

The sample can be extracted from lib/adapters/http.js, which is a simple request based on Node.js http module. You can write two cases. One is using setTimeout like axios v0.18, and the other is like #1752.

fishcharlie

fishcharlie commented on Mar 29, 2020

@fishcharlie

@chinesedfan I was only able to reproduce this error when using the NPM package nock, under very specific conditions. Took me a while to get a failing example. Below you can find the most minimal code I was able to use to recreate the problem.

I attempted to create this also with an Express server that just doesn't send any response, and was unable to reproduce that. So I'm wondering if there is something else going on behind the scenes with like the connection or something.

If you think this all looks good I will open an issue on the nodejs/node repository. But I'm still unsure if there is something else about how nock handles things that I need to investigate further first. But that might be past my expertise level at this point.

Let me know what your thoughts are.


Working Code

const http = require("http");

const nock = require("nock");
nock("http://test.com")
	.persist()
	.get("/test")
	.reply(200, (a, b, c) => {});

const req = http.request("http://test.com/test", () => {
	console.log("Response received");
});
setTimeout(() => {
	req.abort();
	console.error("Response timed out");
}, 5000);

Failing Code

const http = require("http");

const nock = require("nock");
nock("http://test.com")
	.persist()
	.get("/test")
	.reply(200, (a, b, c) => {});

const req = http.request("http://test.com/test", () => {
	console.log("Response received");
});
req.setTimeout(5000, () => {
	req.abort();
	console.error("Response timed out");
});
mooyoul

mooyoul commented on Mar 29, 2020

@mooyoul

This issue is called as "Call Timeout".

From request's README:

timeout - integer containing number of milliseconds, controls two timeouts.

Read timeout: Time to wait for a server to send response headers (and start the response body) before aborting the request.
Connection timeout: Sets the socket to timeout after timeout milliseconds of inactivity. Note that increasing the timeout beyond the OS-wide TCP connection timeout will not have any effect (the default in Linux can be anywhere from 20-120 seconds)

HTTPRequest#setTimeout calls Socket#setTimeout internally, which only tracks "socket inactivity". socket timer will be reset if any socket activity were detected.

according to documentation:

Sets the socket to timeout after timeout milliseconds of inactivity on the socket. By default net.Socket do not have a timeout.

When an idle timeout is triggered the socket will receive a 'timeout' event but the connection will not be severed. The user must manually call socket.end() or socket.destroy() to end the connection.

so, if socket timeout is 1000ms and server sends packets per each 800ms, socket won't trigger timeout event.

Here's quick reproducible codesandbox project:
https://codesandbox.io/s/keen-surf-07ibg

Please take a look into request's source - Read Timeouts has been handled by creating internal timers.

mooyoul

mooyoul commented on Mar 29, 2020

@mooyoul

Quick & Dirty workaround at this moment:

// Quick-and-dirty solution for call-timeout issue
// @see https://github.com/axios/axios/issues/2710
async function patchedAxios<T>(options: AxiosRequestConfig): Promise<AxiosResponse<T>> {
  if (options.timeout) {
    const cancelSource = axios.CancelToken.source();
    const readTimeoutTimer = setTimeout(() => {
      cancelSource.cancel(`timeout of ${options.timeout}ms exceeded`);
    }, options.timeout);

    try {
      return await axios({
        ...options,
        cancelToken: cancelSource.token,
      });
    } catch (e) {
      if (axios.isCancel(e)) {
        throw new Error(e.message);
      }

      throw e;
    } finally {
      clearTimeout(readTimeoutTimer);
    }
  } else {
    return axios(options);
  }
}
fishcharlie

fishcharlie commented on Mar 29, 2020

@fishcharlie

@chinesedfan Wasn’t the reason for this change (and not using setTimeout/internal timers) due to a security vulnerability? Wouldn’t the code @mooyoul just suggested have that same security vulnerability?

34 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @fresidue@fishcharlie@tblasche@chinesedfan@mooyoul

        Issue actions

          difference in timeout behavior between versions 0.18.1 and 0.19.0-2 · Issue #2710 · axios/axios