From 66da592415a66d071f8ad66f3f86eff5b006575c Mon Sep 17 00:00:00 2001 From: Philip Chimento Date: Fri, 19 Jun 2020 15:40:26 -0700 Subject: [PATCH 1/2] src: Move GetDOMException() into Environment 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: https://github.com/nodejs/node/issues/31971 --- src/api/environment.cc | 15 +++++++++++++++ src/node_internals.h | 1 + src/node_messaging.cc | 15 --------------- test/cctest/test_environment.cc | 19 +++++++++++++++++++ 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 8e5caac20bd3a6..0c2f761b06860d 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -517,6 +517,21 @@ MaybeLocal GetPerContextExports(Local context) { return handle_scope.Escape(exports); } +MaybeLocal GetDOMException(Local context) { + Isolate* isolate = context->GetIsolate(); + Local per_context_bindings; + Local domexception_ctor_val; + if (!GetPerContextExports(context).ToLocal(&per_context_bindings) || + !per_context_bindings + ->Get(context, FIXED_ONE_BYTE_STRING(isolate, "DOMException")) + .ToLocal(&domexception_ctor_val)) { + return MaybeLocal(); + } + CHECK(domexception_ctor_val->IsFunction()); + Local domexception_ctor = domexception_ctor_val.As(); + return domexception_ctor; +} + // Any initialization logic should be performed in // InitializeContext, because embedders don't necessarily // call NewContext and so they will experience breakages. diff --git a/src/node_internals.h b/src/node_internals.h index dffaa084db409b..b363c745f8ed0c 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -301,6 +301,7 @@ v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params, v8::MaybeLocal StartExecution(Environment* env, StartExecutionCallback cb = nullptr); v8::MaybeLocal GetPerContextExports(v8::Local context); +v8::MaybeLocal GetDOMException(v8::Local context); v8::MaybeLocal ExecuteBootstrapper( Environment* env, const char* id, diff --git a/src/node_messaging.cc b/src/node_messaging.cc index c615293570475c..a9a09b25cd2aef 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -225,21 +225,6 @@ MaybeLocal GetEmitMessageFunction(Local context) { return emit_message_val.As(); } -MaybeLocal GetDOMException(Local context) { - Isolate* isolate = context->GetIsolate(); - Local per_context_bindings; - Local domexception_ctor_val; - if (!GetPerContextExports(context).ToLocal(&per_context_bindings) || - !per_context_bindings->Get(context, - FIXED_ONE_BYTE_STRING(isolate, "DOMException")) - .ToLocal(&domexception_ctor_val)) { - return MaybeLocal(); - } - CHECK(domexception_ctor_val->IsFunction()); - Local domexception_ctor = domexception_ctor_val.As(); - return domexception_ctor; -} - void ThrowDataCloneException(Local context, Local message) { Isolate* isolate = context->GetIsolate(); Local argv[] = {message, diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 27706044b800f6..448d91217ed514 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -559,3 +559,22 @@ TEST_F(EnvironmentTest, SetImmediateMicrotasks) { EXPECT_EQ(called, 1); } + +TEST_F(EnvironmentTest, GetDOMExceptionTest) { + // Test that GetDOMException() returns a constructor named "DOMException" + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env env{handle_scope, argv}; + + v8::Local domexception_ctor = + node::GetDOMException(env.context()).ToLocalChecked(); + + CHECK(domexception_ctor->IsConstructor()); + + char domexception[13]; + v8::Local v_name = domexception_ctor->GetName(); + CHECK(v_name->IsString()); + v8::Local name = v_name->ToString(env.context()).ToLocalChecked(); + name->WriteUtf8(isolate_, domexception, 13); + EXPECT_STREQ(domexception, "DOMException"); +} From 8236b525c650bc29fe70bc68db501d7ea22a4a5b Mon Sep 17 00:00:00 2001 From: Philip Chimento Date: Fri, 19 Jun 2020 15:42:42 -0700 Subject: [PATCH 2/2] fs: Expose uv_cancel() method on FSReq This adds a FSReqBase::Cancel() method which calls uv_cancel(). This shows up in JS as a cancel() method on FSReqCallback and FSReqPromise. Refs: https://github.com/nodejs/node/issues/31971 --- src/node_file-inl.h | 5 ++ src/node_file.cc | 36 ++++++++++++ src/node_file.h | 8 +++ test/parallel/test-fsreqcallback-cancel.js | 65 ++++++++++++++++++++++ 4 files changed, 114 insertions(+) create mode 100644 test/parallel/test-fsreqcallback-cancel.js diff --git a/src/node_file-inl.h b/src/node_file-inl.h index 2ad8c73f05e490..6bd5afffe9fe51 100644 --- a/src/node_file-inl.h +++ b/src/node_file-inl.h @@ -182,12 +182,17 @@ void FSReqPromise::Reject(v8::Local reject) { object()->Get(env()->context(), env()->promise_string()).ToLocalChecked(); v8::Local resolver = value.As(); + MaybeReplaceWithAbortError(&reject); USE(resolver->Reject(env()->context(), reject).FromJust()); } template void FSReqPromise::Resolve(v8::Local value) { finished_ = true; + if (IsCanceled()) { + Reject(v8::Undefined(env()->isolate())); + return; + } v8::HandleScope scope(env()->isolate()); InternalCallbackScope callback_scope(this); v8::Local val = diff --git a/src/node_file.cc b/src/node_file.cc index 4c368abb064fe1..44e132da461838 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -130,6 +130,32 @@ FileHandleReadWrap::~FileHandleReadWrap() {} FSReqBase::~FSReqBase() {} +void FSReqBase::Cancel(const v8::FunctionCallbackInfo& args) { + FSReqBase* self; + ASSIGN_OR_RETURN_UNWRAP(&self, args.Holder()); + self->ReqWrap::Cancel(); + self->is_canceled_ = true; +} + +void FSReqBase::MaybeReplaceWithAbortError(v8::Local* reject) { + if (!is_canceled_) return; + + Local exception; + Local domexception_ctor; + if (!GetDOMException(env()->context()).ToLocal(&domexception_ctor)) return; + + Local argv[] = { + FIXED_ONE_BYTE_STRING(env()->isolate(), "Operation was cancelled."), + FIXED_ONE_BYTE_STRING(env()->isolate(), "AbortError"), + }; + Local abort_error; + if (!domexception_ctor->NewInstance(env()->context(), arraysize(argv), argv) + .ToLocal(&abort_error)) + return; + + *reject = abort_error; +} + void FSReqBase::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("continuation_data", continuation_data_); } @@ -555,6 +581,7 @@ int FileHandle::DoShutdown(ShutdownWrap* req_wrap) { void FSReqCallback::Reject(Local reject) { + MaybeReplaceWithAbortError(&reject); MakeCallback(env()->oncomplete_string(), 1, &reject); } @@ -563,6 +590,13 @@ void FSReqCallback::ResolveStat(const uv_stat_t* stat) { } void FSReqCallback::Resolve(Local value) { + // If FSReqBase::Cancel() was called, then reject the request rather than + // resolving. + if (IsCanceled()) { + Reject(Undefined(env()->isolate())); + return; + } + Local argv[2] { Null(env()->isolate()), value @@ -2462,6 +2496,7 @@ void Initialize(Local target, fst->InstanceTemplate()->SetInternalFieldCount( FSReqBase::kInternalFieldCount); fst->Inherit(AsyncWrap::GetConstructorTemplate(env)); + env->SetProtoMethod(fst, "cancel", FSReqBase::Cancel); Local wrapString = FIXED_ONE_BYTE_STRING(isolate, "FSReqCallback"); fst->SetClassName(wrapString); @@ -2485,6 +2520,7 @@ void Initialize(Local target, // Create Function Template for FSReqPromise Local fpt = FunctionTemplate::New(isolate); fpt->Inherit(AsyncWrap::GetConstructorTemplate(env)); + env->SetProtoMethod(fpt, "cancel", FSReqBase::Cancel); Local promiseString = FIXED_ONE_BYTE_STRING(isolate, "FSReqPromise"); fpt->SetClassName(promiseString); diff --git a/src/node_file.h b/src/node_file.h index 2b157de5eb485d..8f4e1ea3a8d405 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -85,6 +85,9 @@ class FSReqBase : public ReqWrap { virtual void SetReturnValue( const v8::FunctionCallbackInfo& args) = 0; + // JS-exposed method to cancel the UV request. + static void Cancel(const v8::FunctionCallbackInfo& args); + const char* syscall() const { return syscall_; } const char* data() const { return has_data_ ? *buffer_ : nullptr; } enum encoding encoding() const { return encoding_; } @@ -111,12 +114,17 @@ class FSReqBase : public ReqWrap { BindingData* binding_data() { return binding_data_.get(); } + protected: + bool IsCanceled() { return is_canceled_; } + void MaybeReplaceWithAbortError(v8::Local* value); + private: std::unique_ptr continuation_data_; enum encoding encoding_ = UTF8; bool has_data_ = false; bool use_bigint_ = false; bool is_plain_open_ = false; + bool is_canceled_ = false; const char* syscall_ = nullptr; BaseObjectPtr binding_data_; diff --git a/test/parallel/test-fsreqcallback-cancel.js b/test/parallel/test-fsreqcallback-cancel.js new file mode 100644 index 00000000000000..581a2a450f2225 --- /dev/null +++ b/test/parallel/test-fsreqcallback-cancel.js @@ -0,0 +1,65 @@ +'use strict'; +// Flags: --no-warnings --expose-internals + +const common = require('../common'); +const assert = require('assert'); +const { internalBinding } = require('internal/test/binding'); +const path = require('path'); + +const { FSReqCallback, readdir } = internalBinding('fs'); + +{ + // Return value of cancel() is undefined + const req = new FSReqCallback(); + req.oncomplete = () => {}; + assert.strictEqual(req.cancel(), undefined); +} + + +{ + // Callback is called with AbortError if request is canceled + const req = new FSReqCallback(); + req.oncomplete = common.mustCall((err, files) => { + assert.strictEqual(files, undefined); + assert.strictEqual(err.name, 'AbortError'); + }, 1); + req.cancel(); + readdir(path.toNamespacedPath('../'), 'utf8', false, req); +} + + +{ + // Request is canceled if cancel() called before control returns to main loop + const req = new FSReqCallback(); + req.oncomplete = common.mustCall((err, files) => { + assert.strictEqual(files, undefined); + assert.strictEqual(err.name, 'AbortError'); + }, 1); + readdir(path.toNamespacedPath('../'), 'utf8', false, req); + req.cancel(); +} + + +{ + // Request is still canceled on next tick + const req = new FSReqCallback(); + req.oncomplete = common.mustCall((err, files) => { + assert.strictEqual(files, undefined); + assert.strictEqual(err.name, 'AbortError'); + }, 1); + readdir(path.toNamespacedPath('../'), 'utf8', false, req); + process.nextTick(common.mustCall(() => req.cancel())); +} + + +{ + // Callback is not called a second time if request canceled after it has + // already completed + const req = new FSReqCallback(); + req.oncomplete = common.mustCall((err, files) => { + assert.strictEqual(err, null); + assert.notStrictEqual(files, undefined); + req.cancel(); + }, 1); + readdir(path.toNamespacedPath('../'), 'utf8', false, req); +}