Draft: Cancellation of FSReq

[ptomato]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Draft for early feedback. This adds a cancel() method accessible from JS to FSReqBase which calls uv_cancel() via ReqWrap::Cancel() on the request. This would be a prerequisite for effectively integrating support for AbortController into the fs APIs (#31971).

Looking for feedback on the approach, if that seems sound then I will write tests and documentation.

To see how this is used in my experiments with cancellation, see https://github.com/nodejs/node/compare/master...ptomato:31971-abortcontroller?expand=1

[ptomato]

@addaleax, @jasnell, @Ethan-Arrowood, @benjamingr - I think I heard you volunteer to take a look at this during the collaborator summit 😄

[benjamingr]

Generally looks fine :] Need a few days to review this on a computer with checked out code

[addaleax]

Looks good so far! :)

[ptomato]

Hmm, this is not as straightforward as these patches led me to believe. While writing tests for FSReqBase::Cancel() I discovered that the FSReq callback is not actually called with the AbortError that the code puts into FSReq::Reject(), but instead with libuv's ECANCELED error. So it seems like the AbortError bit is actually not doing anything. But on the other hand looking at https://github.com/nodejs/node/blob/master/src/req_wrap-inl.h#L47-L51 it's also not guaranteed that ReqWrap::Cancel() will actually call uv_cancel(), so I'm guessing in that case the request is rejected with the AbortError after all? I will need to think a bit more about what to do with this.

[ptomato]

I've taken a different approach now, so once again feedback on the approach is welcome. This stores a "cancelled" flag in FSReqBase, and if it is set to true, Resolve() will instead call Reject(), and Reject() will replace any original rejected error with the DOMException AbortError.

There are no tests for FSReqPromise as I'm not sure where that is actually used in lib/fs, it seems it's not possible to get its constructor from the internalBinding like I did with FSReqCallback in the tests.

[ptomato]

@addaleax, @jasnell, @Ethan-Arrowood, @benjamingr - I've rebased this on the latest, any comments on the new approach?

[ptomato]

I probably won't be able to do more work on this until January, but if someone else would like to pick it up sooner, I'm happy to hand it off.

[BridgeAR]

I believe this is actually a better implementation than what we currently have, as far as I know.
I am nevertheless going to close it, since it would require a rebase and a new PR would probably be best for that to pick it up faster and land it.