FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: tighten handle scopes for stream operations · nodejs/node@8695273 · GitHub

/ node Public

Commit 8695273

Browse files
addaleax
committed
src: tighten handle scopes for stream operations
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: #18936 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 3ad7c1a commit 8695273

5 files changed

Lines changed: 38 additions & 37 deletions

File tree

‎src/node_http2.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,6 +1132,8 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
11321132
Http2Stream* stream = static_cast<Http2Stream*>(stream_);
11331133
Http2Session* session = stream->session();
11341134
Environment* env = stream->env();
1135+
HandleScope handle_scope(env->isolate());
1136+
Context::Scope context_scope(env->context());
11351137

11361138
if (nread < 0) {
11371139
PassReadErrorToPreviousListener(nread);
@@ -1422,6 +1424,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
14221424
void Http2Session::MaybeScheduleWrite() {
14231425
CHECK_EQ(flags_ & SESSION_STATE_WRITE_SCHEDULED, 0);
14241426
if (session_ != nullptr && nghttp2_session_want_write(session_)) {
1427+
HandleScope handle_scope(env()->isolate());
14251428
DEBUG_HTTP2SESSION(this, "scheduling write");
14261429
flags_ |= SESSION_STATE_WRITE_SCHEDULED;
14271430
env()->SetImmediate([](Environment* env, void* data) {
@@ -1632,6 +1635,8 @@ inline Http2Stream* Http2Session::SubmitRequest(
16321635

16331636
// Callback used to receive inbound data from the i/o stream
16341637
void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
1638+
HandleScope handle_scope(env()->isolate());
1639+
Context::Scope context_scope(env()->context());
16351640
Http2Scope h2scope(this);
16361641
CHECK_NE(stream_, nullptr);
16371642
DEBUG_HTTP2SESSION2(this, "receiving %d bytes", nread);
@@ -1661,8 +1666,6 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
16611666
CHECK_LE(static_cast<size_t>(nread), stream_buf_.len);
16621667

16631668
Isolate* isolate = env()->isolate();
1664-
HandleScope scope(isolate);
1665-
Context::Scope context_scope(env()->context());
16661669

16671670
// Create an array buffer for the read data. DATA frames will be emitted
16681671
// as slices of this array buffer to avoid having to copy memory.

‎src/stream_base-inl.h‎

Lines changed: 16 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -106,22 +106,33 @@ inline void StreamResource::RemoveStreamListener(StreamListener* listener) {
106106
listener->previous_listener_ = nullptr;
107107
}
108108

109-
110109
inline uv_buf_t StreamResource::EmitAlloc(size_t suggested_size) {
110+
#ifdef DEBUG
111+
v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent());
112+
#endif
111113
return listener_->OnStreamAlloc(suggested_size);
112114
}
113115

114116
inline void StreamResource::EmitRead(ssize_t nread, const uv_buf_t& buf) {
117+
#ifdef DEBUG
118+
v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent());
119+
#endif
115120
if (nread > 0)
116121
bytes_read_ += static_cast<uint64_t>(nread);
117122
listener_->OnStreamRead(nread, buf);
118123
}
119124

120125
inline void StreamResource::EmitAfterWrite(WriteWrap* w, int status) {
126+
#ifdef DEBUG
127+
v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent());
128+
#endif
121129
listener_->OnStreamAfterWrite(w, status);
122130
}
123131

124132
inline void StreamResource::EmitAfterShutdown(ShutdownWrap* w, int status) {
133+
#ifdef DEBUG
134+
v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent());
135+
#endif
125136
listener_->OnStreamAfterShutdown(w, status);
126137
}
127138

@@ -133,29 +144,6 @@ inline Environment* StreamBase::stream_env() const {
133144
return env_;
134145
}
135146

136-
inline void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) {
137-
AfterRequest(req_wrap, [&]() {
138-
EmitAfterWrite(req_wrap, status);
139-
});
140-
}
141-
142-
inline void StreamBase::AfterShutdown(ShutdownWrap* req_wrap, int status) {
143-
AfterRequest(req_wrap, [&]() {
144-
EmitAfterShutdown(req_wrap, status);
145-
});
146-
}
147-
148-
template<typename Wrap, typename EmitEvent>
149-
inline void StreamBase::AfterRequest(Wrap* req_wrap, EmitEvent emit) {
150-
Environment* env = stream_env();
151-
152-
v8::HandleScope handle_scope(env->isolate());
153-
v8::Context::Scope context_scope(env->context());
154-
155-
emit();
156-
req_wrap->Dispose();
157-
}
158-
159147
inline int StreamBase::Shutdown(v8::Local<v8::Object> req_wrap_obj) {
160148
Environment* env = stream_env();
161149
if (req_wrap_obj.IsEmpty()) {
@@ -387,7 +375,8 @@ void StreamBase::JSMethod(const FunctionCallbackInfo<Value>& args) {
387375

388376

389377
inline void ShutdownWrap::OnDone(int status) {
390-
stream()->AfterShutdown(this, status);
378+
stream()->EmitAfterShutdown(this, status);
379+
Dispose();
391380
}
392381

393382
inline void WriteWrap::SetAllocatedStorage(char* data, size_t size) {
@@ -405,7 +394,8 @@ inline size_t WriteWrap::StorageSize() const {
405394
}
406395

407396
inline void WriteWrap::OnDone(int status) {
408-
stream()->AfterWrite(this, status);
397+
stream()->EmitAfterWrite(this, status);
398+
Dispose();
409399
}
410400

411401
inline void StreamReq::Done(int status, const char* error_str) {

‎src/stream_base.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,8 @@ void ReportWritesToJSStreamListener::OnStreamAfterReqFinished(
387387
StreamBase* stream = static_cast<StreamBase*>(stream_);
388388
Environment* env = stream->stream_env();
389389
AsyncWrap* async_wrap = req_wrap->GetAsyncWrap();
390+
HandleScope handle_scope(env->isolate());
391+
Context::Scope context_scope(env->context());
390392
Local<Object> req_wrap_obj = async_wrap->object();
391393

392394
Local<Value> argv[] = {

‎src/stream_base.h‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,8 @@ class ShutdownWrap : public StreamReq {
6161
v8::Local<v8::Object> req_wrap_obj)
6262
: StreamReq(stream, req_wrap_obj) { }
6363

64-
void OnDone(int status) override; // Just calls stream()->AfterShutdown()
64+
// Call stream()->EmitAfterShutdown() and dispose of this request wrap.
65+
void OnDone(int status) override;
6566
};
6667

6768
class WriteWrap : public StreamReq {
@@ -78,7 +79,8 @@ class WriteWrap : public StreamReq {
7879
free(storage_);
7980
}
8081

81-
void OnDone(int status) override; // Just calls stream()->AfterWrite()
82+
// Call stream()->EmitAfterWrite() and dispose of this request wrap.
83+
void OnDone(int status) override;
8284

8385
private:
8486
char* storage_ = nullptr;
@@ -306,13 +308,6 @@ class StreamBase : public StreamResource {
306308
Environment* env_;
307309
EmitToJSStreamListener default_listener_;
308310

309-
// These are called by the respective {Write,Shutdown}Wrap class.
310-
void AfterShutdown(ShutdownWrap* req, int status);
311-
void AfterWrite(WriteWrap* req, int status);
312-
313-
template <typename Wrap, typename EmitEvent>
314-
void AfterRequest(Wrap* req_wrap, EmitEvent emit);
315-
316311
friend class WriteWrap;
317312
friend class ShutdownWrap;
318313
};

‎src/tls_wrap.cc‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,8 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) {
220220
SSL* ssl = const_cast<SSL*>(ssl_);
221221
TLSWrap* c = static_cast<TLSWrap*>(SSL_get_app_data(ssl));
222222
Environment* env = c->env();
223+
HandleScope handle_scope(env->isolate());
224+
Context::Scope context_scope(env->context());
223225
Local<Object> object = c->object();
224226

225227
if (where & SSL_CB_HANDSHAKE_START) {
@@ -289,6 +291,8 @@ void TLSWrap::EncOut() {
289291
NODE_COUNT_NET_BYTES_SENT(write_size_);
290292

291293
if (!res.async) {
294+
HandleScope handle_scope(env()->isolate());
295+
292296
// Simulate asynchronous finishing, TLS cannot handle this at the moment.
293297
env()->SetImmediate([](Environment* env, void* data) {
294298
static_cast<TLSWrap*>(data)->OnStreamAfterWrite(nullptr, 0);
@@ -427,6 +431,7 @@ void TLSWrap::ClearOut() {
427431
// shutdown cleanly (SSL_ERROR_ZERO_RETURN) even when read == 0.
428432
// See node#1642 and SSL_read(3SSL) for details.
429433
if (read <= 0) {
434+
HandleScope handle_scope(env()->isolate());
430435
int err;
431436
Local<Value> arg = GetSSLError(read, &err, nullptr);
432437

@@ -477,6 +482,9 @@ bool TLSWrap::ClearIn() {
477482
}
478483

479484
// Error or partial write
485+
HandleScope handle_scope(env()->isolate());
486+
Context::Scope context_scope(env()->context());
487+
480488
int err;
481489
std::string error_str;
482490
Local<Value> arg = GetSSLError(written, &err, &error_str);
@@ -814,6 +822,9 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) {
814822
if (servername == nullptr)
815823
return SSL_TLSEXT_ERR_OK;
816824

825+
HandleScope handle_scope(env->isolate());
826+
Context::Scope context_scope(env->context());
827+
817828
// Call the SNI callback and use its return value as context
818829
Local<Object> object = p->object();
819830
Local<Value> ctx = object->Get(env->sni_context_string());

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL