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

Requests unexpectedly delayed due to an axios internal promise #2609

Closed
SashaKoro opened this issue Dec 18, 2019 · 10 comments
Closed

Requests unexpectedly delayed due to an axios internal promise #2609

SashaKoro opened this issue Dec 18, 2019 · 10 comments

Comments

@SashaKoro
Copy link
Contributor

SashaKoro commented Dec 18, 2019

Description / Context

We are using axios in production and I've stumbled onto a peculiarity recently. Most of our XHR requests are made via axios but there was one that was made by leveraging XMLHttpRequest directly. When looking at the request waterfall on webpagetest (a popular site for analyzing website performance), the request that is leveraging XMLHttpRequest directly was being sent earlier than the requests leveraging axios. Here's a snippet from webpagetest showing this:

image

Request #46 leverages XMLHttpRequest directly and requests #49-54 leverage axios. The thing is: the requests are discovered in the code at around the same time. In fact, several of the axios requests are above request #46 in our code, (here is a simplified version):

image

So the requests are discovered around the same time but the axios calls are delayed by 200-300ms (up to 1000ms in certain cases) Why this is happening is a combination of two factors; axios creating a promise under the hood (before sending the request), and the main thread being busy with JavaScript execution (represented by the pink bar extending from request #30). Here is a call stack of an axios XHR request (Note the Promise.then line):

image

To Reproduce

I've got a fork of axios with a reproducable comparison here: https://github.com/SashaKoro/axios

git clone https://github.com/SashaKoro/axios.git
yarn 
yarn run examples

Open your browser's devtools and navigate to http://localhost:3000/get. The example sends three XHR requests; one via axios, one via XMLHttpRequest and one via fetch. Afterwards, a long JavaScript while loop ties up the main thread. Here is the code: https://github.com/SashaKoro/axios/blob/master/examples/get/index.html If you inspect the Dev Tools (Network tab), you will see something like this:

image

Three requests are sent, two at roughly the same time, but the third after a ~280ms delay (on my hardware) - the axios request! Because axios turned the request into a promise under the hood (before sending the request), the JavaScript event loop continues onward and runs the while loop, which blocks the main thread and the axios request is delayed from getting sent.

Expected behavior

The expected behavior is that an axios request is sent when it is discovered in the consumers code. This is the behavior of vanilla XMLHttpRequest and the Fetch API. And this is usually the behavior in a lab like environment when axios is being tested in isolation. However in a production environment where your website is doing all of the JavaScript this issue can creep up. Delayed XHR calls for critical content cause assets to be rendered later, affecting bounce rate / conversion rate and user experience.

Environment:

  • Axios Version [0.19.0]
  • Browser [Chrome]
  • Browser Version [Latest]
@SashaKoro SashaKoro changed the title Requests unexpectedly delayed due to internal promise Requests unexpectedly delayed due to an axios internal promise Dec 18, 2019
@chinesedfan
Copy link
Collaborator

The expected behavior is that an axios request is sent when it is discovered in the consumers code.

@SashaKoro Really understand your expectation. But I am afraid that axios can't achieve that as it is a promise-based design.

One workaround can be that don't write too much codes after axios calls. If possible, wrap them in another macro task.

axios.get(url)

setTimeout(() => {
   // do other things
}, 0)

@SashaKoro
Copy link
Contributor Author

SashaKoro commented Dec 19, 2019

@chinesedfan Hey, thanks for the quick response. To clarify further; this is not an issue solely because axios returns a Promise (fetch also returns a promise but does not have this behavior)

Looking at axios internal code, the promise is being created here: https://github.com/axios/axios/blob/master/lib/core/Axios.js#L50 The promise is created in the event that the consumer wants to leverage the interceptor API and perform an async operation inside one of their interceptors, something like (copied from a test):

    axios.interceptors.request.use(function (config) {
      return new Promise(function (resolve) {
        // do something async
        setTimeout(function () {
          config.headers.async = 'promise';
          resolve(config);
        }, 100);
      });

However, not everyone leverages interceptors and creating a promise only when they are used might be a good solution that will fix this issue. (It would also be cool to sniff if the interceptor does an async operation before creating the promise; this would allow consumers to leverage synchronous interceptors without paying a penalty) I'm going to look into this further.


So the issue with wrapping the rest of our code in a timeout block is that it is not just a single function (like in the example), the rest of our code is modules and modules of React and Redux code. It would also have to be handled on a case-by-case basis since we are making dozens upon dozens of different axios calls on our website.

@chinesedfan
Copy link
Collaborator

only when they are used might be a good solution that will fix this issue

@SashaKoro Totally agreed with you. The key point is that the real request is send in a then callback, which is so-called micro-task. And it must wait until the current macro task has been finished.

@SashaKoro
Copy link
Contributor Author

SashaKoro commented Dec 22, 2019

I've got a working proof of concept of a possible solution. My proposed changes revolve around creating a Promise only when necessary. It will cover the following scenarios (behavior will not change):

  • If the adapter configuration parameter is specified. This is necessary because the custom adapter function provided by the consumer is not guaranteed to return a Promise. The adapter param looks to be primarily used to simplify testing.

  • If the request is canceled via the cancel token. The Promise needs to be created in order for the error to be caught in the .catch block.

  • If any request interceptors are declared. This has to be done because there is no guaranteed way to see if the interceptor functions that the user provides are sync or async: discussion
    However, add an optional parameter to the axios.interceptors.request.use function that allows the user to specify if their interceptor is synchronous. Then under the hood iterate over all the request interceptors and if they are all set to be synchronous via the param; execute them synchronously and do not create the Promise pre-emptively.

For all other cases*, a Promise will not be pre-emptively created and the request will be sent in a predictable manner at the time the axios.[method] function is declared in the consumers' code.

*This is safe for response interceptors because the request can be kicked off prior to their attachment.

I'm planning to get the proof of concept into a cleaner state, add comments, post a link to the branch and go over the changes in more detail.

@SashaKoro
Copy link
Contributor Author

SashaKoro commented Dec 27, 2019

I've cleaned the code up. Here is the branch: https://github.com/SashaKoro/axios/tree/issue2609

The two files that have relevant changes are Axios.js:

SashaKoro@d450b06#diff-91dcec0516f33811ee5fa71297160b3b

And InterceptorManager.js:

SashaKoro@d450b06#diff-02a02ddaa4d79e5a11b08edecfaf1e75

On that branch, I also changed /get/index.html so you can test the behavior if you want. I added minimal comments as I hopefully have made the code mostly self-documenting. If things look good I can move forward with adding unit tests and documentation.

@SashaKoro
Copy link
Contributor Author

SashaKoro commented Feb 17, 2020

In order to move fast on our end, we ended up forking axios with the changes I've made and deploying to production. The results have been incredible.

This is our homepage before:

Screen Shot 2020-02-17 at 10 59 24 AM

And after:

Screen Shot 2020-02-17 at 10 58 13 AM

Note the timing of the requests relative to the JS execution. Prior to the change, they were all bunched up at the end of the JS execution, now the requests are being made when they are discovered in the code.

Some of the requests are kicked off up to 500ms faster (this is on Chrome over broadband), on 3G connections we are seeing some requests kicked off up to a second faster.

@chinesedfan
Copy link
Collaborator

chinesedfan commented Feb 18, 2020

@SashaKoro Sure. Really appreciated for your continuous updating.

I think it is one of the most annoying things in open source, especially for axios, which is famous and important, but without enough active maintainers. Your pull request is almost there, except for one or two tiny flaws, then I can give it an approval. If no one opposes it, I can even merge it (in fact, it requires more reviewers to check, as it may break the core functionalities). At last, I have no npm publish permission. The real release date still needs lots of our patience.

@SashaKoro
Copy link
Contributor Author

@chinesedfan Hey, no problem! Totally understandable. I appreciate your feedback, I definitely missed a couple of things in the implementation.

@prabhatmishra33
Copy link

prabhatmishra33 commented Feb 22, 2021

Hi, @chinesedfan @SashaKoro I have found similar things going around our server only application but still not confirmed on the actual cause of the delay. Please have a look at this Question if this makes sense.

@Glandos
Copy link

Glandos commented Sep 8, 2021

#2702 has been merged, so this can be closed I assume?

Anyway, thanks for the improvement!

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

5 participants