From e17d314a21c3635b6bf62e124b82780ef31dbd6d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 12 Nov 2019 14:01:08 +0000 Subject: [PATCH] src: remove keep alive option from SetImmediate() This is no longer necessary now that the copyable `BaseObjectPtr` is available (as opposed to the only-movable `v8::Global`). Backport-PR-URL: https://github.com/nodejs/node/pull/32301 PR-URL: https://github.com/nodejs/node/pull/30374 Refs: https://github.com/nodejs/quic/pull/141 Refs: https://github.com/nodejs/quic/pull/149 Refs: https://github.com/nodejs/quic/pull/141 Refs: https://github.com/nodejs/quic/pull/165 Reviewed-By: James M Snell Reviewed-By: David Carlier --- src/cares_wrap.cc | 10 ++++++---- src/env-inl.h | 21 ++++++++------------- src/env.h | 17 ++++------------- src/node_http2.cc | 16 ++++++++++------ src/stream_pipe.cc | 3 ++- src/tls_wrap.cc | 15 +++++++++------ 6 files changed, 39 insertions(+), 43 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index ea29b2d79053fe..8eeaeddf8300d8 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -627,8 +627,6 @@ class QueryWrap : public AsyncWrap { } else { Parse(response_data_->host.get()); } - - delete this; } void* MakeCallbackPointer() { @@ -686,9 +684,13 @@ class QueryWrap : public AsyncWrap { } void QueueResponseCallback(int status) { - env()->SetImmediate([this](Environment*) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment*) { AfterResponse(); - }, object()); + + // Delete once strong_ref goes out of scope. + Detach(); + }); channel_->set_query_last_ok(status != ARES_ECONNREFUSED); channel_->ModifyActivityQueryCount(-1); diff --git a/src/env-inl.h b/src/env-inl.h index 1c3f45ec0e49ec..e5873d8081994d 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -733,13 +733,9 @@ inline void IsolateData::set_options( } template -void Environment::CreateImmediate(Fn&& cb, - v8::Local keep_alive, - bool ref) { +void Environment::CreateImmediate(Fn&& cb, bool ref) { auto callback = std::make_unique>( - std::move(cb), - v8::Global(isolate(), keep_alive), - ref); + std::move(cb), ref); NativeImmediateCallback* prev_tail = native_immediate_callbacks_tail_; native_immediate_callbacks_tail_ = callback.get(); @@ -752,8 +748,8 @@ void Environment::CreateImmediate(Fn&& cb, } template -void Environment::SetImmediate(Fn&& cb, v8::Local keep_alive) { - CreateImmediate(std::move(cb), keep_alive, true); +void Environment::SetImmediate(Fn&& cb) { + CreateImmediate(std::move(cb), true); if (immediate_info()->ref_count() == 0) ToggleImmediateRef(true); @@ -761,8 +757,8 @@ void Environment::SetImmediate(Fn&& cb, v8::Local keep_alive) { } template -void Environment::SetUnrefImmediate(Fn&& cb, v8::Local keep_alive) { - CreateImmediate(std::move(cb), keep_alive, false); +void Environment::SetUnrefImmediate(Fn&& cb) { + CreateImmediate(std::move(cb), false); } Environment::NativeImmediateCallback::NativeImmediateCallback(bool refed) @@ -784,10 +780,9 @@ void Environment::NativeImmediateCallback::set_next( template Environment::NativeImmediateCallbackImpl::NativeImmediateCallbackImpl( - Fn&& callback, v8::Global&& keep_alive, bool refed) + Fn&& callback, bool refed) : NativeImmediateCallback(refed), - callback_(std::move(callback)), - keep_alive_(std::move(keep_alive)) {} + callback_(std::move(callback)) {} template void Environment::NativeImmediateCallbackImpl::Call(Environment* env) { diff --git a/src/env.h b/src/env.h index 8c46b12565c14e..155cd9e8323a58 100644 --- a/src/env.h +++ b/src/env.h @@ -1191,13 +1191,9 @@ class Environment : public MemoryRetainer { // cb will be called as cb(env) on the next event loop iteration. // keep_alive will be kept alive between now and after the callback has run. template - inline void SetImmediate(Fn&& cb, - v8::Local keep_alive = - v8::Local()); + inline void SetImmediate(Fn&& cb); template - inline void SetUnrefImmediate(Fn&& cb, - v8::Local keep_alive = - v8::Local()); + inline void SetUnrefImmediate(Fn&& cb); // This needs to be available for the JS-land setImmediate(). void ToggleImmediateRef(bool ref); @@ -1274,9 +1270,7 @@ class Environment : public MemoryRetainer { private: template - inline void CreateImmediate(Fn&& cb, - v8::Local keep_alive, - bool ref); + inline void CreateImmediate(Fn&& cb, bool ref); inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -1424,14 +1418,11 @@ class Environment : public MemoryRetainer { template class NativeImmediateCallbackImpl final : public NativeImmediateCallback { public: - NativeImmediateCallbackImpl(Fn&& callback, - v8::Global&& keep_alive, - bool refed); + NativeImmediateCallbackImpl(Fn&& callback, bool refed); void Call(Environment* env) override; private: Fn callback_; - v8::Global keep_alive_; }; std::unique_ptr native_immediate_callbacks_head_; diff --git a/src/node_http2.cc b/src/node_http2.cc index be6c7f5cd0efba..0eebe2935e248b 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1550,7 +1550,8 @@ void Http2Session::MaybeScheduleWrite() { HandleScope handle_scope(env()->isolate()); Debug(this, "scheduling write"); flags_ |= SESSION_STATE_WRITE_SCHEDULED; - env()->SetImmediate([this](Environment* env) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { if (session_ == nullptr || !(flags_ & SESSION_STATE_WRITE_SCHEDULED)) { // This can happen e.g. when a stream was reset before this turn // of the event loop, in which case SendPendingData() is called early, @@ -1563,7 +1564,7 @@ void Http2Session::MaybeScheduleWrite() { HandleScope handle_scope(env->isolate()); InternalCallbackScope callback_scope(this); SendPendingData(); - }, object()); + }); } } @@ -2018,7 +2019,8 @@ void Http2Stream::Destroy() { // Wait until the start of the next loop to delete because there // may still be some pending operations queued for this stream. - env()->SetImmediate([this](Environment* env) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { // 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 queued outbound writes. @@ -2032,9 +2034,11 @@ void Http2Stream::Destroy() { // 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 (session() == nullptr || !session()->HasWritesOnSocketForStream(this)) - delete this; - }, object()); + if (session() == nullptr || !session()->HasWritesOnSocketForStream(this)) { + // Delete once strong_ref goes out of scope. + Detach(); + } + }); statistics_.end_time = uv_hrtime(); session_->statistics_.stream_average_duration = diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc index 504b840972f22d..65b5507efe6fd6 100644 --- a/src/stream_pipe.cc +++ b/src/stream_pipe.cc @@ -72,6 +72,7 @@ void StreamPipe::Unpipe() { // Delay the JS-facing part with SetImmediate, because this might be from // inside the garbage collector, so we can’t run JS here. HandleScope handle_scope(env()->isolate()); + BaseObjectPtr strong_ref{this}; env()->SetImmediate([this](Environment* env) { HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -106,7 +107,7 @@ void StreamPipe::Unpipe() { .IsNothing()) { return; } - }, object()); + }); } uv_buf_t StreamPipe::ReadableListener::OnStreamAlloc(size_t suggested_size) { diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 29b7cf3f0d60ad..36877424216ed4 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -320,9 +320,10 @@ void TLSWrap::EncOut() { // its not clear if it is always correct. Not calling Done() could block // data flow, so for now continue to call Done(), just do it in the next // tick. - env()->SetImmediate([this](Environment* env) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { InvokeQueued(0); - }, object()); + }); } } return; @@ -353,9 +354,10 @@ void TLSWrap::EncOut() { HandleScope handle_scope(env()->isolate()); // Simulate asynchronous finishing, TLS cannot handle this at the moment. - env()->SetImmediate([this](Environment* env) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { OnStreamAfterWrite(nullptr, 0); - }, object()); + }); } } @@ -730,9 +732,10 @@ int TLSWrap::DoWrite(WriteWrap* w, StreamWriteResult res = underlying_stream()->Write(bufs, count, send_handle); if (!res.async) { - env()->SetImmediate([this](Environment* env) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { OnStreamAfterWrite(current_empty_write_, 0); - }, object()); + }); } return 0; }