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

Axios doesn't address memory leaks? #3217

Closed
yizhengfeng-jj opened this issue Aug 21, 2020 · 22 comments · Fixed by #3694 or #3771
Closed

Axios doesn't address memory leaks? #3217

yizhengfeng-jj opened this issue Aug 21, 2020 · 22 comments · Fixed by #3694 or #3771
Assignees
Projects
Milestone

Comments

@yizhengfeng-jj
Copy link

Describe the bug

I used the latest version of Axios (v0.20.0-0) with interceptors, interceptors.response.use () after that, the sent request will be viewed through the chrome tool. The returned content will always be saved and will not be released. This problem should be very common and should have been solved earlier. Is there a problem with my configuration? I hope you can get a reply. Thank you very much

To Reproduce

Pseudo code

servers.interceptors.response.use();

Expected behavior

I send requests to intercept, but don't leak memory

Environment

  • Axios Version 0.20.0-0
  • Browser Chrome
  • Browser Version 84.0.4147.125
  • Node.js Version 14.0.1
  • Additional Library Versions React 16.7

Additional context/Screenshots

Add any other context about the problem here. If applicable, add screenshots to help explain.

@dexturr
Copy link

dexturr commented Aug 27, 2020

We also encountered this issue when we bumped from 0.19.2 -> 0.20.0. Reverting managed to fixed the issue.

@yizhengfeng-jj
Copy link
Author

We also encountered this issue when we bumped from 0.19.2 -> 0.20.0. Reverting managed to fixed the issue.

Thank you very much for your reply. Do you mean that 0.19.2 version of Axios does not have the above class leakage problem. I see this problem in 0.18.1 and v.20.0

@dexturr
Copy link

dexturr commented Aug 31, 2020

We were using 0.19.2 with no issues. We bumped to 0.20.0 and an issue arose with a box running out of memory and crashing. We used interceptors and found the response was not being released, as you described. Slowly leaking and eventually dying. We reverted back to 0.19.2 and everything seems to have gone back to ok. I cannot speak for 0.18.1 myself as this was for a prototype and we never used 0.18.1 on it so there may have been bug fixes in between 0.19.2 and 0.18.1.

Otherwise if you're still having issues on 0.19.2 we may have had different root issues. I have never really used axios, so you're probably way ahead of me if it's a config thing. I was just the unlucky person assigned to finding the issue 👍

@haliloncen
Copy link

if you are using to the axios without create instance,
you can try below code;

const axios = axios.create({...}) // instead of axios.get(), post(), put() etc.

It works with 0.20.0 without memory leaks.

@Ks89
Copy link

Ks89 commented Oct 12, 2020

Same issue, I downgraded to 0.19.2 as suggested and it seems to be ok.

@jasonsaayman jasonsaayman added v0.21 and removed v0.21 labels Oct 23, 2020
@KyruCabading
Copy link

Do we still have this memory leak issue?

@dan-j
Copy link

dan-j commented Jan 27, 2021

I've just ran into the same problem but assumed it was my code. Then after some refactoring and the bug still occurring, I searched and came across this issue.

Using 0.19.2, I haven't tried down/upgrading yet.

@water-a
Copy link

water-a commented Feb 2, 2021

I'm on version 0.21.1 and encountering this issue. The data from the responses isn't being garbage collected.

@joquijada
Copy link

joquijada commented Mar 17, 2021

I'm on version 0.21.1 and encountering this issue. The data from the responses isn't being garbage collected.

I experienced the same with version 0.21.1, here's my comment on another issue with I think relates more to this issue. On 0.19.x we had no issues. On 0.21 the memory and file descriptor leak started. I followed verbatim the workaround found in this other comment of that issue and it worked for me. Give it a shot and see if it works. It basically sets a time limit under which the request should have completed, and cancels the request after such timeout has elapsed.

Anothet tidbit, Axios does offer disclaimer in their README that until 1.0 gets released, minor revisions can introduce breaking changes, though I do believe this is a bug, unless I'm missing documentation somewhere.

image

@jasonsaayman jasonsaayman self-assigned this Mar 18, 2021
@jasonsaayman jasonsaayman added this to To do in v0.22.0 via automation Mar 18, 2021
@jasonsaayman jasonsaayman added this to the v0.22.0 milestone Mar 18, 2021
@jasonsaayman
Copy link
Member

This is an issue that needs to be fixed irrespective of SemVer, I am tagging it and will try get to work on it as soon as possible. If anyone wants to give it a shot please try and open a pull request and add me as a reviewer or assign me.

@joquijada
Copy link

joquijada commented Mar 18, 2021

I'll try to take a look at the issue outside of my work hours and submit a PR if no one has beat me to it, by this Sunday unless it turns out to be a bigger type fix. I haven't contributed to this project before so there's also a bit of a ramp up for me. Cheers!

@jasonsaayman
Copy link
Member

@joquijada Thanks, please just tag me in it so that I can have a look and get it merged asap

@joquijada
Copy link

@joquijada Thanks, please just tag me in it so that I can have a look and get it merged asap

I've done some investigation and have narrowed down the problem some. In my case the FD/memory leak happens when options.maxRedirects > 0, in which case lib/adapters/http.js is using the follow-redirects library. Unfortunately in case of redirects, the request.setTimeout() function gets called on the original request only, and not on the redirect requests. This means the sockets hang around forever unless explicitly destroyed, as indicated in the NodeJS documentation,

image

When I change the request URL so that a redirect does not happen, hence the native NodeJS http/https libraries are used, no leak whatsoever since there's only a single request involved.

This is what I think the problem in. Tomorrow I'll write a wrapper around the follow-redirects library to manually set a timeout in all the redirect requests, and see if that solves the issue with memory leaks when there are redirects involved.

@jasonsaayman
Copy link
Member

@joquijada Thanks that sounds great. Very in depth look into this. Please remember to tag me in the pull request then I can get that out ASAP

@DigitalBrainJS
Copy link
Collaborator

DigitalBrainJS commented Mar 23, 2021

@jasonsaayman Probably, this has been fixed in the follow-redirects v1.13.3 . They just had some potential leaks with event handlers in the overloaded abort method.

@joquijada
Copy link

@jasonsaayman Probably, this has been fixed in the follow-redirects v1.13.3 . They just had some potential leaks with event handlers in the overloaded abort method.

Thanks for letting me know because I slept on it and realized it'd be infinitely easier for the fix to come from follow-redirects. I'll check out v1.13.3 of that library.

@jasonsaayman
Copy link
Member

Ok cool thanks I will see if I can bump it without any repercussions.

@jasonsaayman jasonsaayman linked a pull request Mar 23, 2021 that will close this issue
v0.22.0 automation moved this from To do to Done Mar 23, 2021
@joquijada
Copy link

I know this issue was closed, but problem persists in follow-redirects v1.13.3. I've submitted this PR to that product.

@jasonsaayman
Copy link
Member

Ok cool please reference this and I will reopen

@jasonsaayman jasonsaayman reopened this Mar 26, 2021
v0.22.0 automation moved this from Done to In progress Mar 26, 2021
@joquijada
Copy link

joquijada commented Mar 26, 2021

Ok cool please reference this and I will reopen

Done. I've added steps to reproduce that issue.

@joquijada
Copy link

joquijada commented Apr 26, 2021

@jasonsaayman Update: The follow-redirects team has merged in the socket leak fix and created a new release.

@jasonsaayman
Copy link
Member

Thanks I will update Axios

@jasonsaayman jasonsaayman linked a pull request Apr 29, 2021 that will close this issue
v0.22.0 automation moved this from In progress to Done Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v0.22.0
  
Done
10 participants