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

feat(core): support AbortSignal in readFile #10943

Merged
merged 6 commits into from
Jun 22, 2021

Conversation

benjamingr
Copy link
Contributor

See discussion in #10181

cc @lucacasonato

@lucacasonato lucacasonato added this to the 1.12.0 milestone Jun 13, 2021
@benjamingr
Copy link
Contributor Author

Let me know if there is anything else I should change here - if this is fine I'll go ahead and open PRs for some of the other APIs

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Awesome! The implementation looks great. Just confirming the semantics here:

  1. Initiating a readFile with an already aborted abort signal immediately (no microtask delay) rejects the returned promise.
  2. Aborting a readFile that is completed already (returned promise is resolved) does nothing.
  3. If the promise returned from readFile is still pending and the operation is aborted, the readFile call will always reject with an abort error, even if the read completes in the same event loop cycle.

Also we'll have to wait with landing until the 1.12 merge window opens in a couple of days. I want to get another review from @bartlomieju before merging though.

@benjamingr
Copy link
Contributor Author

Initiating a readFile with an already aborted abort signal immediately (no microtask delay) rejects the returned promise.

That sounds very reasonable to me and is "regular" promise semantics - the promise should be rejected immediately but reacting to that rejection will always happen after a microtick. (That is, await and then are the part that wait a microtick and promise creation is typically synchronous if possible).

Aborting a readFile that is completed already (returned promise is resolved) does nothing.

I think the behaviour should be the same as fetch so this is also the behaviour we've picked for Node.js - but if there are compelling alternatives I don't feel particularly strongly about it.

If the promise returned from readFile is still pending and the operation is aborted, the readFile call will always reject with an abort error, even if the read completes in the same event loop cycle.

That's also my expectation though I'd find returning the fulfilled promise also acceptable - cancellation is "best effort" semantics.


Also we'll have to wait with landing until the 1.12 merge window opens in a couple of days.

That sounds fine to me - I'll wait for that and then follow up with other APIs then.

@bartlomieju bartlomieju requested a review from lucacasonato June 21, 2021 23:33
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM. Requesting additional reviews from @ry, @piscisaureus, @kitsonk, or @bartlomieju because it touches a public API.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thank you @benjamingr

@ry ry merged commit 20b0a51 into denoland:main Jun 22, 2021
@benjamingr
Copy link
Contributor Author

Thanks, I will follow up with PRs for the other APIs :)

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.

4 participants