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

Support AbortController #2020

Merged
merged 20 commits into from Jul 24, 2022
Merged

Support AbortController #2020

merged 20 commits into from Jul 24, 2022

Conversation

jopemachine
Copy link
Contributor

@jopemachine jopemachine commented Mar 29, 2022

Fixes #1511.

Thanks for this awesome lib! :)

I added signal option for supporting AbortController.

when AbortController.abort is called, throws DOMException in the promise of asPromise.ts.

About tests, I copied, pasted and fixed cancel.ts to abort.ts.

I'd appreciate it if you could let me know if I made any mistakes.

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates in both the README and the types.

@jopemachine
Copy link
Contributor Author

It seems that DOMException does not exist until Node 17.
Would it be possible to create and throw AbortError subclassing RequestError instead of throwing DOMException?
Or should the API's target be set to node 17 or higher?

@sindresorhus
Copy link
Owner

See how we did it in sindresorhus/p-retry#59

source/as-promise/index.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

CI is failing. You may need to just rebase from main branch.

@sindresorhus sindresorhus requested a review from szmarczak May 3, 2022
jopemachine added a commit to jopemachine/p-timeout that referenced this pull request May 3, 2022
Copy link
Collaborator

@szmarczak szmarczak left a comment

Choose a reason for hiding this comment

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

I've done some logic changes. Previously the abort signal was only affecting the native HTTP stream, which is incorrect, because it doesn't abort the retry timeout (the request would fail though).

Can you fix the tests please?

@jopemachine
Copy link
Contributor Author

jopemachine commented May 12, 2022

I've done some logic changes. Previously the abort signal was only affecting the native HTTP stream, which is incorrect, because it doesn't abort the retry timeout (the request would fail though).

Can you fix the tests please?

Thanks for your review and for correcting this.

  1. IMHO, I think asPromise's update is still needed to reject the promise correctly and fix tests. I would appreciate your letting me know If I got wrong.

  2. I deleted globalRequest.destroy from asPromise since it should be done with this line. Other lines are same with previous's codes.

@szmarczak
Copy link
Collaborator

IMHO, I think asPromise's update is still needed to reject the promise correctly

Why do you think so?

@jopemachine
Copy link
Contributor Author

jopemachine commented May 13, 2022

Why do you think so?

I think the AbortError should be caught in Promise#catch, or try-catch statement.
I think the error emitted through this.destroy cannot be caught.
To catch this, I think reject should be called in gotPromise when aborted.

But I noticed that I can simplify the code using the firstRequest object, so I reverted 723b960, updated asPromise, AbortError.

@szmarczak
Copy link
Collaborator

I think the error emitted through this.destroy cannot be caught.
To catch this, I think reject should be called in gotPromise when aborted.

It can be caught. destroy() makes Got's Request stream emit error which is caught here and the promise rejects here.

@jopemachine
Copy link
Contributor Author

It can be caught. destroy() makes Got's Request stream emit error which is caught here and the promise rejects here.

Thanks for letting me know.
I never noticed that. I think I jumped to the conclusion that the errors had not been caught just since the test failed.
I just reverted the commits, fixed the tests.

Copy link
Collaborator

@szmarczak szmarczak left a comment

Choose a reason for hiding this comment

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

LGTM. The only concern I have is the use of AbortSignal global. I'm not sure if it breaks when someone uses @types/node v14 in their project.

source/core/options.ts Outdated Show resolved Hide resolved
source/core/index.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

The only concern I have is the use of AbortSignal global. I'm not sure if it breaks when someone uses @types/node v14 in their project.

I think it would. I suggest we just use any for now with a comment to use AbortSignal when targeting Node.js 16.

@jopemachine
Copy link
Contributor Author

jopemachine commented Jul 11, 2022

I think for setting the error code value properly, maybe it would be good to make a new Error class AbortError, which is inherited from RequestError.

And because DOMException is not NodeJS.ErrnoException, I think using DOMException is not possible when generating the AbortError, so the updated code shape looks different from other PRs' ones.

@sindresorhus
Copy link
Owner

I think for setting the error code value properly, maybe it would be good to make a new Error class AbortError, which is inherited from RequestError.

The intention is that we will change to the native DOMException when we can. I don't think we should inherit from RequestError.

And because DOMException is not NodeJS.ErrnoException, I think using DOMException is not possible when generating the AbortError

Why is it not possible?

@jopemachine
Copy link
Contributor Author

I'm sorry, I was confused if I should throw a custom error here or native DOMException (when possible).

Why is it not possible?

I thought using DOMException is not possible because I had thought AbortError should inherit RequestError, which is not correct.

@jopemachine
Copy link
Contributor Author

I think this statement might need to be edited since AbortError doesn't have code and request object.

@sindresorhus
Copy link
Owner

I think this statement might need to be edited since AbortError doesn't have code and request object.

You can add a sentence below it noting the exception.

*/
export class AbortError extends RequestError {
constructor(request: Request, error?: Error) {
super('This operation was aborted.', {...error, code: 'ERR_ABORTED'}, request);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of cloning error, please set this.code like in errors above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since error (abortsignal.reason) is optional here, it seems passing error into the second argument of the constructor cause compile error.

// Compile error occurs

// Argument of type 'ErrnoException | undefined' is not assignable to parameter of type 'Partial<ErrnoException & { code?: string | undefined; }>'.
// Type 'undefined' is not assignable to type 'Partial<ErrnoException & { code?: string | undefined; }>'
super('This operation was aborted.', error, request);
this.code = 'ERR_ABORTED';

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, thanks for clarifying. In this case you can just skip this. Like so:

super('This operation was aborted.', {}, request);
this.name = 'AbortError';
this.code = 'ERR_ABORTED';

The reason is user accessible via controller.signal.reason, so I don't think we need to expose it for now. That might be for #1953

source/core/options.ts Outdated Show resolved Hide resolved
@szmarczak
Copy link
Collaborator

I don't think we should inherit from RequestError.

Why is that? If we strip error metadata then people won't know which request was aborted except the fact that it was aborted. I'm -1 on this.

@sindresorhus
Copy link
Owner

Why is that? If we strip error metadata then people won't know which request was aborted except the fact that it was aborted. I'm -1 on this.

That's a good point. I agree it's more important with context than the consistency of using DOMException.

@szmarczak
Copy link
Collaborator

Friendly reminder: can you address #2020 (comment) please?

@szmarczak szmarczak requested a review from sindresorhus Jul 23, 2022
@sindresorhus sindresorhus merged commit 6a6d2a9 into sindresorhus:main Jul 24, 2022
9 checks passed
@sindresorhus
Copy link
Owner

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

Successfully merging this pull request may close these issues.

Support AbortController
3 participants