From 8695273948846b999f528ede97c764638fbb6c40 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 24 Feb 2018 17:42:27 +0100 Subject: [PATCH] src: tighten handle scopes for stream operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Put `HandleScope`s and `Context::Scope`s where they are used, and don’t create one for native stream callbacks automatically. This is slightly less convenient but means that stream listeners that don’t actually call back into JS don’t have to pay the (small) cost of setting these up. PR-URL: https://github.com/nodejs/node/pull/18936 Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- src/node_http2.cc | 7 +++++-- src/stream_base-inl.h | 42 ++++++++++++++++-------------------------- src/stream_base.cc | 2 ++ src/stream_base.h | 13 ++++--------- src/tls_wrap.cc | 11 +++++++++++ 5 files changed, 38 insertions(+), 37 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index d683b075bc2ed3..939e0011bdfa42 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1132,6 +1132,8 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { Http2Stream* stream = static_cast(stream_); Http2Session* session = stream->session(); Environment* env = stream->env(); + HandleScope handle_scope(env->isolate()); + Context::Scope context_scope(env->context()); if (nread < 0) { PassReadErrorToPreviousListener(nread); @@ -1422,6 +1424,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { void Http2Session::MaybeScheduleWrite() { CHECK_EQ(flags_ & SESSION_STATE_WRITE_SCHEDULED, 0); if (session_ != nullptr && nghttp2_session_want_write(session_)) { + HandleScope handle_scope(env()->isolate()); DEBUG_HTTP2SESSION(this, "scheduling write"); flags_ |= SESSION_STATE_WRITE_SCHEDULED; env()->SetImmediate([](Environment* env, void* data) { @@ -1632,6 +1635,8 @@ inline Http2Stream* Http2Session::SubmitRequest( // Callback used to receive inbound data from the i/o stream void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { + HandleScope handle_scope(env()->isolate()); + Context::Scope context_scope(env()->context()); Http2Scope h2scope(this); CHECK_NE(stream_, nullptr); DEBUG_HTTP2SESSION2(this, "receiving %d bytes", nread); @@ -1661,8 +1666,6 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { CHECK_LE(static_cast(nread), stream_buf_.len); Isolate* isolate = env()->isolate(); - HandleScope scope(isolate); - Context::Scope context_scope(env()->context()); // Create an array buffer for the read data. DATA frames will be emitted // as slices of this array buffer to avoid having to copy memory. diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 1534dcd1d53359..c87393e6fc1c72 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -106,22 +106,33 @@ inline void StreamResource::RemoveStreamListener(StreamListener* listener) { listener->previous_listener_ = nullptr; } - inline uv_buf_t StreamResource::EmitAlloc(size_t suggested_size) { +#ifdef DEBUG + v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent()); +#endif return listener_->OnStreamAlloc(suggested_size); } inline void StreamResource::EmitRead(ssize_t nread, const uv_buf_t& buf) { +#ifdef DEBUG + v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent()); +#endif if (nread > 0) bytes_read_ += static_cast(nread); listener_->OnStreamRead(nread, buf); } inline void StreamResource::EmitAfterWrite(WriteWrap* w, int status) { +#ifdef DEBUG + v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent()); +#endif listener_->OnStreamAfterWrite(w, status); } inline void StreamResource::EmitAfterShutdown(ShutdownWrap* w, int status) { +#ifdef DEBUG + v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent()); +#endif listener_->OnStreamAfterShutdown(w, status); } @@ -133,29 +144,6 @@ inline Environment* StreamBase::stream_env() const { return env_; } -inline void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) { - AfterRequest(req_wrap, [&]() { - EmitAfterWrite(req_wrap, status); - }); -} - -inline void StreamBase::AfterShutdown(ShutdownWrap* req_wrap, int status) { - AfterRequest(req_wrap, [&]() { - EmitAfterShutdown(req_wrap, status); - }); -} - -template -inline void StreamBase::AfterRequest(Wrap* req_wrap, EmitEvent emit) { - Environment* env = stream_env(); - - v8::HandleScope handle_scope(env->isolate()); - v8::Context::Scope context_scope(env->context()); - - emit(); - req_wrap->Dispose(); -} - inline int StreamBase::Shutdown(v8::Local req_wrap_obj) { Environment* env = stream_env(); if (req_wrap_obj.IsEmpty()) { @@ -387,7 +375,8 @@ void StreamBase::JSMethod(const FunctionCallbackInfo& args) { inline void ShutdownWrap::OnDone(int status) { - stream()->AfterShutdown(this, status); + stream()->EmitAfterShutdown(this, status); + Dispose(); } inline void WriteWrap::SetAllocatedStorage(char* data, size_t size) { @@ -405,7 +394,8 @@ inline size_t WriteWrap::StorageSize() const { } inline void WriteWrap::OnDone(int status) { - stream()->AfterWrite(this, status); + stream()->EmitAfterWrite(this, status); + Dispose(); } inline void StreamReq::Done(int status, const char* error_str) { diff --git a/src/stream_base.cc b/src/stream_base.cc index 1d1d324841537f..8838a1a6dfb6b3 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -387,6 +387,8 @@ void ReportWritesToJSStreamListener::OnStreamAfterReqFinished( StreamBase* stream = static_cast(stream_); Environment* env = stream->stream_env(); AsyncWrap* async_wrap = req_wrap->GetAsyncWrap(); + HandleScope handle_scope(env->isolate()); + Context::Scope context_scope(env->context()); Local req_wrap_obj = async_wrap->object(); Local argv[] = { diff --git a/src/stream_base.h b/src/stream_base.h index 8af05059f49e47..6962648650e1a6 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -61,7 +61,8 @@ class ShutdownWrap : public StreamReq { v8::Local req_wrap_obj) : StreamReq(stream, req_wrap_obj) { } - void OnDone(int status) override; // Just calls stream()->AfterShutdown() + // Call stream()->EmitAfterShutdown() and dispose of this request wrap. + void OnDone(int status) override; }; class WriteWrap : public StreamReq { @@ -78,7 +79,8 @@ class WriteWrap : public StreamReq { free(storage_); } - void OnDone(int status) override; // Just calls stream()->AfterWrite() + // Call stream()->EmitAfterWrite() and dispose of this request wrap. + void OnDone(int status) override; private: char* storage_ = nullptr; @@ -306,13 +308,6 @@ class StreamBase : public StreamResource { Environment* env_; EmitToJSStreamListener default_listener_; - // These are called by the respective {Write,Shutdown}Wrap class. - void AfterShutdown(ShutdownWrap* req, int status); - void AfterWrite(WriteWrap* req, int status); - - template - void AfterRequest(Wrap* req_wrap, EmitEvent emit); - friend class WriteWrap; friend class ShutdownWrap; }; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 1cc5478bb57296..cddef66c44a8e5 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -220,6 +220,8 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) { SSL* ssl = const_cast(ssl_); TLSWrap* c = static_cast(SSL_get_app_data(ssl)); Environment* env = c->env(); + HandleScope handle_scope(env->isolate()); + Context::Scope context_scope(env->context()); Local object = c->object(); if (where & SSL_CB_HANDSHAKE_START) { @@ -289,6 +291,8 @@ void TLSWrap::EncOut() { NODE_COUNT_NET_BYTES_SENT(write_size_); if (!res.async) { + HandleScope handle_scope(env()->isolate()); + // Simulate asynchronous finishing, TLS cannot handle this at the moment. env()->SetImmediate([](Environment* env, void* data) { static_cast(data)->OnStreamAfterWrite(nullptr, 0); @@ -427,6 +431,7 @@ void TLSWrap::ClearOut() { // shutdown cleanly (SSL_ERROR_ZERO_RETURN) even when read == 0. // See node#1642 and SSL_read(3SSL) for details. if (read <= 0) { + HandleScope handle_scope(env()->isolate()); int err; Local arg = GetSSLError(read, &err, nullptr); @@ -477,6 +482,9 @@ bool TLSWrap::ClearIn() { } // Error or partial write + HandleScope handle_scope(env()->isolate()); + Context::Scope context_scope(env()->context()); + int err; std::string error_str; Local arg = GetSSLError(written, &err, &error_str); @@ -814,6 +822,9 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) { if (servername == nullptr) return SSL_TLSEXT_ERR_OK; + HandleScope handle_scope(env->isolate()); + Context::Scope context_scope(env->context()); + // Call the SNI callback and use its return value as context Local object = p->object(); Local ctx = object->Get(env->sni_context_string());