From 157c618b745625063ea87c4cfcee306f5d169118 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 20 Mar 2018 02:52:49 +0100 Subject: [PATCH 1/2] src: simplify http2 perf tracking code Use `unique_ptr`s and use the resulting simplification to reduce indentation in these functions. --- src/node_http2.cc | 95 ++++++++++++++++++++++++----------------------- 1 file changed, 48 insertions(+), 47 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 523fda730d2a4a..52b0f8d66622a5 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -515,35 +515,35 @@ void Http2Stream::EmitStatistics() { Http2StreamPerformanceEntry* entry = new Http2StreamPerformanceEntry(env(), id_, statistics_); env()->SetImmediate([](Environment* env, void* data) { - Http2StreamPerformanceEntry* entry = - static_cast(data); - if (HasHttp2Observer(env)) { - AliasedBuffer& buffer = - env->http2_state()->stream_stats_buffer; - buffer[IDX_STREAM_STATS_ID] = entry->id(); - if (entry->first_byte() != 0) { - buffer[IDX_STREAM_STATS_TIMETOFIRSTBYTE] = - (entry->first_byte() - entry->startTimeNano()) / 1e6; - } else { - buffer[IDX_STREAM_STATS_TIMETOFIRSTBYTE] = 0; - } - if (entry->first_header() != 0) { - buffer[IDX_STREAM_STATS_TIMETOFIRSTHEADER] = - (entry->first_header() - entry->startTimeNano()) / 1e6; - } else { - buffer[IDX_STREAM_STATS_TIMETOFIRSTHEADER] = 0; - } - if (entry->first_byte_sent() != 0) { - buffer[IDX_STREAM_STATS_TIMETOFIRSTBYTESENT] = - (entry->first_byte_sent() - entry->startTimeNano()) / 1e6; - } else { - buffer[IDX_STREAM_STATS_TIMETOFIRSTBYTESENT] = 0; - } - buffer[IDX_STREAM_STATS_SENTBYTES] = entry->sent_bytes(); - buffer[IDX_STREAM_STATS_RECEIVEDBYTES] = entry->received_bytes(); - entry->Notify(entry->ToObject()); + // This takes ownership, the entr is destroyed at the end of this scope. + std::unique_ptr entry { + static_cast(data) }; + if (!HasHttp2Observer(env)) + return; + AliasedBuffer& buffer = + env->http2_state()->stream_stats_buffer; + buffer[IDX_STREAM_STATS_ID] = entry->id(); + if (entry->first_byte() != 0) { + buffer[IDX_STREAM_STATS_TIMETOFIRSTBYTE] = + (entry->first_byte() - entry->startTimeNano()) / 1e6; + } else { + buffer[IDX_STREAM_STATS_TIMETOFIRSTBYTE] = 0; } - delete entry; + if (entry->first_header() != 0) { + buffer[IDX_STREAM_STATS_TIMETOFIRSTHEADER] = + (entry->first_header() - entry->startTimeNano()) / 1e6; + } else { + buffer[IDX_STREAM_STATS_TIMETOFIRSTHEADER] = 0; + } + if (entry->first_byte_sent() != 0) { + buffer[IDX_STREAM_STATS_TIMETOFIRSTBYTESENT] = + (entry->first_byte_sent() - entry->startTimeNano()) / 1e6; + } else { + buffer[IDX_STREAM_STATS_TIMETOFIRSTBYTESENT] = 0; + } + buffer[IDX_STREAM_STATS_SENTBYTES] = entry->sent_bytes(); + buffer[IDX_STREAM_STATS_RECEIVEDBYTES] = entry->received_bytes(); + entry->Notify(entry->ToObject()); }, static_cast(entry)); } @@ -553,25 +553,25 @@ void Http2Session::EmitStatistics() { Http2SessionPerformanceEntry* entry = new Http2SessionPerformanceEntry(env(), statistics_, session_type_); env()->SetImmediate([](Environment* env, void* data) { - Http2SessionPerformanceEntry* entry = - static_cast(data); - if (HasHttp2Observer(env)) { - AliasedBuffer& buffer = - env->http2_state()->session_stats_buffer; - buffer[IDX_SESSION_STATS_TYPE] = entry->type(); - buffer[IDX_SESSION_STATS_PINGRTT] = entry->ping_rtt() / 1e6; - buffer[IDX_SESSION_STATS_FRAMESRECEIVED] = entry->frame_count(); - buffer[IDX_SESSION_STATS_FRAMESSENT] = entry->frame_sent(); - buffer[IDX_SESSION_STATS_STREAMCOUNT] = entry->stream_count(); - buffer[IDX_SESSION_STATS_STREAMAVERAGEDURATION] = - entry->stream_average_duration(); - buffer[IDX_SESSION_STATS_DATA_SENT] = entry->data_sent(); - buffer[IDX_SESSION_STATS_DATA_RECEIVED] = entry->data_received(); - buffer[IDX_SESSION_STATS_MAX_CONCURRENT_STREAMS] = - entry->max_concurrent_streams(); - entry->Notify(entry->ToObject()); - } - delete entry; + // This takes ownership, the entr is destroyed at the end of this scope. + std::unique_ptr entry { + static_cast(data) }; + if (!HasHttp2Observer(env)) + return; + AliasedBuffer& buffer = + env->http2_state()->session_stats_buffer; + buffer[IDX_SESSION_STATS_TYPE] = entry->type(); + buffer[IDX_SESSION_STATS_PINGRTT] = entry->ping_rtt() / 1e6; + buffer[IDX_SESSION_STATS_FRAMESRECEIVED] = entry->frame_count(); + buffer[IDX_SESSION_STATS_FRAMESSENT] = entry->frame_sent(); + buffer[IDX_SESSION_STATS_STREAMCOUNT] = entry->stream_count(); + buffer[IDX_SESSION_STATS_STREAMAVERAGEDURATION] = + entry->stream_average_duration(); + buffer[IDX_SESSION_STATS_DATA_SENT] = entry->data_sent(); + buffer[IDX_SESSION_STATS_DATA_RECEIVED] = entry->data_received(); + buffer[IDX_SESSION_STATS_MAX_CONCURRENT_STREAMS] = + entry->max_concurrent_streams(); + entry->Notify(entry->ToObject()); }, static_cast(entry)); } @@ -1379,6 +1379,7 @@ void Http2Session::MaybeScheduleWrite() { // Sending data may call arbitrary JS code, so keep track of // async context. + HandleScope handle_scope(env->isolate()); InternalCallbackScope callback_scope(session); session->SendPendingData(); }, static_cast(this), object()); From 5cb01dd7d73c792d760df4608c8e569e9530b691 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 20 Mar 2018 02:55:19 +0100 Subject: [PATCH 2/2] src: ensure that `SetImmediate()`s have `HandleScope`s Make sure that all native `SetImmediate()` functions have `HandleScope`s if they create handles. --- src/env.cc | 3 +++ src/node_file.cc | 1 + src/node_http2.cc | 2 ++ 3 files changed, 6 insertions(+) diff --git a/src/env.cc b/src/env.cc index c876103972fe92..16a24260f626d1 100644 --- a/src/env.cc +++ b/src/env.cc @@ -376,6 +376,9 @@ void Environment::RunAndClearNativeImmediates() { auto drain_list = [&]() { v8::TryCatch try_catch(isolate()); for (auto it = list.begin(); it != list.end(); ++it) { +#ifdef DEBUG + v8::SealHandleScope seal_handle_scope(isolate()); +#endif it->cb_(this, it->data_); if (it->refed_) ref_count++; diff --git a/src/node_file.cc b/src/node_file.cc index 510689607e87a7..7b493de131f6b2 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -178,6 +178,7 @@ inline void FileHandle::Close() { // it is being thrown from within the SetImmediate handler and // there is no JS stack to bubble it to. In other words, tearing // down the process is the only reasonable thing we can do here. + HandleScope handle_scope(env->isolate()); env->ThrowUVException(detail->ret, "close", msg); delete detail; }, detail); diff --git a/src/node_http2.cc b/src/node_http2.cc index 52b0f8d66622a5..63ef4df793c0c5 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -520,6 +520,7 @@ void Http2Stream::EmitStatistics() { static_cast(data) }; if (!HasHttp2Observer(env)) return; + HandleScope handle_scope(env->isolate()); AliasedBuffer& buffer = env->http2_state()->stream_stats_buffer; buffer[IDX_STREAM_STATS_ID] = entry->id(); @@ -558,6 +559,7 @@ void Http2Session::EmitStatistics() { static_cast(data) }; if (!HasHttp2Observer(env)) return; + HandleScope handle_scope(env->isolate()); AliasedBuffer& buffer = env->http2_state()->session_stats_buffer; buffer[IDX_SESSION_STATS_TYPE] = entry->type();