Draft: Cancellation of FSReq #34080
Draft: Cancellation of FSReq #34080
Conversation
nodejs-github-bot
added
c++
|
@addaleax, @jasnell, @Ethan-Arrowood, @benjamingr - I think I heard you volunteer to take a look at this during the collaborator summit 😄 |
|
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
force-pushed
the
31971-fsreq-cancel
branch
from
fa4e9e2 to
8ca9511
Compare
|
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. |
GetDOMException() was a private function in node_messaging, but we would like to use it to throw AbortErrors when cancelling something via an AbortController. Move it into environment.cc to be in the same place as GetPerContextExports(), and expose it internally in node_internals.h. Refs: nodejs#31971
This adds a FSReqBase::Cancel() method which calls uv_cancel(). This shows up in JS as a cancel() method on FSReqCallback and FSReqPromise. Refs: nodejs#31971
ptomato
force-pushed
the
31971-fsreq-cancel
branch
from
8ca9511 to
8236b52
Compare
|
@addaleax, @jasnell, @Ethan-Arrowood, @benjamingr - I've rebased this on the latest, any comments on the new approach? |
|
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. |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesDraft for early feedback. This adds a
cancel()method accessible from JS toFSReqBasewhich callsuv_cancel()viaReqWrap::Cancel()on the request. This would be a prerequisite for effectively integrating support for AbortController into thefsAPIs (#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