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

Fixed cancelToken leakage; Added AbortController support; #3305

Merged

Conversation

DigitalBrainJS
Copy link
Collaborator

@DigitalBrainJS DigitalBrainJS commented Sep 28, 2020

This PR fixes cancelToken and adds an alternative way to cancel Axios request with AbortController.

  • Reworked cancelToken to fix memory leakage in case of using a persistent token for several requests. This can happen because requests cannot unsubscribe from the token after it completed since cancelToken doesn't provide any methods to do this. Possible this is a fix for CancelToken causes memory leak if re-used for multiple requests #3001 issue, but I'm not sure.
  • Added AbortController support. I believe that using the existing API makes it easier to use the library.
  • CancelToken and AbortController can be used simultaneously (to facilitate the transition to the new API)
  • No breaking changes.
const controller = new AbortController();

axios.get('http://localhost/', {
    signal: controller.signal
}).then(
    response=> console.log('Done:', response.data), 
    err=> console.log('Failed:', err)
);

setTimeout(()=>{
    controller.abort();
}, 1000)

Recently published as axios-lab npm fork
Live demo

@Nigui
Copy link

Nigui commented Apr 14, 2021

Any updates about feature integration in master branch ? It seems to be a very useful feature for me.

@DigitalBrainJS
Copy link
Collaborator Author

Just for testing purposes, you can play around with this and some other PRs in this sandbox;

@plandem
Copy link

plandem commented Jun 10, 2021

any blockers? It would be nice to have that support without breaking changes.

@DigitalBrainJS
Copy link
Collaborator Author

@jasonsaayman Is there any chance that this PR will be reviewed? :)

@Ruegen
Copy link

Ruegen commented Jun 25, 2021

@jasonsaayman I would love to see this in too

@JamesHarrisonZa
Copy link

Im migrating our Web applications api client from fetch to axios and we have consumers passing signal so would love this to be supported.

@julesrx
Copy link

julesrx commented Jul 17, 2021

Would like to see this merged too.
I'm seeing around 40Mb more in memory heap when using a single cancel token with 5k+ requests.

@zuzusik
Copy link

zuzusik commented Aug 23, 2021

Would love this one to get in!

@nickserv
Copy link

The proposal for cancelToken has been withdrawn, this should be merged to maintain cancellation support.

@DigitalBrainJS DigitalBrainJS changed the base branch from master to release/v0.22.0 September 15, 2021 13:36
@DigitalBrainJS
Copy link
Collaborator Author

DigitalBrainJS commented Sep 15, 2021

@jasonsaayman rebased this on 0.22.0 just in case ;)

@jasonsaayman jasonsaayman merged commit 9bcff10 into axios:release/v0.22.0 Sep 16, 2021
@jasonsaayman jasonsaayman mentioned this pull request Oct 1, 2021
jasonsaayman added a commit that referenced this pull request Oct 1, 2021
* fix/Avoid package.json import; (#4041)

* Added auto-generated config module `env/data.js` for importing package environment vars without importing the whole `package.json`;
Refactored `http.js` to use `env/data.js` instead of package.json;

* Added `env/data.js`;
Added `env/README.md`;

* Feat/export package version constant (#4065)

* Added auto-generated config module `env/data.js` for importing package environment vars without importing the whole `package.json`;
Refactored `http.js` to use `env/data.js` instead of package.json;

* Added `env/data.js`;
Added `env/README.md`;

* Export package version constant;

* Fixed cancelToken leakage; Added AbortController support; (#3305)

* Fixed cancelToken leakage;
Added AbortController support;

* Fixed typings;

* Documented `signal` option;

* Added processing of early cancellation using AbortController without sending a request;

Co-authored-by: Jay <[email protected]>

* Updating CI to run on release branches

* Fixed default transitional config for custom Axios instance; (#4052)

Refactored `/core/mergeConfig`;

Co-authored-by: Jay <[email protected]>

* Prepping v0.22.0 for release

* Updated date

Co-authored-by: Dmitriy Mozgovoy <[email protected]>
jasonsaayman added a commit that referenced this pull request Oct 6, 2021
* fix/Avoid package.json import; (#4041)

* Added auto-generated config module `env/data.js` for importing package environment vars without importing the whole `package.json`;
Refactored `http.js` to use `env/data.js` instead of package.json;

* Added `env/data.js`;
Added `env/README.md`;

* Feat/export package version constant (#4065)

* Added auto-generated config module `env/data.js` for importing package environment vars without importing the whole `package.json`;
Refactored `http.js` to use `env/data.js` instead of package.json;

* Added `env/data.js`;
Added `env/README.md`;

* Export package version constant;

* Fixed cancelToken leakage; Added AbortController support; (#3305)

* Fixed cancelToken leakage;
Added AbortController support;

* Fixed typings;

* Documented `signal` option;

* Added processing of early cancellation using AbortController without sending a request;

Co-authored-by: Jay <[email protected]>

* Updating CI to run on release branches

* Fixed default transitional config for custom Axios instance; (#4052)

Refactored `/core/mergeConfig`;

Co-authored-by: Jay <[email protected]>

* Prepping v0.22.0 for release

* Updated date

Co-authored-by: Dmitriy Mozgovoy <[email protected]>
mbargiel pushed a commit to mbargiel/axios that referenced this pull request Jan 27, 2022
* fix/Avoid package.json import; (axios#4041)

* Added auto-generated config module `env/data.js` for importing package environment vars without importing the whole `package.json`;
Refactored `http.js` to use `env/data.js` instead of package.json;

* Added `env/data.js`;
Added `env/README.md`;

* Feat/export package version constant (axios#4065)

* Added auto-generated config module `env/data.js` for importing package environment vars without importing the whole `package.json`;
Refactored `http.js` to use `env/data.js` instead of package.json;

* Added `env/data.js`;
Added `env/README.md`;

* Export package version constant;

* Fixed cancelToken leakage; Added AbortController support; (axios#3305)

* Fixed cancelToken leakage;
Added AbortController support;

* Fixed typings;

* Documented `signal` option;

* Added processing of early cancellation using AbortController without sending a request;

Co-authored-by: Jay <[email protected]>

* Updating CI to run on release branches

* Fixed default transitional config for custom Axios instance; (axios#4052)

Refactored `/core/mergeConfig`;

Co-authored-by: Jay <[email protected]>

* Prepping v0.22.0 for release

* Updated date

Co-authored-by: Dmitriy Mozgovoy <[email protected]>
mbargiel pushed a commit to mbargiel/axios that referenced this pull request Jan 27, 2022
* fix/Avoid package.json import; (axios#4041)

* Added auto-generated config module `env/data.js` for importing package environment vars without importing the whole `package.json`;
Refactored `http.js` to use `env/data.js` instead of package.json;

* Added `env/data.js`;
Added `env/README.md`;

* Feat/export package version constant (axios#4065)

* Added auto-generated config module `env/data.js` for importing package environment vars without importing the whole `package.json`;
Refactored `http.js` to use `env/data.js` instead of package.json;

* Added `env/data.js`;
Added `env/README.md`;

* Export package version constant;

* Fixed cancelToken leakage; Added AbortController support; (axios#3305)

* Fixed cancelToken leakage;
Added AbortController support;

* Fixed typings;

* Documented `signal` option;

* Added processing of early cancellation using AbortController without sending a request;

Co-authored-by: Jay <[email protected]>

* Updating CI to run on release branches

* Fixed default transitional config for custom Axios instance; (axios#4052)

Refactored `/core/mergeConfig`;

Co-authored-by: Jay <[email protected]>

* Prepping v0.22.0 for release

* Updated date

Co-authored-by: Dmitriy Mozgovoy <[email protected]>
@CodeJuggernaut
Copy link

CodeJuggernaut commented Jul 28, 2022

error TS2739: Type 'AbortSignal' is missing the following properties from type 'AbortSignal': reason, throwIfAborted
error TS2322: Type 'import("/node_modules/node-abort-controller/index").AbortSignal' is not assignable to type 'AbortSignal'.

The type signatures don't match on [email protected] and [email protected], also doesn't match on [email protected]

The workaround is to cast it as any which is not good, but works until a better solution is found.

          {
            signal: options.signal as any
          }

Comment on lines +38 to +52
this.promise.then = function(onfulfilled) {
var _resolve;
// eslint-disable-next-line func-names
var promise = new Promise(function(resolve) {
token.subscribe(resolve);
_resolve = resolve;
}).then(onfulfilled);

promise.cancel = function reject() {
token.unsubscribe(_resolve);
};

return promise;
};

Copy link

@Hitotsubashi Hitotsubashi Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you had removed code "config.cancelToken.promise.then(function onCanceled(cancel) " from the file named "adapter/xhr.js" in this commit, I think lines 38~52 are no longer needed.

I try to remove lines 38~52 here and then run the script "npm run test", finally find out that all tests are passed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but CancelToken is a public interface, so we can't be sure that user code is somehow not relying on its thenable/promise interface. IMHO, this should be kept as long as the CancelToken exists in the codebase.

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

Successfully merging this pull request may close these issues.