Skip to content

Commit

Permalink
src: remove keep alive option from SetImmediate()
Browse files Browse the repository at this point in the history
This is no longer necessary now that the copyable `BaseObjectPtr`
is available (as opposed to the only-movable `v8::Global`).

Backport-PR-URL: #32301
PR-URL: #30374
Refs: nodejs/quic#141
Refs: nodejs/quic#149
Refs: nodejs/quic#141
Refs: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
  • Loading branch information
addaleax authored and MylesBorins committed Apr 1, 2020
1 parent 6db84d3 commit e17d314
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 43 deletions.
10 changes: 6 additions & 4 deletions src/cares_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -627,8 +627,6 @@ class QueryWrap : public AsyncWrap {
} else {
Parse(response_data_->host.get());
}

delete this;
}

void* MakeCallbackPointer() {
Expand Down Expand Up @@ -686,9 +684,13 @@ class QueryWrap : public AsyncWrap {
}

void QueueResponseCallback(int status) {
env()->SetImmediate([this](Environment*) {
BaseObjectPtr<QueryWrap> 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);
Expand Down
21 changes: 8 additions & 13 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -733,13 +733,9 @@ inline void IsolateData::set_options(
}

template <typename Fn>
void Environment::CreateImmediate(Fn&& cb,
v8::Local<v8::Object> keep_alive,
bool ref) {
void Environment::CreateImmediate(Fn&& cb, bool ref) {
auto callback = std::make_unique<NativeImmediateCallbackImpl<Fn>>(
std::move(cb),
v8::Global<v8::Object>(isolate(), keep_alive),
ref);
std::move(cb), ref);
NativeImmediateCallback* prev_tail = native_immediate_callbacks_tail_;

native_immediate_callbacks_tail_ = callback.get();
Expand All @@ -752,17 +748,17 @@ void Environment::CreateImmediate(Fn&& cb,
}

template <typename Fn>
void Environment::SetImmediate(Fn&& cb, v8::Local<v8::Object> 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);
immediate_info()->ref_count_inc(1);
}

template <typename Fn>
void Environment::SetUnrefImmediate(Fn&& cb, v8::Local<v8::Object> keep_alive) {
CreateImmediate(std::move(cb), keep_alive, false);
void Environment::SetUnrefImmediate(Fn&& cb) {
CreateImmediate(std::move(cb), false);
}

Environment::NativeImmediateCallback::NativeImmediateCallback(bool refed)
Expand All @@ -784,10 +780,9 @@ void Environment::NativeImmediateCallback::set_next(

template <typename Fn>
Environment::NativeImmediateCallbackImpl<Fn>::NativeImmediateCallbackImpl(
Fn&& callback, v8::Global<v8::Object>&& 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 <typename Fn>
void Environment::NativeImmediateCallbackImpl<Fn>::Call(Environment* env) {
Expand Down
17 changes: 4 additions & 13 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 <typename Fn>
inline void SetImmediate(Fn&& cb,
v8::Local<v8::Object> keep_alive =
v8::Local<v8::Object>());
inline void SetImmediate(Fn&& cb);
template <typename Fn>
inline void SetUnrefImmediate(Fn&& cb,
v8::Local<v8::Object> keep_alive =
v8::Local<v8::Object>());
inline void SetUnrefImmediate(Fn&& cb);
// This needs to be available for the JS-land setImmediate().
void ToggleImmediateRef(bool ref);

Expand Down Expand Up @@ -1274,9 +1270,7 @@ class Environment : public MemoryRetainer {

private:
template <typename Fn>
inline void CreateImmediate(Fn&& cb,
v8::Local<v8::Object> keep_alive,
bool ref);
inline void CreateImmediate(Fn&& cb, bool ref);

inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
const char* errmsg);
Expand Down Expand Up @@ -1424,14 +1418,11 @@ class Environment : public MemoryRetainer {
template <typename Fn>
class NativeImmediateCallbackImpl final : public NativeImmediateCallback {
public:
NativeImmediateCallbackImpl(Fn&& callback,
v8::Global<v8::Object>&& keep_alive,
bool refed);
NativeImmediateCallbackImpl(Fn&& callback, bool refed);
void Call(Environment* env) override;

private:
Fn callback_;
v8::Global<v8::Object> keep_alive_;
};

std::unique_ptr<NativeImmediateCallback> native_immediate_callbacks_head_;
Expand Down
16 changes: 10 additions & 6 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<Http2Session> 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,
Expand All @@ -1563,7 +1564,7 @@ void Http2Session::MaybeScheduleWrite() {
HandleScope handle_scope(env->isolate());
InternalCallbackScope callback_scope(this);
SendPendingData();
}, object());
});
}
}

Expand Down Expand Up @@ -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<Http2Stream> 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.
Expand All @@ -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 =
Expand Down
3 changes: 2 additions & 1 deletion src/stream_pipe.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<StreamPipe> strong_ref{this};
env()->SetImmediate([this](Environment* env) {
HandleScope handle_scope(env->isolate());
Context::Scope context_scope(env->context());
Expand Down Expand Up @@ -106,7 +107,7 @@ void StreamPipe::Unpipe() {
.IsNothing()) {
return;
}
}, object());
});
}

uv_buf_t StreamPipe::ReadableListener::OnStreamAlloc(size_t suggested_size) {
Expand Down
15 changes: 9 additions & 6 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<TLSWrap> strong_ref{this};
env()->SetImmediate([this, strong_ref](Environment* env) {
InvokeQueued(0);
}, object());
});
}
}
return;
Expand Down Expand Up @@ -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<TLSWrap> strong_ref{this};
env()->SetImmediate([this, strong_ref](Environment* env) {
OnStreamAfterWrite(nullptr, 0);
}, object());
});
}
}

Expand Down Expand Up @@ -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<TLSWrap> strong_ref{this};
env()->SetImmediate([this, strong_ref](Environment* env) {
OnStreamAfterWrite(current_empty_write_, 0);
}, object());
});
}
return 0;
}
Expand Down

0 comments on commit e17d314

Please sign in to comment.