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

timeout doesn't fire in Node for partial response #4364

Closed
WestonThayer opened this issue Dec 31, 2021 · 3 comments
Closed

timeout doesn't fire in Node for partial response #4364

WestonThayer opened this issue Dec 31, 2021 · 3 comments

Comments

@WestonThayer
Copy link

Describe the bug

The timeout option is documented to cause an error if the request (from start to finish) doesn't complete within the specified ms. This isn't working in Node if the server sends the first response chunk within the timeout, but then hangs.

To Reproduce

const axios = require("axios");
const http = require("http");

const server = http.createServer((req, res) => {
  res.write("partial response");
  setTimeout(() => res.end(), 2000);
});
server.listen(3000);

axios
  .get("http://localhost:3000", { timeout: 1000 })
  .then(() => console.log("got response"))
  .catch((error) => console.log(error));

Expected behavior

axios.get should reject with a timeout error.

Environment

  • Axios Version 0.24.0
  • Node.js Version v14.17.6

Additional context/Screenshots

Under the hood, axios calls follow-redirect's .setTimeout() method, which has a bug follow-redirects/follow-redirects#181.

However, if that bug is fixed to match Node's native http.ClientRequest.setTimeout behavior, axios will have a different bug:

const axios = require("axios");
const http = require("http");

const delay = (ms) => new Promise((resolve) => setTimeout(resolve, ms));

const server = http.createServer(async (req, res) => {
  for (let i = 0; i < 5; i++) {
    await delay(500);
    res.write("some data...");
  }
  res.end();
});
server.listen(3000);

axios
  .get("http://localhost:3000", { timeout: 1000 })
  .then(() => console.log("got response"))
  .catch((error) => console.log(error));

Even though the request takes 2500ms from start to finish (500 * 5), axios won't detect a timeout, because follow-redirects will have defined timeout to match Node's definition — the time a socket is idle between packets, not total request time.

The fix would be to stop relying on follow-redirects setTimeout feature and set our own timer.

@WestonThayer
Copy link
Author

WestonThayer commented Dec 31, 2021

Essentially the same issue reported in #2143, but that issue's fix did improve thing by throwing if the server never sent any response (which is probably way more common than a hung partial response).

@wzijden
Copy link

wzijden commented Feb 3, 2022

I'm seeing severe performance issues after upgrading from axios 0.21.4 to 0.25.0.

It seems those issue are related to this issue. We have a lot of logic relying on being able to cancel request. I'm seeing failed cancels and some requests seem to ignore the timeout setting, running much longer.

We don't use a Node server.

@jasonsaayman
Copy link
Member

Hi 👋

Please could you retry with the latest pre-release version and open a new issue should this error still be relevant?

Thanks

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

No branches or pull requests

3 participants