diff --git a/node.gyp b/node.gyp index 7da486ff6d8c7b..1f47e88a3132fe 100644 --- a/node.gyp +++ b/node.gyp @@ -359,9 +359,10 @@ 'src/node_internals.h', 'src/node_javascript.h', 'src/node_mutex.h', - 'src/node_platform.h', 'src/node_perf.h', 'src/node_perf_common.h', + 'src/node_persistent.h', + 'src/node_platform.h', 'src/node_root_certs.h', 'src/node_version.h', 'src/node_watchdog.h', diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 93bd3d4864fd5d..cd9f26d7782d46 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -410,8 +410,8 @@ static void DisablePromiseHook(const FunctionCallbackInfo& args) { class DestroyParam { public: double asyncId; - v8::Persistent target; - v8::Persistent propBag; + Persistent target; + Persistent propBag; }; @@ -426,8 +426,6 @@ void AsyncWrap::WeakCallback(const v8::WeakCallbackInfo& info) { if (val->IsFalse()) { AsyncWrap::EmitDestroy(env, p->asyncId); } - p->target.Reset(); - p->propBag.Reset(); delete p; } diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 900fc2b3edb9ca..51ef46599667df 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -42,12 +42,7 @@ inline BaseObject::BaseObject(Environment* env, v8::Local handle) } -inline BaseObject::~BaseObject() { - CHECK(persistent_handle_.IsEmpty()); -} - - -inline v8::Persistent& BaseObject::persistent() { +inline Persistent& BaseObject::persistent() { return persistent_handle_; } @@ -65,8 +60,7 @@ inline Environment* BaseObject::env() const { template inline void BaseObject::WeakCallback( const v8::WeakCallbackInfo& data) { - std::unique_ptr self(data.GetParameter()); - self->persistent().Reset(); + delete data.GetParameter(); } diff --git a/src/base_object.h b/src/base_object.h index 965683d029e43e..478499bbfeb5b2 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -24,6 +24,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "node_persistent.h" #include "v8.h" namespace node { @@ -33,18 +34,13 @@ class Environment; class BaseObject { public: inline BaseObject(Environment* env, v8::Local handle); - inline virtual ~BaseObject(); + virtual ~BaseObject() = default; // Returns the wrapped object. Returns an empty handle when // persistent.IsEmpty() is true. inline v8::Local object(); - // The parent class is responsible for calling .Reset() on destruction - // when the persistent handle is strong because there is no way for - // BaseObject to know when the handle goes out of scope. - // Weak handles have been reset by the time the destructor runs but - // calling .Reset() again is harmless. - inline v8::Persistent& persistent(); + inline Persistent& persistent(); inline Environment* env() const; @@ -71,7 +67,7 @@ class BaseObject { // position of members in memory are predictable. For more information please // refer to `doc/guides/node-postmortem-support.md` friend int GenDebugSymbols(); - v8::Persistent persistent_handle_; + Persistent persistent_handle_; Environment* env_; }; diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 165a8cda20618b..b8da20346692c3 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -598,7 +598,6 @@ class QueryWrap : public AsyncWrap { ~QueryWrap() override { CHECK_EQ(false, persistent().IsEmpty()); ClearWrap(object()); - persistent().Reset(); } // Subclasses should implement the appropriate Send method. diff --git a/src/env-inl.h b/src/env-inl.h index 0a2adae2bb0ade..f647f428c324ff 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -357,9 +357,6 @@ inline Environment::~Environment() { context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, nullptr); -#define V(PropertyName, TypeName) PropertyName ## _.Reset(); - ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) -#undef V delete[] heap_statistics_buffer_; delete[] heap_space_statistics_buffer_; @@ -541,8 +538,8 @@ void Environment::CreateImmediate(native_immediate_callback cb, native_immediate_callbacks_.push_back({ cb, data, - std::unique_ptr>(obj.IsEmpty() ? - nullptr : new v8::Persistent(isolate_, obj)), + std::unique_ptr>(obj.IsEmpty() ? + nullptr : new Persistent(isolate_, obj)), ref }); immediate_info()->count_inc(1); diff --git a/src/env.cc b/src/env.cc index ca023125c2627c..455f5980731f3f 100644 --- a/src/env.cc +++ b/src/env.cc @@ -296,13 +296,24 @@ void Environment::RunAndClearNativeImmediates() { size_t ref_count = 0; std::vector list; native_immediate_callbacks_.swap(list); - for (const auto& cb : list) { - cb.cb_(this, cb.data_); - if (cb.keep_alive_) - cb.keep_alive_->Reset(); - if (cb.refed_) - ref_count++; - } + auto drain_list = [&]() { + v8::TryCatch try_catch(isolate()); + for (auto it = list.begin(); it != list.end(); ++it) { + it->cb_(this, it->data_); + if (it->refed_) + ref_count++; + if (UNLIKELY(try_catch.HasCaught())) { + FatalException(isolate(), try_catch); + // Bail out, remove the already executed callbacks from list + // and set up a new TryCatch for the other pending callbacks. + std::move_backward(it, list.end(), list.begin() + (list.end() - it)); + list.resize(list.end() - it); + return true; + } + } + return false; + }; + while (drain_list()) {} #ifdef DEBUG CHECK_GE(immediate_info()->count(), count); diff --git a/src/env.h b/src/env.h index 86612bf354d2b3..b08b767cb7cba1 100644 --- a/src/env.h +++ b/src/env.h @@ -800,7 +800,7 @@ class Environment { struct NativeImmediateCallback { native_immediate_callback cb_; void* data_; - std::unique_ptr> keep_alive_; + std::unique_ptr> keep_alive_; bool refed_; }; std::vector native_immediate_callbacks_; @@ -811,8 +811,7 @@ class Environment { v8::Local promise, v8::Local parent); -#define V(PropertyName, TypeName) \ - v8::Persistent PropertyName ## _; +#define V(PropertyName, TypeName) Persistent PropertyName ## _; ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 7dcafa2ce6e842..a3b0209eb3121f 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -98,11 +98,6 @@ HandleWrap::HandleWrap(Environment* env, } -HandleWrap::~HandleWrap() { - CHECK(persistent().IsEmpty()); -} - - void HandleWrap::OnClose(uv_handle_t* handle) { HandleWrap* wrap = static_cast(handle->data); Environment* env = wrap->env(); @@ -120,7 +115,6 @@ void HandleWrap::OnClose(uv_handle_t* handle) { wrap->MakeCallback(env->onclose_string(), 0, nullptr); ClearWrap(wrap->object()); - wrap->persistent().Reset(); delete wrap; } diff --git a/src/handle_wrap.h b/src/handle_wrap.h index 19fd36891a2fed..e7a335f5140253 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -75,7 +75,6 @@ class HandleWrap : public AsyncWrap { v8::Local object, uv_handle_t* handle, AsyncWrap::ProviderType provider); - ~HandleWrap() override; private: friend class Environment; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 7ba1a144711524..e143d316d2e6fa 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -30,7 +30,6 @@ using v8::HandleScope; using v8::Isolate; using v8::Local; using v8::Object; -using v8::Persistent; using v8::String; using v8::Value; diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 0555f5e18c2129..56fb407930fac5 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -97,7 +97,7 @@ class Agent { private: void ToggleAsyncHook(v8::Isolate* isolate, - const v8::Persistent& fn); + const Persistent& fn); node::Environment* parent_env_; std::unique_ptr client_; @@ -109,8 +109,8 @@ class Agent { bool pending_enable_async_hook_; bool pending_disable_async_hook_; - v8::Persistent enable_async_hook_function_; - v8::Persistent disable_async_hook_function_; + Persistent enable_async_hook_function_; + Persistent disable_async_hook_function_; }; } // namespace inspector diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 428d8391f2581a..1cced9420aea6c 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -19,7 +19,6 @@ using v8::Local; using v8::MaybeLocal; using v8::NewStringType; using v8::Object; -using v8::Persistent; using v8::String; using v8::Value; @@ -85,10 +84,6 @@ class JSBindingsConnection : public AsyncWrap { inspector->Connect(&delegate_); } - ~JSBindingsConnection() override { - callback_.Reset(); - } - void OnMessage(Local value) { MakeCallback(callback_.Get(env()->isolate()), 1, &value); } @@ -112,7 +107,6 @@ class JSBindingsConnection : public AsyncWrap { delegate_.Disconnect(); if (!persistent().IsEmpty()) { ClearWrap(object()); - persistent().Reset(); } delete this; } diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 24e73cc3e101c6..6393b4600940a9 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -59,9 +59,6 @@ ModuleWrap::~ModuleWrap() { break; } } - - module_.Reset(); - context_.Reset(); } void ModuleWrap::New(const FunctionCallbackInfo& args) { @@ -215,8 +212,6 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo& args) { module->InstantiateModule(context, ModuleWrap::ResolveCallback); // clear resolve cache on instantiate - for (auto& entry : obj->resolve_cache_) - entry.second.Reset(); obj->resolve_cache_.clear(); if (!ok.FromMaybe(false)) { diff --git a/src/module_wrap.h b/src/module_wrap.h index bedf665165c8f6..04f27decc2aeb4 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -49,11 +49,11 @@ class ModuleWrap : public BaseObject { v8::Local specifier, v8::Local referrer); - v8::Persistent module_; - v8::Persistent url_; + Persistent module_; + Persistent url_; bool linked_ = false; - std::unordered_map> resolve_cache_; - v8::Persistent context_; + std::unordered_map> resolve_cache_; + Persistent context_; }; } // namespace loader diff --git a/src/node_api.cc b/src/node_api.cc index 2c5f3066f728b1..63ce1d8e86955e 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -31,17 +31,11 @@ struct napi_env__ { : isolate(_isolate), last_error(), loop(_loop) {} - ~napi_env__() { - last_exception.Reset(); - wrap_template.Reset(); - function_data_template.Reset(); - accessor_data_template.Reset(); - } v8::Isolate* isolate; - v8::Persistent last_exception; - v8::Persistent wrap_template; - v8::Persistent function_data_template; - v8::Persistent accessor_data_template; + node::Persistent last_exception; + node::Persistent wrap_template; + node::Persistent function_data_template; + node::Persistent accessor_data_template; napi_extended_error_info last_error; int open_handle_scopes = 0; int open_callback_scopes = 0; @@ -274,13 +268,13 @@ static_assert(sizeof(v8::Local) == sizeof(napi_value), "Cannot convert between v8::Local and napi_value"); static -napi_deferred JsDeferredFromV8Persistent(v8::Persistent* local) { +napi_deferred JsDeferredFromNodePersistent(node::Persistent* local) { return reinterpret_cast(local); } static -v8::Persistent* V8PersistentFromJsDeferred(napi_deferred local) { - return reinterpret_cast*>(local); +node::Persistent* NodePersistentFromJsDeferred(napi_deferred local) { + return reinterpret_cast*>(local); } static @@ -360,7 +354,7 @@ class Finalizer { void* _finalize_hint; }; -// Wrapper around v8::Persistent that implements reference counting. +// Wrapper around node::Persistent that implements reference counting. class Reference : private Finalizer { private: Reference(napi_env env, @@ -381,16 +375,6 @@ class Reference : private Finalizer { } } - ~Reference() { - // The V8 Persistent class currently does not reset in its destructor: - // see NonCopyablePersistentTraits::kResetInDestructor = false. - // (Comments there claim that might change in the future.) - // To avoid memory leaks, it is better to reset at this time, however - // care must be taken to avoid attempting this after the Isolate has - // shut down, for example via a static (atexit) destructor. - _persistent.Reset(); - } - public: void* Data() { return _finalize_data; @@ -470,7 +454,7 @@ class Reference : private Finalizer { } } - v8::Persistent _persistent; + node::Persistent _persistent; uint32_t _refcount; bool _delete_self; }; @@ -846,8 +830,8 @@ napi_status ConcludeDeferred(napi_env env, CHECK_ARG(env, result); v8::Local context = env->isolate->GetCurrentContext(); - v8::Persistent* deferred_ref = - V8PersistentFromJsDeferred(deferred); + node::Persistent* deferred_ref = + NodePersistentFromJsDeferred(deferred); v8::Local v8_deferred = v8::Local::New(env->isolate, *deferred_ref); @@ -857,7 +841,6 @@ napi_status ConcludeDeferred(napi_env env, v8_resolver->Resolve(context, v8impl::V8LocalValueFromJsValue(result)) : v8_resolver->Reject(context, v8impl::V8LocalValueFromJsValue(result)); - deferred_ref->Reset(); delete deferred_ref; RETURN_STATUS_IF_FALSE(env, success.FromMaybe(false), napi_generic_failure); @@ -3493,10 +3476,10 @@ napi_status napi_create_promise(napi_env env, CHECK_MAYBE_EMPTY(env, maybe, napi_generic_failure); auto v8_resolver = maybe.ToLocalChecked(); - auto v8_deferred = new v8::Persistent(); + auto v8_deferred = new node::Persistent(); v8_deferred->Reset(env->isolate, v8_resolver); - *deferred = v8impl::JsDeferredFromV8Persistent(v8_deferred); + *deferred = v8impl::JsDeferredFromNodePersistent(v8_deferred); *promise = v8impl::JsValueFromV8LocalValue(v8_resolver->GetPromise()); return GET_RETURN_STATUS(env); } diff --git a/src/node_buffer.cc b/src/node_buffer.cc index dff9d3c0995e02..f9a807602f612c 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -78,7 +78,6 @@ using v8::Local; using v8::Maybe; using v8::MaybeLocal; using v8::Object; -using v8::Persistent; using v8::String; using v8::Uint32Array; using v8::Uint8Array; @@ -103,7 +102,6 @@ class CallbackInfo { FreeCallback callback, char* data, void* hint); - ~CallbackInfo(); Persistent persistent_; FreeCallback const callback_; char* const data_; @@ -147,11 +145,6 @@ CallbackInfo::CallbackInfo(Isolate* isolate, } -CallbackInfo::~CallbackInfo() { - persistent_.Reset(); -} - - void CallbackInfo::WeakCallback( const WeakCallbackInfo& data) { CallbackInfo* self = data.GetParameter(); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index aac72514575695..f49a2362769bc2 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -50,7 +50,6 @@ using v8::NamedPropertyHandlerConfiguration; using v8::Nothing; using v8::Object; using v8::ObjectTemplate; -using v8::Persistent; using v8::PropertyAttribute; using v8::PropertyCallbackInfo; using v8::PropertyDescriptor; @@ -110,11 +109,6 @@ ContextifyContext::ContextifyContext( } -ContextifyContext::~ContextifyContext() { - context_.Reset(); -} - - // This is an object that just keeps an internal pointer to this // ContextifyContext. It's passed to the NamedPropertyHandler. If we // pass the main JavaScript context object we're embedded in, then the @@ -1158,11 +1152,6 @@ class ContextifyScript : public BaseObject { : BaseObject(env, object) { MakeWeak(this); } - - - ~ContextifyScript() override { - script_.Reset(); - } }; diff --git a/src/node_contextify.h b/src/node_contextify.h index c2b5b4dd9c93aa..e6b7e0a9e080f2 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -15,13 +15,12 @@ class ContextifyContext { enum { kSandboxObjectIndex = 1 }; Environment* const env_; - v8::Persistent context_; + Persistent context_; public: ContextifyContext(Environment* env, v8::Local sandbox_obj, v8::Local options_obj); - ~ContextifyContext(); v8::Local CreateDataWrapper(Environment* env); v8::Local CreateV8Context(Environment* env, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index eb62fc1bcdc315..0f6212b81b954c 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -107,7 +107,6 @@ using v8::MaybeLocal; using v8::Null; using v8::Object; using v8::ObjectTemplate; -using v8::Persistent; using v8::PropertyAttribute; using v8::ReadOnly; using v8::Signature; @@ -2804,7 +2803,6 @@ void SSLWrap::CertCbDone(const FunctionCallbackInfo& args) { if (cons->HasInstance(ctx)) { SecureContext* sc; ASSIGN_OR_RETURN_UNWRAP(&sc, ctx.As()); - w->sni_context_.Reset(); w->sni_context_.Reset(env->isolate(), ctx); int rv; @@ -3601,7 +3599,8 @@ void Connection::GetServername(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&conn, args.Holder()); if (conn->is_server() && !conn->servername_.IsEmpty()) { - args.GetReturnValue().Set(conn->servername_); + args.GetReturnValue().Set( + PersistentToLocal(args.GetIsolate(), conn->servername_)); } else { args.GetReturnValue().Set(false); } @@ -5562,7 +5561,6 @@ class PBKDF2Request : public AsyncWrap { keylen_ = 0; ClearWrap(object()); - persistent().Reset(); } uv_work_t* work_req() { @@ -5729,7 +5727,6 @@ class RandomBytesRequest : public AsyncWrap { ~RandomBytesRequest() override { ClearWrap(object()); - persistent().Reset(); } uv_work_t* work_req() { diff --git a/src/node_crypto.h b/src/node_crypto.h index b866117f844358..f1efa811985681 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -225,14 +225,6 @@ class SSLWrap { SSL_SESSION_free(next_sess_); next_sess_ = nullptr; } - -#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - sni_context_.Reset(); -#endif - -#ifdef NODE__HAVE_TLSEXT_STATUS_CB - ocsp_response_.Reset(); -#endif // NODE__HAVE_TLSEXT_STATUS_CB } inline SSL* ssl() const { return ssl_; } @@ -354,11 +346,11 @@ class SSLWrap { ClientHelloParser hello_parser_; #ifdef NODE__HAVE_TLSEXT_STATUS_CB - v8::Persistent ocsp_response_; + Persistent ocsp_response_; #endif // NODE__HAVE_TLSEXT_STATUS_CB #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - v8::Persistent sni_context_; + Persistent sni_context_; #endif friend class SecureContext; @@ -380,13 +372,13 @@ class Connection : public AsyncWrap, public SSLWrap { void NewSessionDoneCb(); #ifndef OPENSSL_NO_NEXTPROTONEG - v8::Persistent npnProtos_; - v8::Persistent selectedNPNProto_; + Persistent npnProtos_; + Persistent selectedNPNProto_; #endif #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - v8::Persistent sniObject_; - v8::Persistent servername_; + Persistent sniObject_; + Persistent servername_; #endif size_t self_size() const override { return sizeof(*this); } diff --git a/src/node_file.cc b/src/node_file.cc index 9df13be5bd2dff..10655e54e54e90 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -366,7 +366,8 @@ class fs_req_wrap { After(uv_req); \ req_wrap = nullptr; \ } else { \ - args.GetReturnValue().Set(req_wrap->persistent()); \ + args.GetReturnValue().Set( \ + PersistentToLocal(env->isolate(), req_wrap->persistent())); \ } #define ASYNC_CALL(func, req, encoding, ...) \ @@ -1140,7 +1141,8 @@ static void WriteString(const FunctionCallbackInfo& args) { return; } - return args.GetReturnValue().Set(req_wrap->persistent()); + return args.GetReturnValue().Set( + PersistentToLocal(env->isolate(), req_wrap->persistent())); } diff --git a/src/node_http2.cc b/src/node_http2.cc index 5240ea8b5ca864..7650969f8639ce 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -278,8 +278,6 @@ Http2Session::Http2Settings::Http2Settings( Http2Session::Http2Settings::~Http2Settings() { if (!object().IsEmpty()) ClearWrap(object()); - persistent().Reset(); - CHECK(persistent().IsEmpty()); } // Generates a Buffer that contains the serialized payload of a SETTINGS @@ -533,10 +531,6 @@ Http2Session::Http2Session(Environment* env, Http2Session::~Http2Session() { CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0); - if (!object().IsEmpty()) - ClearWrap(object()); - persistent().Reset(); - CHECK(persistent().IsEmpty()); DEBUG_HTTP2SESSION(this, "freeing nghttp2 session"); nghttp2_session_del(session_); } @@ -1706,6 +1700,14 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { stream_buf_ = uv_buf_init(nullptr, 0); } +bool Http2Session::HasWritesOnSocketForStream(Http2Stream* stream) { + for (const nghttp2_stream_write& wr : outgoing_buffers_) { + if (wr.req_wrap != nullptr && wr.req_wrap->stream() == stream) + return true; + } + return false; +} + // Every Http2Session session is tightly bound to a single i/o StreamBase // (typically a net.Socket or tls.TLSSocket). The lifecycle of the two is // tightly coupled with all data transfer between the two happening at the @@ -1759,15 +1761,11 @@ Http2Stream::Http2Stream( Http2Stream::~Http2Stream() { + DEBUG_HTTP2STREAM(this, "tearing down stream"); if (session_ != nullptr) { session_->RemoveStream(this); session_ = nullptr; } - - if (!object().IsEmpty()) - ClearWrap(object()); - persistent().Reset(); - CHECK(persistent().IsEmpty()); } // Notify the Http2Stream that a new block of HEADERS is being processed. @@ -1845,7 +1843,7 @@ inline void Http2Stream::Destroy() { Http2Stream* stream = static_cast(data); // Free any remaining outgoing data chunks here. This should be done // here because it's possible for destroy to have been called while - // we still have qeueued outbound writes. + // we still have queued outbound writes. while (!stream->queue_.empty()) { nghttp2_stream_write& head = stream->queue_.front(); if (head.req_wrap != nullptr) @@ -1853,7 +1851,11 @@ inline void Http2Stream::Destroy() { stream->queue_.pop(); } - delete stream; + // We can destroy the stream now if there are no writes for it + // already on the socket. Otherwise, we'll wait for the garbage collector + // to take care of cleaning up. + if (!stream->session()->HasWritesOnSocketForStream(stream)) + delete stream; }, this, this->object()); statistics_.end_time = uv_hrtime(); @@ -2784,8 +2786,6 @@ Http2Session::Http2Ping::Http2Ping( Http2Session::Http2Ping::~Http2Ping() { if (!object().IsEmpty()) ClearWrap(object()); - persistent().Reset(); - CHECK(persistent().IsEmpty()); } void Http2Session::Http2Ping::Send(uint8_t* payload) { diff --git a/src/node_http2.h b/src/node_http2.h index 217c19c09287af..8f6662a0160bec 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -878,6 +878,9 @@ class Http2Session : public AsyncWrap, public StreamListener { // Removes a stream instance from this session inline void RemoveStream(Http2Stream* stream); + // Indicates whether there currently exist outgoing buffers for this stream. + bool HasWritesOnSocketForStream(Http2Stream* stream); + // Write data to the session inline ssize_t Write(const uv_buf_t* bufs, size_t nbufs); diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index d4044f8bbeea7b..207310f4068f43 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -157,7 +157,6 @@ class Parser : public AsyncWrap, public StreamListener { ~Parser() override { ClearWrap(object()); - persistent().Reset(); } diff --git a/src/node_internals.h b/src/node_internals.h index ced92da3216a28..af716c83997c78 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -25,6 +25,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "node.h" +#include "node_persistent.h" #include "util-inl.h" #include "env-inl.h" #include "uv.h" @@ -214,7 +215,7 @@ class Environment; template inline v8::Local PersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent); + const Persistent& persistent); // Creates a new context with Node.js-specific tweaks. Currently, it removes // the `v8BreakIterator` property from the global `Intl` object if present. diff --git a/src/node_persistent.h b/src/node_persistent.h new file mode 100644 index 00000000000000..762842dd4bd373 --- /dev/null +++ b/src/node_persistent.h @@ -0,0 +1,30 @@ +#ifndef SRC_NODE_PERSISTENT_H_ +#define SRC_NODE_PERSISTENT_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "v8.h" + +namespace node { + +template +struct ResetInDestructorPersistentTraits { + static const bool kResetInDestructor = true; + template + // Disallow copy semantics by leaving this unimplemented. + inline static void Copy( + const v8::Persistent&, + v8::Persistent>*); +}; + +// v8::Persistent does not reset the object slot in its destructor. That is +// acknowledged as a flaw in the V8 API and expected to change in the future +// but for now node::Persistent is the easier and safer alternative. +template +using Persistent = v8::Persistent>; + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_NODE_PERSISTENT_H_ diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 8ef4383e0355de..388630d507a433 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -46,7 +46,6 @@ using v8::HandleScope; using v8::Local; using v8::Number; using v8::Object; -using v8::Persistent; using v8::String; using v8::Uint32Array; using v8::Value; diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index 4a7984e649c733..11b1389fa0e771 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -25,7 +25,6 @@ template ReqWrap::~ReqWrap() { CHECK_EQ(req_.data, this); // Assert that someone has called Dispatched(). CHECK_EQ(false, persistent().IsEmpty()); - persistent().Reset(); } template diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 008f9c01513bc0..81adf7a866b927 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -223,8 +223,8 @@ inline StreamWriteResult StreamBase::Write( return StreamWriteResult { async, err, req_wrap }; } -template -SimpleShutdownWrap::SimpleShutdownWrap( +template +SimpleShutdownWrap::SimpleShutdownWrap( StreamBase* stream, v8::Local req_wrap_obj) : ShutdownWrap(stream, req_wrap_obj), @@ -234,14 +234,9 @@ SimpleShutdownWrap::SimpleShutdownWrap( Wrap(req_wrap_obj, static_cast(this)); } -template -SimpleShutdownWrap::~SimpleShutdownWrap() { +template +SimpleShutdownWrap::~SimpleShutdownWrap() { ClearWrap(static_cast(this)->object()); - if (kResetPersistent) { - auto& persistent = static_cast(this)->persistent(); - CHECK_EQ(persistent.IsEmpty(), false); - persistent.Reset(); - } } inline ShutdownWrap* StreamBase::CreateShutdownWrap( @@ -249,8 +244,8 @@ inline ShutdownWrap* StreamBase::CreateShutdownWrap( return new SimpleShutdownWrap(this, object); } -template -SimpleWriteWrap::SimpleWriteWrap( +template +SimpleWriteWrap::SimpleWriteWrap( StreamBase* stream, v8::Local req_wrap_obj) : WriteWrap(stream, req_wrap_obj), @@ -260,14 +255,9 @@ SimpleWriteWrap::SimpleWriteWrap( Wrap(req_wrap_obj, static_cast(this)); } -template -SimpleWriteWrap::~SimpleWriteWrap() { +template +SimpleWriteWrap::~SimpleWriteWrap() { ClearWrap(static_cast(this)->object()); - if (kResetPersistent) { - auto& persistent = static_cast(this)->persistent(); - CHECK_EQ(persistent.IsEmpty(), false); - persistent.Reset(); - } } inline WriteWrap* StreamBase::CreateWriteWrap( diff --git a/src/stream_base.h b/src/stream_base.h index 59b8ee7b7221f0..8af05059f49e47 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -322,7 +322,7 @@ class StreamBase : public StreamResource { // `OtherBase` must have a constructor that matches the `AsyncWrap` // constructors’s (Environment*, Local, AsyncWrap::Provider) signature // and be a subclass of `AsyncWrap`. -template +template class SimpleShutdownWrap : public ShutdownWrap, public OtherBase { public: SimpleShutdownWrap(StreamBase* stream, @@ -333,7 +333,7 @@ class SimpleShutdownWrap : public ShutdownWrap, public OtherBase { size_t self_size() const override { return sizeof(*this); } }; -template +template class SimpleWriteWrap : public WriteWrap, public OtherBase { public: SimpleWriteWrap(StreamBase* stream, diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index e1df9edd39e151..27fe48d1165c75 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -264,8 +264,8 @@ void LibuvStreamWrap::SetBlocking(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(uv_stream_set_blocking(wrap->stream(), enable)); } -typedef SimpleShutdownWrap, false> LibuvShutdownWrap; -typedef SimpleWriteWrap, false> LibuvWriteWrap; +typedef SimpleShutdownWrap> LibuvShutdownWrap; +typedef SimpleWriteWrap> LibuvWriteWrap; ShutdownWrap* LibuvStreamWrap::CreateShutdownWrap(Local object) { return new LibuvShutdownWrap(this, object); diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index a0a58fb1b5cc8d..61b08217b8f129 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -174,11 +174,6 @@ TCPWrap::TCPWrap(Environment* env, Local object, ProviderType provider) } -TCPWrap::~TCPWrap() { - CHECK(persistent().IsEmpty()); -} - - void TCPWrap::SetNoDelay(const FunctionCallbackInfo& args) { TCPWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, diff --git a/src/tcp_wrap.h b/src/tcp_wrap.h index a7f6b1901981f6..2ab50f1fdcdfab 100644 --- a/src/tcp_wrap.h +++ b/src/tcp_wrap.h @@ -55,7 +55,6 @@ class TCPWrap : public ConnectionWrap { TCPWrap(Environment* env, v8::Local object, ProviderType provider); - ~TCPWrap(); static void New(const v8::FunctionCallbackInfo& args); static void SetNoDelay(const v8::FunctionCallbackInfo& args); diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 18b56afece9e09..7ff49522438d65 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -86,12 +86,7 @@ TLSWrap::TLSWrap(Environment* env, TLSWrap::~TLSWrap() { enc_in_ = nullptr; enc_out_ = nullptr; - sc_ = nullptr; - -#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - sni_context_.Reset(); -#endif // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB } @@ -852,7 +847,6 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) { return SSL_TLSEXT_ERR_NOACK; } - p->sni_context_.Reset(); p->sni_context_.Reset(env->isolate(), ctx); SecureContext* sc = Unwrap(ctx.As()); diff --git a/src/util-inl.h b/src/util-inl.h index c5a25c91ffb088..d07cfea9227fbe 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -168,7 +168,7 @@ inline ContainerOfHelper ContainerOf(Inner Outer::*field, template inline v8::Local PersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent) { + const Persistent& persistent) { if (persistent.IsWeak()) { return WeakPersistentToLocal(isolate, persistent); } else { @@ -178,15 +178,15 @@ inline v8::Local PersistentToLocal( template inline v8::Local StrongPersistentToLocal( - const v8::Persistent& persistent) { + const Persistent& persistent) { return *reinterpret_cast*>( - const_cast*>(&persistent)); + const_cast*>(&persistent)); } template inline v8::Local WeakPersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent) { + const Persistent& persistent) { return v8::Local::New(isolate, persistent); } diff --git a/src/util.h b/src/util.h index 21c566a4ca6cd6..7c679952d5fb1f 100644 --- a/src/util.h +++ b/src/util.h @@ -24,6 +24,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "node_persistent.h" #include "v8.h" #include @@ -218,7 +219,7 @@ inline ContainerOfHelper ContainerOf(Inner Outer::*field, template inline v8::Local PersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent); + const Persistent& persistent); // Unchecked conversion from a non-weak Persistent to Local, // use with care! @@ -227,12 +228,12 @@ inline v8::Local PersistentToLocal( // scope, it will destroy the reference to the object. template inline v8::Local StrongPersistentToLocal( - const v8::Persistent& persistent); + const Persistent& persistent); template inline v8::Local WeakPersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent); + const Persistent& persistent); // Convenience wrapper around v8::String::NewFromOneByte(). inline v8::Local OneByteString(v8::Isolate* isolate, diff --git a/test/parallel/test-http2-write-finishes-after-stream-destroy.js b/test/parallel/test-http2-write-finishes-after-stream-destroy.js new file mode 100644 index 00000000000000..3b2dd4bcd4e548 --- /dev/null +++ b/test/parallel/test-http2-write-finishes-after-stream-destroy.js @@ -0,0 +1,62 @@ +// Flags: --expose-gc +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); +const makeDuplexPair = require('../common/duplexpair'); + +// Make sure the Http2Stream destructor works, since we don't clean the +// stream up like we would otherwise do. +process.on('exit', global.gc); + +{ + const { clientSide, serverSide } = makeDuplexPair(); + + let serverSideHttp2Stream; + let serverSideHttp2StreamDestroyed = false; + const server = http2.createServer(); + server.on('stream', common.mustCall((stream, headers) => { + serverSideHttp2Stream = stream; + stream.respond({ + 'content-type': 'text/html', + ':status': 200 + }); + + const originalWrite = serverSide._write; + serverSide._write = (buf, enc, cb) => { + if (serverSideHttp2StreamDestroyed) { + serverSide.destroy(); + serverSide.write = () => {}; + } else { + setImmediate(() => { + originalWrite.call(serverSide, buf, enc, () => setImmediate(cb)); + }); + } + }; + + // Enough data to fit into a single *session* window, + // not enough data to fit into a single *stream* window. + stream.write(Buffer.alloc(40000)); + })); + + server.emit('connection', serverSide); + + const client = http2.connect('http://localhost:80', { + createConnection: common.mustCall(() => clientSide) + }); + + const req = client.request({ ':path': '/' }); + + req.on('response', common.mustCall((headers) => { + assert.strictEqual(headers[':status'], 200); + })); + + req.on('data', common.mustCallAtLeast(() => { + if (!serverSideHttp2StreamDestroyed) { + serverSideHttp2Stream.destroy(); + serverSideHttp2StreamDestroyed = true; + } + })); +}