From c1234673bbba1ac6c8425dffb2604ccf647bbfcf Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sat, 13 Jan 2018 16:51:28 -0500 Subject: [PATCH 01/24] timers: allow Immediates to be unrefed Refactor Immediates handling to allow for them to be unrefed, similar to setTimeout, but without extra handles. Document the new `immediate.ref()` and `immediate.unref()` methods. Add SetImmediateUnref on the C++ side. PR-URL: https://github.com/nodejs/node/pull/18139 Reviewed-By: Anna Henningsen Reviewed-By: Franziska Hinkelmann Reviewed-By: James M Snell --- doc/api/timers.md | 32 ++++ lib/timers.js | 143 ++++++++++-------- src/env-inl.h | 40 ++++- src/env.cc | 36 ++--- src/env.h | 17 ++- src/node_perf.cc | 18 +-- src/timer_wrap.cc | 12 +- test/addons-napi/test_uv_loop/test_uv_loop.cc | 9 ++ .../test-timers-immediate-unref-simple.js | 7 + test/parallel/test-timers-immediate-unref.js | 37 +++++ 10 files changed, 249 insertions(+), 102 deletions(-) create mode 100644 test/parallel/test-timers-immediate-unref-simple.js create mode 100644 test/parallel/test-timers-immediate-unref.js diff --git a/doc/api/timers.md b/doc/api/timers.md index 09502dee100..13f2dea37d9 100644 --- a/doc/api/timers.md +++ b/doc/api/timers.md @@ -18,6 +18,38 @@ This object is created internally and is returned from [`setImmediate()`][]. It can be passed to [`clearImmediate()`][] in order to cancel the scheduled actions. +By default, when an immediate is scheduled, the Node.js event loop will continue +running as long as the immediate is active. The `Immediate` object returned by +[`setImmediate()`][] exports both `immediate.ref()` and `immediate.unref()` +functions that can be used to control this default behavior. + +### immediate.ref() + + +When called, requests that the Node.js event loop *not* exit so long as the +`Immediate` is active. Calling `immediate.ref()` multiple times will have no +effect. + +*Note*: By default, all `Immediate` objects are "ref'd", making it normally +unnecessary to call `immediate.ref()` unless `immediate.unref()` had been called +previously. + +Returns a reference to the `Immediate`. + +### immediate.unref() + + +When called, the active `Immediate` object will not require the Node.js event +loop to remain active. If there is no other activity keeping the event loop +running, the process may exit before the `Immediate` object's callback is +invoked. Calling `immediate.unref()` multiple times will have no effect. + +Returns a reference to the `Immediate`. + ## Class: Timeout This object is created internally and is returned from [`setTimeout()`][] and diff --git a/lib/timers.js b/lib/timers.js index 8a74bcf2de7..6fc552fac7d 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -53,11 +53,14 @@ const trigger_async_id_symbol = timerInternals.trigger_async_id_symbol; // *Must* match Environment::ImmediateInfo::Fields in src/env.h. const kCount = 0; -const kHasOutstanding = 1; +const kRefCount = 1; +const kHasOutstanding = 2; -const [activateImmediateCheck, immediateInfo] = +const [immediateInfo, toggleImmediateRef] = setImmediateCallback(processImmediate); +const kRefed = Symbol('refed'); + // The Timeout class const Timeout = timerInternals.Timeout; @@ -656,42 +659,41 @@ function processImmediate() { const queue = outstandingQueue.head !== null ? outstandingQueue : immediateQueue; var immediate = queue.head; - var tail = queue.tail; + const tail = queue.tail; // Clear the linked list early in case new `setImmediate()` calls occur while // immediate callbacks are executed queue.head = queue.tail = null; - while (immediate !== null) { - if (!immediate._onImmediate) { - immediate = immediate._idleNext; - continue; - } + let count = 0; + let refCount = 0; - // Save next in case `clearImmediate(immediate)` is called from callback - const next = immediate._idleNext; + while (immediate !== null) { + immediate._destroyed = true; const asyncId = immediate[async_id_symbol]; emitBefore(asyncId, immediate[trigger_async_id_symbol]); - tryOnImmediate(immediate, next, tail); + count++; + if (immediate[kRefed]) + refCount++; + immediate[kRefed] = undefined; + + tryOnImmediate(immediate, tail, count, refCount); emitAfter(asyncId); - // If `clearImmediate(immediate)` wasn't called from the callback, use the - // `immediate`'s next item - if (immediate._idleNext !== null) - immediate = immediate._idleNext; - else - immediate = next; + immediate = immediate._idleNext; } + immediateInfo[kCount] -= count; + immediateInfo[kRefCount] -= refCount; immediateInfo[kHasOutstanding] = 0; } // An optimization so that the try/finally only de-optimizes (since at least v8 // 4.7) what is in this smaller function. -function tryOnImmediate(immediate, next, oldTail) { +function tryOnImmediate(immediate, oldTail, count, refCount) { var threw = true; try { // make the actual call outside the try/finally to allow it to be optimized @@ -700,21 +702,21 @@ function tryOnImmediate(immediate, next, oldTail) { } finally { immediate._onImmediate = null; - if (!immediate._destroyed) { - immediate._destroyed = true; - immediateInfo[kCount]--; - - if (async_hook_fields[kDestroy] > 0) { - emitDestroy(immediate[async_id_symbol]); - } + if (async_hook_fields[kDestroy] > 0) { + emitDestroy(immediate[async_id_symbol]); } - if (threw && (immediate._idleNext !== null || next !== null)) { - // Handle any remaining Immediates after error handling has resolved, - // assuming we're still alive to do so. - outstandingQueue.head = immediate._idleNext || next; - outstandingQueue.tail = oldTail; - immediateInfo[kHasOutstanding] = 1; + if (threw) { + immediateInfo[kCount] -= count; + immediateInfo[kRefCount] -= refCount; + + if (immediate._idleNext !== null) { + // Handle any remaining Immediates after error handling has resolved, + // assuming we're still alive to do so. + outstandingQueue.head = immediate._idleNext; + outstandingQueue.tail = oldTail; + immediateInfo[kHasOutstanding] = 1; + } } } } @@ -729,31 +731,51 @@ function runCallback(timer) { } -function Immediate(callback, args) { - this._idleNext = null; - this._idlePrev = null; - // this must be set to null first to avoid function tracking - // on the hidden class, revisit in V8 versions after 6.2 - this._onImmediate = null; - this._onImmediate = callback; - this._argv = args; - this._destroyed = false; +const Immediate = class Immediate { + constructor(callback, args) { + this._idleNext = null; + this._idlePrev = null; + // this must be set to null first to avoid function tracking + // on the hidden class, revisit in V8 versions after 6.2 + this._onImmediate = null; + this._onImmediate = callback; + this._argv = args; + this._destroyed = false; + this[kRefed] = false; + + this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter]; + this[trigger_async_id_symbol] = getDefaultTriggerAsyncId(); + if (async_hook_fields[kInit] > 0) { + emitInit(this[async_id_symbol], + 'Immediate', + this[trigger_async_id_symbol], + this); + } + + this.ref(); + immediateInfo[kCount]++; - this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter]; - this[trigger_async_id_symbol] = getDefaultTriggerAsyncId(); - if (async_hook_fields[kInit] > 0) { - emitInit(this[async_id_symbol], - 'Immediate', - this[trigger_async_id_symbol], - this); + immediateQueue.append(this); } - if (immediateInfo[kCount] === 0) - activateImmediateCheck(); - immediateInfo[kCount]++; + ref() { + if (this[kRefed] === false) { + this[kRefed] = true; + if (immediateInfo[kRefCount]++ === 0) + toggleImmediateRef(true); + } + return this; + } - immediateQueue.append(this); -} + unref() { + if (this[kRefed] === true) { + this[kRefed] = false; + if (--immediateInfo[kRefCount] === 0) + toggleImmediateRef(false); + } + return this; + } +}; function setImmediate(callback, arg1, arg2, arg3) { if (typeof callback !== 'function') { @@ -793,15 +815,18 @@ exports.setImmediate = setImmediate; exports.clearImmediate = function(immediate) { - if (!immediate) return; + if (!immediate || immediate._destroyed) + return; - if (!immediate._destroyed) { - immediateInfo[kCount]--; - immediate._destroyed = true; + immediateInfo[kCount]--; + immediate._destroyed = true; - if (async_hook_fields[kDestroy] > 0) { - emitDestroy(immediate[async_id_symbol]); - } + if (immediate[kRefed] && --immediateInfo[kRefCount] === 0) + toggleImmediateRef(false); + immediate[kRefed] = undefined; + + if (async_hook_fields[kDestroy] > 0) { + emitDestroy(immediate[async_id_symbol]); } immediate._onImmediate = null; diff --git a/src/env-inl.h b/src/env-inl.h index 78a02909c46..4fec606219a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -229,6 +229,10 @@ inline uint32_t Environment::ImmediateInfo::count() const { return fields_[kCount]; } +inline uint32_t Environment::ImmediateInfo::ref_count() const { + return fields_[kRefCount]; +} + inline bool Environment::ImmediateInfo::has_outstanding() const { return fields_[kHasOutstanding] == 1; } @@ -241,6 +245,14 @@ inline void Environment::ImmediateInfo::count_dec(uint32_t decrement) { fields_[kCount] = fields_[kCount] - decrement; } +inline void Environment::ImmediateInfo::ref_count_inc(uint32_t increment) { + fields_[kRefCount] = fields_[kRefCount] + increment; +} + +inline void Environment::ImmediateInfo::ref_count_dec(uint32_t decrement) { + fields_[kRefCount] = fields_[kRefCount] - decrement; +} + inline Environment::TickInfo::TickInfo(v8::Isolate* isolate) : fields_(isolate, kFieldsCount) {} @@ -536,20 +548,36 @@ inline void Environment::set_fs_stats_field_array(double* fields) { fs_stats_field_array_ = fields; } -void Environment::SetImmediate(native_immediate_callback cb, +void Environment::CreateImmediate(native_immediate_callback cb, void* data, - v8::Local obj) { + v8::Local obj, + bool ref) { native_immediate_callbacks_.push_back({ cb, data, - std::unique_ptr>( - obj.IsEmpty() ? nullptr : new v8::Persistent(isolate_, obj)) + std::unique_ptr>(obj.IsEmpty() ? + nullptr : new v8::Persistent(isolate_, obj)), + ref }); - if (immediate_info()->count() == 0) - ActivateImmediateCheck(); immediate_info()->count_inc(1); } +void Environment::SetImmediate(native_immediate_callback cb, + void* data, + v8::Local obj) { + CreateImmediate(cb, data, obj, true); + + if (immediate_info()->ref_count() == 0) + ToggleImmediateRef(true); + immediate_info()->ref_count_inc(1); +} + +void Environment::SetUnrefImmediate(native_immediate_callback cb, + void* data, + v8::Local obj) { + CreateImmediate(cb, data, obj, false); +} + inline performance::performance_state* Environment::performance_state() { return performance_state_; } diff --git a/src/env.cc b/src/env.cc index fb23a8c1c93..706af774541 100644 --- a/src/env.cc +++ b/src/env.cc @@ -82,6 +82,8 @@ void Environment::Start(int argc, uv_idle_init(event_loop(), immediate_idle_handle()); + uv_check_start(immediate_check_handle(), CheckImmediate); + // Inform V8's CPU profiler when we're idle. The profiler is sampling-based // but not all samples are created equal; mark the wall clock time spent in // epoll_wait() and friends so profiling tools can filter it out. The samples @@ -274,39 +276,35 @@ void Environment::EnvPromiseHook(v8::PromiseHookType type, void Environment::RunAndClearNativeImmediates() { size_t count = native_immediate_callbacks_.size(); if (count > 0) { + size_t ref_count = 0; std::vector list; native_immediate_callbacks_.swap(list); for (const auto& cb : list) { cb.cb_(this, cb.data_); if (cb.keep_alive_) cb.keep_alive_->Reset(); + if (cb.refed_) + ref_count++; } #ifdef DEBUG CHECK_GE(immediate_info()->count(), count); #endif immediate_info()->count_dec(count); + immediate_info()->ref_count_dec(ref_count); } } -static bool MaybeStopImmediate(Environment* env) { - if (env->immediate_info()->count() == 0) { - uv_check_stop(env->immediate_check_handle()); - uv_idle_stop(env->immediate_idle_handle()); - return true; - } - return false; -} - void Environment::CheckImmediate(uv_check_t* handle) { Environment* env = Environment::from_immediate_check_handle(handle); - HandleScope scope(env->isolate()); - Context::Scope context_scope(env->context()); - if (MaybeStopImmediate(env)) + if (env->immediate_info()->count() == 0) return; + HandleScope scope(env->isolate()); + Context::Scope context_scope(env->context()); + env->RunAndClearNativeImmediates(); do { @@ -318,13 +316,17 @@ void Environment::CheckImmediate(uv_check_t* handle) { {0, 0}).ToLocalChecked(); } while (env->immediate_info()->has_outstanding()); - MaybeStopImmediate(env); + if (env->immediate_info()->ref_count() == 0) + env->ToggleImmediateRef(false); } -void Environment::ActivateImmediateCheck() { - uv_check_start(&immediate_check_handle_, CheckImmediate); - // Idle handle is needed only to stop the event loop from blocking in poll. - uv_idle_start(&immediate_idle_handle_, [](uv_idle_t*){ }); +void Environment::ToggleImmediateRef(bool ref) { + if (ref) { + // Idle handle is needed only to stop the event loop from blocking in poll. + uv_idle_start(immediate_idle_handle(), [](uv_idle_t*){ }); + } else { + uv_idle_stop(immediate_idle_handle()); + } } diff --git a/src/env.h b/src/env.h index e7ec682c8ce..a9f2af0a947 100644 --- a/src/env.h +++ b/src/env.h @@ -453,17 +453,22 @@ class Environment { public: inline AliasedBuffer& fields(); inline uint32_t count() const; + inline uint32_t ref_count() const; inline bool has_outstanding() const; inline void count_inc(uint32_t increment); inline void count_dec(uint32_t decrement); + inline void ref_count_inc(uint32_t increment); + inline void ref_count_dec(uint32_t decrement); + private: friend class Environment; // So we can call the constructor. inline explicit ImmediateInfo(v8::Isolate* isolate); enum Fields { kCount, + kRefCount, kHasOutstanding, kFieldsCount }; @@ -694,8 +699,12 @@ class Environment { inline void SetImmediate(native_immediate_callback cb, void* data, v8::Local obj = v8::Local()); + inline void SetUnrefImmediate(native_immediate_callback cb, + void* data, + v8::Local obj = + v8::Local()); // This needs to be available for the JS-land setImmediate(). - void ActivateImmediateCheck(); + void ToggleImmediateRef(bool ref); class ShouldNotAbortOnUncaughtScope { public: @@ -712,6 +721,11 @@ class Environment { static inline Environment* ForAsyncHooks(AsyncHooks* hooks); private: + inline void CreateImmediate(native_immediate_callback cb, + void* data, + v8::Local obj, + bool ref); + inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -776,6 +790,7 @@ class Environment { native_immediate_callback cb_; void* data_; std::unique_ptr> keep_alive_; + bool refed_; }; std::vector native_immediate_callbacks_; void RunAndClearNativeImmediates(); diff --git a/src/node_perf.cc b/src/node_perf.cc index 97d3a2d9952..34e6a35000e 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -182,9 +182,8 @@ void SetupPerformanceObservers(const FunctionCallbackInfo& args) { } // Creates a GC Performance Entry and passes it to observers -void PerformanceGCCallback(uv_async_t* handle) { - GCPerformanceEntry* entry = static_cast(handle->data); - Environment* env = entry->env(); +void PerformanceGCCallback(Environment* env, void* ptr) { + GCPerformanceEntry* entry = static_cast(ptr); HandleScope scope(env->isolate()); Local context = env->context(); @@ -201,10 +200,6 @@ void PerformanceGCCallback(uv_async_t* handle) { } delete entry; - auto closeCB = [](uv_handle_t* handle) { - delete reinterpret_cast(handle); - }; - uv_close(reinterpret_cast(handle), closeCB); } // Marks the start of a GC cycle @@ -221,16 +216,13 @@ void MarkGarbageCollectionEnd(Isolate* isolate, v8::GCCallbackFlags flags, void* data) { Environment* env = static_cast(data); - uv_async_t* async = new uv_async_t(); - if (uv_async_init(env->event_loop(), async, PerformanceGCCallback)) - return delete async; - uv_unref(reinterpret_cast(async)); - async->data = + GCPerformanceEntry* entry = new GCPerformanceEntry(env, static_cast(type), performance_last_gc_start_mark_, PERFORMANCE_NOW()); - CHECK_EQ(0, uv_async_send(async)); + env->SetUnrefImmediate(PerformanceGCCallback, + entry); } diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index ab450fcb3e7..1725cf30e71 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -83,16 +83,16 @@ class TimerWrap : public HandleWrap { CHECK(args[0]->IsFunction()); auto env = Environment::GetCurrent(args); env->set_immediate_callback_function(args[0].As()); - auto activate_cb = [] (const FunctionCallbackInfo& args) { - Environment::GetCurrent(args)->ActivateImmediateCheck(); + auto toggle_ref_cb = [] (const FunctionCallbackInfo& args) { + Environment::GetCurrent(args)->ToggleImmediateRef(args[0]->IsTrue()); }; - auto activate_function = - env->NewFunctionTemplate(activate_cb)->GetFunction(env->context()) + auto toggle_ref_function = + env->NewFunctionTemplate(toggle_ref_cb)->GetFunction(env->context()) .ToLocalChecked(); auto result = Array::New(env->isolate(), 2); - result->Set(env->context(), 0, activate_function).FromJust(); - result->Set(env->context(), 1, + result->Set(env->context(), 0, env->immediate_info()->fields().GetJSArray()).FromJust(); + result->Set(env->context(), 1, toggle_ref_function).FromJust(); args.GetReturnValue().Set(result); } diff --git a/test/addons-napi/test_uv_loop/test_uv_loop.cc b/test/addons-napi/test_uv_loop/test_uv_loop.cc index 44819f72bb6..048e25af9dd 100644 --- a/test/addons-napi/test_uv_loop/test_uv_loop.cc +++ b/test/addons-napi/test_uv_loop/test_uv_loop.cc @@ -24,6 +24,15 @@ void* SetImmediate(napi_env env, T&& cb) { assert(cb() != nullptr); }); + // Idle handle is needed only to stop the event loop from blocking in poll. + uv_idle_t* idle = new uv_idle_t; + uv_idle_init(loop, idle); + uv_idle_start(idle, [](uv_idle_t* idle) { + uv_close(reinterpret_cast(idle), [](uv_handle_t* handle) { + delete reinterpret_cast(handle); + }); + }); + return nullptr; } diff --git a/test/parallel/test-timers-immediate-unref-simple.js b/test/parallel/test-timers-immediate-unref-simple.js new file mode 100644 index 00000000000..68497460328 --- /dev/null +++ b/test/parallel/test-timers-immediate-unref-simple.js @@ -0,0 +1,7 @@ +'use strict'; + +const common = require('../common'); + +// This immediate should not execute as it was unrefed +// and nothing else is keeping the event loop alive +setImmediate(common.mustNotCall()).unref(); diff --git a/test/parallel/test-timers-immediate-unref.js b/test/parallel/test-timers-immediate-unref.js new file mode 100644 index 00000000000..5b56eb7e1d8 --- /dev/null +++ b/test/parallel/test-timers-immediate-unref.js @@ -0,0 +1,37 @@ +'use strict'; + +const common = require('../common'); +const Countdown = require('../common/countdown'); + +// This immediate should execute as it was unrefed and refed again. +// It also confirms that unref/ref are chainable. +setImmediate(common.mustCall(firstStep)).ref().unref().unref().ref(); + +function firstStep() { + const countdown = + new Countdown(2, common.mustCall(() => setImmediate(secondStep))); + // Unrefed setImmediate executes if it was unrefed but something else keeps + // the loop open + setImmediate(() => countdown.dec()).unref(); + setTimeout(() => countdown.dec(), 50); +} + +function secondStep() { + // clearImmediate works just fine with unref'd immediates + const immA = setImmediate(() => { + clearImmediate(immA); + clearImmediate(immB); + // this should not keep the event loop open indefinitely + // or do anything else weird + immA.ref(); + immB.ref(); + }).unref(); + const immB = setImmediate(common.mustNotCall()).unref(); + setImmediate(common.mustCall(finalStep)); +} + +function finalStep() { + // This immediate should not execute as it was unrefed + // and nothing else is keeping the event loop alive + setImmediate(common.mustNotCall()).unref(); +} From dadf28271c96f11aea77878a79263e2bfa4f8bf8 Mon Sep 17 00:00:00 2001 From: Stephen Belanger Date: Thu, 21 Dec 2017 15:10:24 -0800 Subject: [PATCH 02/24] doc: add change info for async_hooks.executionAsyncId() Add meta information to async_hooks documentation informing that executionAsyncId was renamed from currentId at Node.js 8.2.0. PR-URL: https://github.com/nodejs/node/pull/17813 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Ali Ijaz Sheikh --- doc/api/async_hooks.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/api/async_hooks.md b/doc/api/async_hooks.md index e8cb9344c4c..fdc1158f22b 100644 --- a/doc/api/async_hooks.md +++ b/doc/api/async_hooks.md @@ -470,6 +470,14 @@ init for PROMISE with id 6, trigger id: 5 # the Promise returned by then() #### `async_hooks.executionAsyncId()` + + * Returns: {number} The `asyncId` of the current execution context. Useful to track when something calls. From 0c417821a087a17fa3e645e2ba8dbee56dfc29a1 Mon Sep 17 00:00:00 2001 From: Franziska Hinkelmann Date: Mon, 15 Jan 2018 13:18:40 +0100 Subject: [PATCH 03/24] doc: V8 branch used in 8.x not active anymore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add 8.x to the LTS versions that use a V8 branch that is not active anymore. PR-URL: https://github.com/nodejs/node/pull/18155 Reviewed-By: Evan Lucas Reviewed-By: Michaël Zasso Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- doc/guides/maintaining-V8.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/guides/maintaining-V8.md b/doc/guides/maintaining-V8.md index 859dd63e7ba..04bc1556601 100644 --- a/doc/guides/maintaining-V8.md +++ b/doc/guides/maintaining-V8.md @@ -143,9 +143,10 @@ includes the following branches1: -The versions of V8 used in Node.js v4.x and v6.x have already been abandoned by -upstream V8. However, Node.js needs to continue supporting these branches for -many months (Current branches) or several years (LTS branches). +The versions of V8 used in Node.js v4.x, v6.x, and 8.x have already been +abandoned by upstream V8. However, Node.js needs to continue supporting +these branches for many months (Current branches) or several +years (LTS branches). ## Maintenance Process From da3078835a78e1348d5f30c8c8f1c2301bc1ccc4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 8 Jan 2018 01:52:17 +0100 Subject: [PATCH 04/24] src: introduce internal buffer slice constructor Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: https://github.com/nodejs/node/pull/18030 Reviewed-By: James M Snell --- src/node_buffer.cc | 48 ++++++++++++++++++-------------------------- src/node_internals.h | 13 ++++++++++++ 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 2705fc1c078..72776304c8e 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -303,15 +303,14 @@ MaybeLocal New(Environment* env, size_t length) { data, length, ArrayBufferCreationMode::kInternalized); - Local ui = Uint8Array::New(ab, 0, length); - Maybe mb = - ui->SetPrototype(env->context(), env->buffer_prototype_object()); - if (mb.FromMaybe(false)) - return scope.Escape(ui); + MaybeLocal ui = Buffer::New(env, ab, 0, length); - // Object failed to be created. Clean up resources. - free(data); - return Local(); + if (ui.IsEmpty()) { + // Object failed to be created. Clean up resources. + free(data); + } + + return scope.Escape(ui.FromMaybe(Local())); } @@ -349,15 +348,14 @@ MaybeLocal Copy(Environment* env, const char* data, size_t length) { new_data, length, ArrayBufferCreationMode::kInternalized); - Local ui = Uint8Array::New(ab, 0, length); - Maybe mb = - ui->SetPrototype(env->context(), env->buffer_prototype_object()); - if (mb.FromMaybe(false)) - return scope.Escape(ui); + MaybeLocal ui = Buffer::New(env, ab, 0, length); - // Object failed to be created. Clean up resources. - free(new_data); - return Local(); + if (ui.IsEmpty()) { + // Object failed to be created. Clean up resources. + free(new_data); + } + + return scope.Escape(ui.FromMaybe(Local())); } @@ -392,15 +390,14 @@ MaybeLocal New(Environment* env, // correct. if (data == nullptr) ab->Neuter(); - Local ui = Uint8Array::New(ab, 0, length); - Maybe mb = - ui->SetPrototype(env->context(), env->buffer_prototype_object()); + MaybeLocal ui = Buffer::New(env, ab, 0, length); - if (!mb.FromMaybe(false)) + if (ui.IsEmpty()) { return Local(); + } CallbackInfo::New(env->isolate(), ab, callback, data, hint); - return scope.Escape(ui); + return scope.Escape(ui.ToLocalChecked()); } @@ -415,8 +412,6 @@ MaybeLocal New(Isolate* isolate, char* data, size_t length) { MaybeLocal New(Environment* env, char* data, size_t length) { - EscapableHandleScope scope(env->isolate()); - if (length > 0) { CHECK_NE(data, nullptr); CHECK(length <= kMaxLength); @@ -427,12 +422,7 @@ MaybeLocal New(Environment* env, char* data, size_t length) { data, length, ArrayBufferCreationMode::kInternalized); - Local ui = Uint8Array::New(ab, 0, length); - Maybe mb = - ui->SetPrototype(env->context(), env->buffer_prototype_object()); - if (mb.FromMaybe(false)) - return scope.Escape(ui); - return Local(); + return Buffer::New(env, ab, 0, length).FromMaybe(Local()); } namespace { diff --git a/src/node_internals.h b/src/node_internals.h index 09bcbba6e0b..bb03d490f83 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -357,6 +357,19 @@ v8::MaybeLocal New(Environment* env, // Mixing operator new and free() is undefined behavior so don't do that. v8::MaybeLocal New(Environment* env, char* data, size_t length); +inline +v8::MaybeLocal New(Environment* env, + v8::Local ab, + size_t byte_offset, + size_t length) { + v8::Local ui = v8::Uint8Array::New(ab, byte_offset, length); + v8::Maybe mb = + ui->SetPrototype(env->context(), env->buffer_prototype_object()); + if (mb.IsNothing()) + return v8::MaybeLocal(); + return ui; +} + // Construct a Buffer from a MaybeStackBuffer (and also its subclasses like // Utf8Value and TwoByteValue). // If |buf| is invalidated, an empty MaybeLocal is returned, and nothing is From 0625627d824fb539cf02e797c6aab2b17b6df99f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 8 Jan 2018 03:50:51 +0100 Subject: [PATCH 05/24] http2: refactor read mechanism Refactor the read mechanism to completely avoid copying. Instead of copying individual `DATA` frame contents into buffers, create `ArrayBuffer` instances for all socket reads and emit slices of those `ArrayBuffer`s to JS. PR-URL: https://github.com/nodejs/node/pull/18030 Reviewed-By: James M Snell --- lib/internal/http2/core.js | 30 ++- src/node_http2.cc | 184 ++++++++---------- src/node_http2.h | 18 +- test/common/README.md | 11 ++ test/common/index.js | 6 + test/parallel/test-http2-backpressure.js | 49 +++++ ...t-http2-misbehaving-flow-control-paused.js | 3 + 7 files changed, 174 insertions(+), 127 deletions(-) create mode 100644 test/parallel/test-http2-backpressure.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 88afd08a151..7c5792c6ce1 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -279,7 +279,7 @@ function submitRstStream(code) { // point, close them. If there is an open fd for file send, close that also. // At this point the underlying node::http2:Http2Stream handle is no // longer usable so destroy it also. -function onStreamClose(code, hasData) { +function onStreamClose(code) { const stream = this[kOwner]; if (stream.destroyed) return; @@ -287,8 +287,7 @@ function onStreamClose(code, hasData) { const state = stream[kState]; debug(`Http2Stream ${stream[kID]} [Http2Session ` + - `${sessionName(stream[kSession][kType])}]: closed with code ${code}` + - ` [has data? ${hasData}]`); + `${sessionName(stream[kSession][kType])}]: closed with code ${code}`); if (!stream.closed) { // Clear timeout and remove timeout listeners @@ -306,13 +305,14 @@ function onStreamClose(code, hasData) { if (state.fd !== undefined) tryClose(state.fd); - stream[kMaybeDestroy](null, code, hasData); + stream.push(null); + stream[kMaybeDestroy](null, code); } // Receives a chunk of data for a given stream and forwards it on // to the Http2Stream Duplex for processing. -function onStreamRead(nread, buf, handle) { - const stream = handle[kOwner]; +function onStreamRead(nread, buf) { + const stream = this[kOwner]; if (nread >= 0 && !stream.destroyed) { debug(`Http2Stream ${stream[kID]} [Http2Session ` + `${sessionName(stream[kSession][kType])}]: receiving data chunk ` + @@ -320,7 +320,7 @@ function onStreamRead(nread, buf, handle) { stream[kUpdateTimer](); if (!stream.push(buf)) { if (!stream.destroyed) // we have to check a second time - handle.readStop(); + this.readStop(); } return; } @@ -1431,13 +1431,8 @@ function streamOnResume() { } function streamOnPause() { - // if (!this.destroyed && !this.pending) - // this[kHandle].readStop(); -} - -function handleFlushData(self) { if (!this.destroyed && !this.pending) - this[kHandle].flushData(); + this[kHandle].readStop(); } // If the writable side of the Http2Stream is still open, emit the @@ -1686,11 +1681,10 @@ class Http2Stream extends Duplex { this.push(null); return; } - const flushfn = handleFlushData.bind(this); if (!this.pending) { - flushfn(); + streamOnResume.call(this); } else { - this.once('ready', flushfn); + this.once('ready', streamOnResume); } } @@ -1831,10 +1825,10 @@ class Http2Stream extends Duplex { // The Http2Stream can be destroyed if it has closed and if the readable // side has received the final chunk. - [kMaybeDestroy](error, code = NGHTTP2_NO_ERROR, hasData = true) { + [kMaybeDestroy](error, code = NGHTTP2_NO_ERROR) { if (error == null) { if (code === NGHTTP2_NO_ERROR && - ((!this._readableState.ended && hasData) || + (!this._readableState.ended || !this._writableState.ended || this._writableState.pendingcb > 0 || !this.closed)) { diff --git a/src/node_http2.cc b/src/node_http2.cc index cf346efba45..cd28f57ffe2 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -9,6 +9,7 @@ namespace node { +using v8::ArrayBuffer; using v8::Boolean; using v8::Context; using v8::Float64Array; @@ -978,7 +979,6 @@ inline int Http2Session::OnStreamClose(nghttp2_session* handle, // Intentionally ignore the callback if the stream does not exist or has // already been destroyed if (stream != nullptr && !stream->IsDestroyed()) { - stream->AddChunk(nullptr, 0); stream->Close(code); // It is possible for the stream close to occur before the stream is // ever passed on to the javascript side. If that happens, skip straight @@ -989,9 +989,8 @@ inline int Http2Session::OnStreamClose(nghttp2_session* handle, stream->object()->Get(context, env->onstreamclose_string()) .ToLocalChecked(); if (fn->IsFunction()) { - Local argv[2] = { - Integer::NewFromUnsigned(isolate, code), - Boolean::New(isolate, stream->HasDataChunks(true)) + Local argv[] = { + Integer::NewFromUnsigned(isolate, code) }; stream->MakeCallback(fn.As(), arraysize(argv), argv); } else { @@ -1028,6 +1027,8 @@ inline int Http2Session::OnDataChunkReceived(nghttp2_session* handle, Http2Session* session = static_cast(user_data); DEBUG_HTTP2SESSION2(session, "buffering data chunk for stream %d, size: " "%d, flags: %d", id, len, flags); + Environment* env = session->env(); + HandleScope scope(env->isolate()); // We should never actually get a 0-length chunk so this check is // only a precaution at this point. if (len > 0) { @@ -1039,8 +1040,25 @@ inline int Http2Session::OnDataChunkReceived(nghttp2_session* handle, // If the stream has been destroyed, ignore this chunk if (stream->IsDestroyed()) return 0; + stream->statistics_.received_bytes += len; - stream->AddChunk(data, len); + + // There is a single large array buffer for the entire data read from the + // network; create a slice of that array buffer and emit it as the + // received data buffer. + CHECK(!session->stream_buf_ab_.IsEmpty()); + size_t offset = reinterpret_cast(data) - session->stream_buf_; + // Verify that the data offset is inside the current read buffer. + CHECK_LE(offset, session->stream_buf_size_); + + Local buf = + Buffer::New(env, session->stream_buf_ab_, offset, len).ToLocalChecked(); + + stream->EmitData(len, buf, Local()); + if (!stream->IsReading()) + stream->inbound_consumed_data_while_paused_ += len; + else + nghttp2_session_consume_stream(handle, id, len); } return 0; } @@ -1226,9 +1244,8 @@ inline void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { // Called by OnFrameReceived when a complete DATA frame has been received. -// If we know that this is the last DATA frame (because the END_STREAM flag -// is set), then we'll terminate the readable side of the StreamBase. If -// the StreamBase is flowing, we'll push the chunks of data out to JS land. +// If we know that this was the last DATA frame (because the END_STREAM flag +// is set), then we'll terminate the readable side of the StreamBase. inline void Http2Session::HandleDataFrame(const nghttp2_frame* frame) { int32_t id = GetFrameID(frame); DEBUG_HTTP2SESSION2(this, "handling data frame for stream %d", id); @@ -1239,11 +1256,8 @@ inline void Http2Session::HandleDataFrame(const nghttp2_frame* frame) { return; if (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { - stream->AddChunk(nullptr, 0); + stream->EmitData(UV_EOF, Local(), Local()); } - - if (stream->IsReading()) - stream->FlushDataChunks(); } @@ -1618,45 +1632,67 @@ void Http2Session::OnStreamAllocImpl(size_t suggested_size, uv_buf_t* buf, void* ctx) { Http2Session* session = static_cast(ctx); - buf->base = session->stream_alloc(); - buf->len = kAllocBufferSize; + CHECK_EQ(session->stream_buf_, nullptr); + CHECK_EQ(session->stream_buf_size_, 0); + buf->base = session->stream_buf_ = Malloc(suggested_size); + buf->len = session->stream_buf_size_ = suggested_size; + session->IncrementCurrentSessionMemory(suggested_size); } // Callback used to receive inbound data from the i/o stream void Http2Session::OnStreamReadImpl(ssize_t nread, - const uv_buf_t* bufs, + const uv_buf_t* buf, uv_handle_type pending, void* ctx) { Http2Session* session = static_cast(ctx); Http2Scope h2scope(session); CHECK_NE(session->stream_, nullptr); DEBUG_HTTP2SESSION2(session, "receiving %d bytes", nread); - if (nread < 0) { - uv_buf_t tmp_buf; - tmp_buf.base = nullptr; - tmp_buf.len = 0; - session->prev_read_cb_.fn(nread, - &tmp_buf, - pending, - session->prev_read_cb_.ctx); - return; - } - if (bufs->len > 0) { + if (nread <= 0) { + free(session->stream_buf_); + if (nread < 0) { + uv_buf_t tmp_buf = uv_buf_init(nullptr, 0); + session->prev_read_cb_.fn(nread, + &tmp_buf, + pending, + session->prev_read_cb_.ctx); + } + } else { // Only pass data on if nread > 0 - uv_buf_t buf[] { uv_buf_init((*bufs).base, nread) }; + + // Verify that currently: There is memory allocated into which + // the data has been read, and that memory buffer is at least as large + // as the amount of data we have read, but we have not yet made an + // ArrayBuffer out of it. + CHECK_NE(session->stream_buf_, nullptr); + CHECK_EQ(session->stream_buf_, buf->base); + CHECK_EQ(session->stream_buf_size_, buf->len); + CHECK_GE(session->stream_buf_size_, static_cast(nread)); + CHECK(session->stream_buf_ab_.IsEmpty()); + + Environment* env = session->env(); + Isolate* isolate = env->isolate(); + HandleScope scope(isolate); + Local context = env->context(); + Context::Scope context_scope(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. + session->stream_buf_ab_ = + ArrayBuffer::New(isolate, + session->stream_buf_, + session->stream_buf_size_, + v8::ArrayBufferCreationMode::kInternalized); + + uv_buf_t buf_ = uv_buf_init(buf->base, nread); session->statistics_.data_received += nread; - ssize_t ret = session->Write(buf, 1); + ssize_t ret = session->Write(&buf_, 1); // Note: if ssize_t is not defined (e.g. on Win32), nghttp2 will typedef // ssize_t to int. Cast here so that the < 0 check actually works on // Windows. if (static_cast(ret) < 0) { DEBUG_HTTP2SESSION2(session, "fatal error receiving data: %d", ret); - Environment* env = session->env(); - Isolate* isolate = env->isolate(); - HandleScope scope(isolate); - Local context = env->context(); - Context::Scope context_scope(context); Local argv[1] = { Integer::New(isolate, ret), @@ -1667,6 +1703,13 @@ void Http2Session::OnStreamReadImpl(ssize_t nread, nghttp2_session_want_read(**session)); } } + + // Since we are finished handling this write, reset the stream buffer. + // The memory has either been free()d or was handed over to V8. + session->DecrementCurrentSessionMemory(session->stream_buf_size_); + session->stream_buf_ = nullptr; + session->stream_buf_size_ = 0; + session->stream_buf_ab_ = Local(); } void Http2Session::OnStreamDestructImpl(void* ctx) { @@ -1781,30 +1824,6 @@ void Http2Stream::OnTrailers(const SubmitTrailers& submit_trailers) { } } -inline bool Http2Stream::HasDataChunks(bool ignore_eos) { - return data_chunks_.size() > (ignore_eos ? 1 : 0); -} - -// Appends a chunk of received DATA frame data to this Http2Streams internal -// queue. Note that we must memcpy each chunk because of the way that nghttp2 -// handles it's internal memory`. -inline void Http2Stream::AddChunk(const uint8_t* data, size_t len) { - CHECK(!this->IsDestroyed()); - if (this->statistics_.first_byte == 0) - this->statistics_.first_byte = uv_hrtime(); - if (flags_ & NGHTTP2_STREAM_FLAG_EOS) - return; - char* buf = nullptr; - if (len > 0 && data != nullptr) { - buf = Malloc(len); - memcpy(buf, data, len); - } else if (data == nullptr) { - flags_ |= NGHTTP2_STREAM_FLAG_EOS; - } - data_chunks_.emplace(uv_buf_init(buf, len)); -} - - inline void Http2Stream::Close(int32_t code) { CHECK(!this->IsDestroyed()); flags_ |= NGHTTP2_STREAM_FLAG_CLOSED; @@ -1841,13 +1860,6 @@ inline void Http2Stream::Destroy() { DEBUG_HTTP2STREAM(this, "destroying stream"); - // Free any remaining incoming data chunks. - while (!data_chunks_.empty()) { - uv_buf_t buf = data_chunks_.front(); - free(buf.base); - data_chunks_.pop(); - } - // Wait until the start of the next loop to delete because there // may still be some pending operations queued for this stream. env()->SetImmediate([](Environment* env, void* data) { @@ -1873,39 +1885,6 @@ inline void Http2Stream::Destroy() { } -// Uses the StreamBase API to push a single chunk of queued inbound DATA -// to JS land. -void Http2Stream::OnDataChunk(uv_buf_t* chunk) { - CHECK(!this->IsDestroyed()); - Isolate* isolate = env()->isolate(); - HandleScope scope(isolate); - ssize_t len = -1; - Local buf; - if (chunk != nullptr) { - len = chunk->len; - buf = Buffer::New(isolate, chunk->base, len).ToLocalChecked(); - } - EmitData(len, buf, this->object()); -} - - -inline void Http2Stream::FlushDataChunks() { - CHECK(!this->IsDestroyed()); - Http2Scope h2scope(this); - if (!data_chunks_.empty()) { - uv_buf_t buf = data_chunks_.front(); - data_chunks_.pop(); - if (buf.len > 0) { - CHECK_EQ(nghttp2_session_consume_stream(session_->session(), - id_, buf.len), 0); - OnDataChunk(&buf); - } else { - OnDataChunk(nullptr); - } - } -} - - // Initiates a response on the Http2Stream using data provided via the // StreamBase Streams API. inline int Http2Stream::SubmitResponse(nghttp2_nv* nva, @@ -2012,13 +1991,20 @@ inline Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva, // Switch the StreamBase into flowing mode to begin pushing chunks of data // out to JS land. inline int Http2Stream::ReadStart() { + Http2Scope h2scope(this); CHECK(!this->IsDestroyed()); flags_ |= NGHTTP2_STREAM_FLAG_READ_START; flags_ &= ~NGHTTP2_STREAM_FLAG_READ_PAUSED; - // Flush any queued data chunks immediately out to the JS layer - FlushDataChunks(); DEBUG_HTTP2STREAM(this, "reading starting"); + + // Tell nghttp2 about our consumption of the data that was handed + // off to JS land. + nghttp2_session_consume_stream(session_->session(), + id_, + inbound_consumed_data_while_paused_); + inbound_consumed_data_while_paused_ = 0; + return 0; } diff --git a/src/node_http2.h b/src/node_http2.h index 4ed06c95970..9027ed7feb7 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -550,12 +550,6 @@ class Http2Stream : public AsyncWrap, inline void EmitStatistics(); - inline bool HasDataChunks(bool ignore_eos = false); - - inline void AddChunk(const uint8_t* data, size_t len); - - inline void FlushDataChunks(); - // Process a Data Chunk void OnDataChunk(uv_buf_t* chunk); @@ -740,8 +734,11 @@ class Http2Stream : public AsyncWrap, uint32_t current_headers_length_ = 0; // total number of octets std::vector current_headers_; - // Inbound Data... This is the data received via DATA frames for this stream. - std::queue data_chunks_; + // This keeps track of the amount of data read from the socket while the + // socket was in paused mode. When `ReadStart()` is called (and not before + // then), we tell nghttp2 that we consumed that data to get proper + // backpressure handling. + size_t inbound_consumed_data_while_paused_ = 0; // Outbound Data... This is the data written by the JS layer that is // waiting to be written out to the socket. @@ -1085,8 +1082,9 @@ class Http2Session : public AsyncWrap { // use this to allow timeout tracking during long-lasting writes uint32_t chunks_sent_since_last_write_ = 0; - uv_prepare_t* prep_ = nullptr; - char stream_buf_[kAllocBufferSize]; + char* stream_buf_ = nullptr; + size_t stream_buf_size_ = 0; + v8::Local stream_buf_ab_; size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; std::queue outstanding_pings_; diff --git a/test/common/README.md b/test/common/README.md index b50d8ca88ee..3e3e5543b40 100644 --- a/test/common/README.md +++ b/test/common/README.md @@ -268,6 +268,17 @@ fail. If `fn` is not provided, an empty function will be used. +### mustCallAsync([fn][, exact]) +* `fn` [<Function>] +* `exact` [<Number>] default = 1 +* return [<Function>] + +The same as `mustCall()`, except that it is also checked that the Promise +returned by the function is fulfilled for each invocation of the function. + +The return value of the wrapped function is the return value of the original +function, if necessary wrapped as a promise. + ### mustCallAtLeast([fn][, minimum]) * `fn` [<Function>] default = () => {} * `minimum` [<Number>] default = 1 diff --git a/test/common/index.js b/test/common/index.js index 418abef1b37..0cc48cf74cd 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -501,6 +501,12 @@ exports.mustCallAtLeast = function(fn, minimum) { return _mustCallInner(fn, minimum, 'minimum'); }; +exports.mustCallAsync = function(fn, exact) { + return exports.mustCall((...args) => { + return Promise.resolve(fn(...args)).then(exports.mustCall((val) => val)); + }, exact); +}; + function _mustCallInner(fn, criteria = 1, field) { if (process._exiting) throw new Error('Cannot use common.mustCall*() in process exit handler'); diff --git a/test/parallel/test-http2-backpressure.js b/test/parallel/test-http2-backpressure.js new file mode 100644 index 00000000000..9b69dddbfd2 --- /dev/null +++ b/test/parallel/test-http2-backpressure.js @@ -0,0 +1,49 @@ +'use strict'; + +// Verifies that a full HTTP2 pipeline handles backpressure. + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); +const makeDuplexPair = require('../common/duplexpair'); + +common.crashOnUnhandledRejection(); + +{ + let req; + const server = http2.createServer(); + server.on('stream', common.mustCallAsync(async (stream, headers) => { + stream.respond({ + 'content-type': 'text/html', + ':status': 200 + }); + req._readableState.highWaterMark = 20; + stream._writableState.highWaterMark = 20; + assert.strictEqual(stream.write('A'.repeat(5)), true); + assert.strictEqual(stream.write('A'.repeat(40)), false); + assert.strictEqual(await event(req, 'data'), 'A'.repeat(5)); + assert.strictEqual(await event(req, 'data'), 'A'.repeat(40)); + await event(stream, 'drain'); + assert.strictEqual(stream.write('A'.repeat(5)), true); + assert.strictEqual(stream.write('A'.repeat(40)), false); + })); + + const { clientSide, serverSide } = makeDuplexPair(); + server.emit('connection', serverSide); + + const client = http2.connect('http://localhost:80', { + createConnection: common.mustCall(() => clientSide) + }); + + req = client.request({ ':path': '/' }); + req.setEncoding('utf8'); + req.end(); +} + +function event(ee, eventName) { + return new Promise((resolve) => { + ee.once(eventName, common.mustCall(resolve)); + }); +} diff --git a/test/parallel/test-http2-misbehaving-flow-control-paused.js b/test/parallel/test-http2-misbehaving-flow-control-paused.js index 0b7299d5ac8..d69e0fd8029 100644 --- a/test/parallel/test-http2-misbehaving-flow-control-paused.js +++ b/test/parallel/test-http2-misbehaving-flow-control-paused.js @@ -56,6 +56,9 @@ let client; const server = h2.createServer({ settings: { initialWindowSize: 36 } }); server.on('stream', (stream) => { + // Set the high water mark to zero, since otherwise we still accept + // reads from the source stream (if we can consume them). + stream._readableState.highWaterMark = 0; stream.pause(); stream.on('error', common.expectsError({ code: 'ERR_HTTP2_STREAM_ERROR', From 7972bdf783c62247bea594ebd4bbb3e39925a0de Mon Sep 17 00:00:00 2001 From: Sho Miyamoto Date: Sat, 13 Jan 2018 22:05:26 +0900 Subject: [PATCH 06/24] test: add assertions for TextEncoder/Decoder PR-URL: https://github.com/nodejs/node/pull/18132 Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Colin Ihrig --- .../test-whatwg-encoding-textdecoder.js | 41 +++++++++++++------ .../test-whatwg-encoding-textencoder.js | 34 +++++++++------ 2 files changed, 51 insertions(+), 24 deletions(-) diff --git a/test/parallel/test-whatwg-encoding-textdecoder.js b/test/parallel/test-whatwg-encoding-textdecoder.js index c4f919289ad..5f9499930a0 100644 --- a/test/parallel/test-whatwg-encoding-textdecoder.js +++ b/test/parallel/test-whatwg-encoding-textdecoder.js @@ -91,18 +91,35 @@ if (common.hasIntl) { } { - const fn = TextDecoder.prototype[inspect]; - assert.doesNotThrow(() => { - fn.call(new TextDecoder(), Infinity, {}); - }); - - [{}, [], true, 1, '', new TextEncoder()].forEach((i) => { - common.expectsError(() => fn.call(i, Infinity, {}), - { - code: 'ERR_INVALID_THIS', - type: TypeError, - message: 'Value of "this" must be of type TextDecoder' - }); + const inspectFn = TextDecoder.prototype[inspect]; + const decodeFn = TextDecoder.prototype.decode; + const { + encoding: { get: encodingGetter }, + fatal: { get: fatalGetter }, + ignoreBOM: { get: ignoreBOMGetter }, + } = Object.getOwnPropertyDescriptors(TextDecoder.prototype); + + const instance = new TextDecoder(); + + const expectedError = { + code: 'ERR_INVALID_THIS', + type: TypeError, + message: 'Value of "this" must be of type TextDecoder' + }; + + assert.doesNotThrow(() => inspectFn.call(instance, Infinity, {})); + assert.doesNotThrow(() => decodeFn.call(instance)); + assert.doesNotThrow(() => encodingGetter.call(instance)); + assert.doesNotThrow(() => fatalGetter.call(instance)); + assert.doesNotThrow(() => ignoreBOMGetter.call(instance)); + + const invalidThisArgs = [{}, [], true, 1, '', new TextEncoder()]; + invalidThisArgs.forEach((i) => { + common.expectsError(() => inspectFn.call(i, Infinity, {}), expectedError); + common.expectsError(() => decodeFn.call(i), expectedError); + common.expectsError(() => encodingGetter.call(i), expectedError); + common.expectsError(() => fatalGetter.call(i), expectedError); + common.expectsError(() => ignoreBOMGetter.call(i), expectedError); }); } diff --git a/test/parallel/test-whatwg-encoding-textencoder.js b/test/parallel/test-whatwg-encoding-textencoder.js index fba5445c819..4096a02432e 100644 --- a/test/parallel/test-whatwg-encoding-textencoder.js +++ b/test/parallel/test-whatwg-encoding-textencoder.js @@ -35,17 +35,27 @@ assert(TextEncoder); } { - const fn = TextEncoder.prototype[inspect]; - assert.doesNotThrow(() => { - fn.call(new TextEncoder(), Infinity, {}); - }); - - [{}, [], true, 1, '', new TextDecoder()].forEach((i) => { - common.expectsError(() => fn.call(i, Infinity, {}), - { - code: 'ERR_INVALID_THIS', - type: TypeError, - message: 'Value of "this" must be of type TextEncoder' - }); + const inspectFn = TextEncoder.prototype[inspect]; + const encodeFn = TextEncoder.prototype.encode; + const encodingGetter = + Object.getOwnPropertyDescriptor(TextEncoder.prototype, 'encoding').get; + + const instance = new TextEncoder(); + + const expectedError = { + code: 'ERR_INVALID_THIS', + type: TypeError, + message: 'Value of "this" must be of type TextEncoder' + }; + + assert.doesNotThrow(() => inspectFn.call(instance, Infinity, {})); + assert.doesNotThrow(() => encodeFn.call(instance)); + assert.doesNotThrow(() => encodingGetter.call(instance)); + + const invalidThisArgs = [{}, [], true, 1, '', new TextDecoder()]; + invalidThisArgs.forEach((i) => { + common.expectsError(() => inspectFn.call(i, Infinity, {}), expectedError); + common.expectsError(() => encodeFn.call(i), expectedError); + common.expectsError(() => encodingGetter.call(i), expectedError); }); } From 628307774eaf013654b3755f8c9ee6935fba06ad Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 13 Jan 2018 14:41:16 +0100 Subject: [PATCH 07/24] src: refactor callback #defines into C++ templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use template helpers instead of `#define`s to generate the raw C callbacks that are passed to the HTTP parser library. A nice effect of this is that it is more obvious what parameters the `Parser` methods take. PR-URL: https://github.com/nodejs/node/pull/18133 Reviewed-By: James M Snell Reviewed-By: Jon Moss Reviewed-By: Tobias Nießen Reviewed-By: Joyee Cheung Reviewed-By: Franziska Hinkelmann --- src/node_http_parser.cc | 64 ++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index f378a0475a6..0f170505455 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -72,22 +72,6 @@ const uint32_t kOnMessageComplete = 3; const uint32_t kOnExecute = 4; -#define HTTP_CB(name) \ - static int name(http_parser* p_) { \ - Parser* self = ContainerOf(&Parser::parser_, p_); \ - return self->name##_(); \ - } \ - int name##_() - - -#define HTTP_DATA_CB(name) \ - static int name(http_parser* p_, const char* at, size_t length) { \ - Parser* self = ContainerOf(&Parser::parser_, p_); \ - return self->name##_(at, length); \ - } \ - int name##_(const char* at, size_t length) - - // helper class for the Parser struct StringPtr { StringPtr() { @@ -182,7 +166,7 @@ class Parser : public AsyncWrap { } - HTTP_CB(on_message_begin) { + int on_message_begin() { num_fields_ = num_values_ = 0; url_.Reset(); status_message_.Reset(); @@ -190,19 +174,19 @@ class Parser : public AsyncWrap { } - HTTP_DATA_CB(on_url) { + int on_url(const char* at, size_t length) { url_.Update(at, length); return 0; } - HTTP_DATA_CB(on_status) { + int on_status(const char* at, size_t length) { status_message_.Update(at, length); return 0; } - HTTP_DATA_CB(on_header_field) { + int on_header_field(const char* at, size_t length) { if (num_fields_ == num_values_) { // start of new field name num_fields_++; @@ -224,7 +208,7 @@ class Parser : public AsyncWrap { } - HTTP_DATA_CB(on_header_value) { + int on_header_value(const char* at, size_t length) { if (num_values_ != num_fields_) { // start of new header value num_values_++; @@ -240,7 +224,7 @@ class Parser : public AsyncWrap { } - HTTP_CB(on_headers_complete) { + int on_headers_complete() { // Arguments for the on-headers-complete javascript callback. This // list needs to be kept in sync with the actual argument list for // `parserOnHeadersComplete` in lib/_http_common.js. @@ -317,7 +301,7 @@ class Parser : public AsyncWrap { } - HTTP_DATA_CB(on_body) { + int on_body(const char* at, size_t length) { EscapableHandleScope scope(env()->isolate()); Local obj = object(); @@ -354,7 +338,7 @@ class Parser : public AsyncWrap { } - HTTP_CB(on_message_complete) { + int on_message_complete() { HandleScope scope(env()->isolate()); if (num_fields_) @@ -751,21 +735,35 @@ class Parser : public AsyncWrap { StreamResource::Callback prev_alloc_cb_; StreamResource::Callback prev_read_cb_; int refcount_ = 1; + + // These are helper functions for filling `http_parser_settings`, which turn + // a member function of Parser into a C-style HTTP parser callback. + template struct Proxy; + template + struct Proxy { + static int Raw(http_parser* p, Args ... args) { + Parser* parser = ContainerOf(&Parser::parser_, p); + return (parser->*Member)(std::forward(args)...); + } + }; + + typedef int (Parser::*Call)(); + typedef int (Parser::*DataCall)(const char* at, size_t length); + static const struct http_parser_settings settings; friend class ScopedRetainParser; }; - const struct http_parser_settings Parser::settings = { - Parser::on_message_begin, - Parser::on_url, - Parser::on_status, - Parser::on_header_field, - Parser::on_header_value, - Parser::on_headers_complete, - Parser::on_body, - Parser::on_message_complete, + Proxy::Raw, + Proxy::Raw, + Proxy::Raw, + Proxy::Raw, + Proxy::Raw, + Proxy::Raw, + Proxy::Raw, + Proxy::Raw, nullptr, // on_chunk_header nullptr // on_chunk_complete }; From a7a1ada5b2afd586a8b73822ae0b3f59059a3552 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 13 Jan 2018 17:48:07 +0100 Subject: [PATCH 08/24] http: simplify parser lifetime tracking Instead of providing a separate class for keeping the parser alive during its own call back, just delay a possible `.close()` call until the stack has cleared completely. PR-URL: https://github.com/nodejs/node/pull/18135 Reviewed-By: James M Snell Reviewed-By: Franziska Hinkelmann Reviewed-By: Anatoli Papirovski Reviewed-By: Colin Ihrig --- lib/_http_common.js | 5 ++++- src/node_http_parser.cc | 24 +----------------------- 2 files changed, 5 insertions(+), 24 deletions(-) diff --git a/lib/_http_common.js b/lib/_http_common.js index cf37bbebe36..6ad13104772 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -190,6 +190,7 @@ var parsers = new FreeList('parsers', 1000, function() { return parser; }); +function closeParserInstance(parser) { parser.close(); } // Free the parser and also break any links that it // might have to any other things. @@ -212,7 +213,9 @@ function freeParser(parser, req, socket) { parser.outgoing = null; parser[kOnExecute] = null; if (parsers.free(parser) === false) { - parser.close(); + // Make sure the parser's stack has unwound before deleting the + // corresponding C++ object through .close(). + setImmediate(closeParserInstance, parser); } else { // Since the Parser destructor isn't going to run the destroy() callbacks // it needs to be triggered manually. diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 0f170505455..9debb8a205e 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -376,8 +376,7 @@ class Parser : public AsyncWrap { Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); - if (--parser->refcount_ == 0) - delete parser; + delete parser; } @@ -543,22 +542,6 @@ class Parser : public AsyncWrap { } protected: - class ScopedRetainParser { - public: - explicit ScopedRetainParser(Parser* p) : p_(p) { - CHECK_GT(p_->refcount_, 0); - p_->refcount_++; - } - - ~ScopedRetainParser() { - if (0 == --p_->refcount_) - delete p_; - } - - private: - Parser* const p_; - }; - static const size_t kAllocBufferSize = 64 * 1024; static void OnAllocImpl(size_t suggested_size, uv_buf_t* buf, void* ctx) { @@ -595,8 +578,6 @@ class Parser : public AsyncWrap { if (nread == 0) return; - ScopedRetainParser retain(parser); - parser->current_buffer_.Clear(); Local ret = parser->Execute(buf->base, nread); @@ -734,7 +715,6 @@ class Parser : public AsyncWrap { char* current_buffer_data_; StreamResource::Callback prev_alloc_cb_; StreamResource::Callback prev_read_cb_; - int refcount_ = 1; // These are helper functions for filling `http_parser_settings`, which turn // a member function of Parser into a C-style HTTP parser callback. @@ -751,8 +731,6 @@ class Parser : public AsyncWrap { typedef int (Parser::*DataCall)(const char* at, size_t length); static const struct http_parser_settings settings; - - friend class ScopedRetainParser; }; const struct http_parser_settings Parser::settings = { From 4b9ba9b83390f351d4a9f32ffef7730f87b17d2b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 11 Jan 2018 16:14:33 -0800 Subject: [PATCH 09/24] fs: encapsulate FSReqWrap more In further preparation for the Promises enabled fs API, further encapsulate FSReqWrap details. The intent here is to isolate, as much as possible, the functionality of the various fs functions from the details of how the results are notified. The promises implementation will use a `FSReqPromise` alternative to `FSReqWrap` that will use the same API so that both models can be used without changing any of the actual implementation details for the various methods. PR-URL: https://github.com/nodejs/node/pull/18112 Reviewed-By: Matteo Collina Reviewed-By: Ben Noordhuis Reviewed-By: Anna Henningsen --- src/node_file.cc | 120 +++++++----------- src/node_file.h | 59 +++------ test/sequential/test-async-wrap-getasyncid.js | 3 +- 3 files changed, 67 insertions(+), 115 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index c9bbea1ee5f..c8cdc795915 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -94,6 +94,7 @@ using v8::MaybeLocal; using v8::Number; using v8::Object; using v8::String; +using v8::Undefined; using v8::Value; #ifndef MIN @@ -102,32 +103,16 @@ using v8::Value; #define GET_OFFSET(a) ((a)->IsNumber() ? (a)->IntegerValue() : -1) -FSReqWrap* FSReqWrap::New(Environment* env, - Local req, - const char* syscall, - const char* data, - enum encoding encoding, - Ownership ownership) { - const bool copy = (data != nullptr && ownership == COPY); - const size_t size = copy ? 1 + strlen(data) : 0; - FSReqWrap* that; - char* const storage = new char[sizeof(*that) + size]; - that = new(storage) FSReqWrap(env, req, syscall, data, encoding); - if (copy) - that->data_ = static_cast(memcpy(that->inline_data(), data, size)); - return that; +void FSReqWrap::Reject(Local reject) { + MakeCallback(env()->oncomplete_string(), 1, &reject); } - -void FSReqWrap::Dispose() { - this->~FSReqWrap(); - delete[] reinterpret_cast(this); +void FSReqWrap::FillStatsArray(const uv_stat_t* stat) { + node::FillStatsArray(env()->fs_stats_field_array(), stat); } - -void FSReqWrap::Reject(Local reject) { - Local argv[1] { reject }; - MakeCallback(env()->oncomplete_string(), arraysize(argv), argv); +void FSReqWrap::ResolveStat() { + Resolve(Undefined(env()->isolate())); } void FSReqWrap::Resolve(Local value) { @@ -138,9 +123,26 @@ void FSReqWrap::Resolve(Local value) { MakeCallback(env()->oncomplete_string(), arraysize(argv), argv); } +void FSReqWrap::Init(const char* syscall, + const char* data, + size_t len, + enum encoding encoding) { + syscall_ = syscall; + encoding_ = encoding; + + if (data != nullptr) { + CHECK_EQ(data_, nullptr); + buffer_.AllocateSufficientStorage(len + 1); + buffer_.SetLengthAndZeroTerminate(len); + memcpy(*buffer_, data, len); + data_ = *buffer_; + } +} + void NewFSReqWrap(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); - ClearWrap(args.This()); + Environment* env = Environment::GetCurrent(args.GetIsolate()); + new FSReqWrap(env, args.This()); } @@ -150,12 +152,11 @@ FSReqAfterScope::FSReqAfterScope(FSReqWrap* wrap, uv_fs_t* req) handle_scope_(wrap->env()->isolate()), context_scope_(wrap->env()->context()) { CHECK_EQ(wrap_->req(), req); - wrap_->ReleaseEarly(); // Free memory that's no longer used now. } FSReqAfterScope::~FSReqAfterScope() { uv_fs_req_cleanup(wrap_->req()); - wrap_->Dispose(); + delete wrap_; } // TODO(joyeecheung): create a normal context object, and @@ -195,12 +196,10 @@ void AfterNoArgs(uv_fs_t* req) { void AfterStat(uv_fs_t* req) { FSReqWrap* req_wrap = static_cast(req->data); FSReqAfterScope after(req_wrap, req); - Environment* env = req_wrap->env(); if (after.Proceed()) { - FillStatsArray(env->fs_stats_field_array(), - static_cast(req->ptr)); - req_wrap->Resolve(Undefined(req_wrap->env()->isolate())); + req_wrap->FillStatsArray(&req->statbuf); + req_wrap->ResolveStat(); } } @@ -222,7 +221,7 @@ void AfterStringPath(uv_fs_t* req) { if (after.Proceed()) { link = StringBytes::Encode(req_wrap->env()->isolate(), static_cast(req->path), - req_wrap->encoding_, + req_wrap->encoding(), &error); if (link.IsEmpty()) req_wrap->Reject(error); @@ -241,7 +240,7 @@ void AfterStringPtr(uv_fs_t* req) { if (after.Proceed()) { link = StringBytes::Encode(req_wrap->env()->isolate(), static_cast(req->ptr), - req_wrap->encoding_, + req_wrap->encoding(), &error); if (link.IsEmpty()) req_wrap->Reject(error); @@ -278,7 +277,7 @@ void AfterScanDir(uv_fs_t* req) { MaybeLocal filename = StringBytes::Encode(env->isolate(), ent.name, - req_wrap->encoding_, + req_wrap->encoding(), &error); if (filename.IsEmpty()) return req_wrap->Reject(error); @@ -317,12 +316,12 @@ class fs_req_wrap { template inline FSReqWrap* AsyncDestCall(Environment* env, const FunctionCallbackInfo& args, - const char* syscall, const char* dest, - enum encoding enc, FSReqWrap::Ownership ownership, - uv_fs_cb after, Func fn, Args... fn_args) { + const char* syscall, const char* dest, size_t len, + enum encoding enc, uv_fs_cb after, Func fn, Args... fn_args) { Local req = args[args.Length() - 1].As(); - FSReqWrap* req_wrap = FSReqWrap::New(env, req, - syscall, dest, enc, ownership); + FSReqWrap* req_wrap = Unwrap(req); + CHECK_NE(req_wrap, nullptr); + req_wrap->Init(syscall, dest, len, enc); int err = fn(env->event_loop(), req_wrap->req(), fn_args..., after); req_wrap->Dispatched(); if (err < 0) { @@ -339,38 +338,16 @@ inline FSReqWrap* AsyncDestCall(Environment* env, return req_wrap; } -// Defaults to COPY ownership. -template -inline FSReqWrap* AsyncDestCall(Environment* env, - const FunctionCallbackInfo& args, - const char* syscall, const char* dest, enum encoding enc, - uv_fs_cb after, Func fn, Args... fn_args) { - return AsyncDestCall(env, args, - syscall, dest, enc, FSReqWrap::COPY, - after, fn, fn_args...); -} - template inline FSReqWrap* AsyncCall(Environment* env, const FunctionCallbackInfo& args, - const char* syscall, enum encoding enc, FSReqWrap::Ownership ownership, + const char* syscall, enum encoding enc, uv_fs_cb after, Func fn, Args... fn_args) { return AsyncDestCall(env, args, - syscall, nullptr, enc, ownership, + syscall, nullptr, 0, enc, after, fn, fn_args...); } -// Defaults to COPY ownership. -template -inline FSReqWrap* AsyncCall(Environment* env, - const FunctionCallbackInfo& args, - const char* syscall, enum encoding enc, - uv_fs_cb after, Func fn, Args... fn_args) { - return AsyncCall(env, args, - syscall, enc, FSReqWrap::COPY, - after, fn, fn_args...); -} - // Template counterpart of SYNC_CALL, except that it only puts // the error number and the syscall in the context instead of // creating an error in the C++ land. @@ -623,8 +600,8 @@ static void Symlink(const FunctionCallbackInfo& args) { if (args[3]->IsObject()) { // symlink(target, path, flags, req) CHECK_EQ(args.Length(), 4); - AsyncDestCall(env, args, "symlink", *path, UTF8, AfterNoArgs, - uv_fs_symlink, *target, *path, flags); + AsyncDestCall(env, args, "symlink", *path, path.length(), UTF8, + AfterNoArgs, uv_fs_symlink, *target, *path, flags); } else { // symlink(target, path, flags) SYNC_DEST_CALL(symlink, *target, *path, *target, *path, flags) } @@ -643,8 +620,8 @@ static void Link(const FunctionCallbackInfo& args) { if (args[2]->IsObject()) { // link(src, dest, req) CHECK_EQ(args.Length(), 3); - AsyncDestCall(env, args, "link", *dest, UTF8, AfterNoArgs, - uv_fs_link, *src, *dest); + AsyncDestCall(env, args, "link", *dest, dest.length(), UTF8, + AfterNoArgs, uv_fs_link, *src, *dest); } else { // link(src, dest) SYNC_DEST_CALL(link, *src, *dest, *src, *dest) } @@ -695,8 +672,8 @@ static void Rename(const FunctionCallbackInfo& args) { if (args[2]->IsObject()) { CHECK_EQ(args.Length(), 3); - AsyncDestCall(env, args, "rename", *new_path, UTF8, AfterNoArgs, - uv_fs_rename, *old_path, *new_path); + AsyncDestCall(env, args, "rename", *new_path, new_path.length(), + UTF8, AfterNoArgs, uv_fs_rename, *old_path, *new_path); } else { SYNC_DEST_CALL(rename, *old_path, *new_path, *old_path, *new_path) } @@ -1041,7 +1018,6 @@ static void WriteString(const FunctionCallbackInfo& args) { char* buf = nullptr; int64_t pos; size_t len; - FSReqWrap::Ownership ownership = FSReqWrap::COPY; // will assign buf and len if string was external if (!StringBytes::GetExternalParts(string, @@ -1053,7 +1029,6 @@ static void WriteString(const FunctionCallbackInfo& args) { // StorageSize may return too large a char, so correct the actual length // by the write size len = StringBytes::Write(env->isolate(), buf, len, args[1], enc); - ownership = FSReqWrap::MOVE; } pos = GET_OFFSET(args[2]); @@ -1061,8 +1036,7 @@ static void WriteString(const FunctionCallbackInfo& args) { if (args[4]->IsObject()) { CHECK_EQ(args.Length(), 5); - AsyncCall(env, args, - "write", UTF8, ownership, AfterInteger, + AsyncCall(env, args, "write", UTF8, AfterInteger, uv_fs_write, fd, &uvbuf, 1, pos); } else { // SYNC_CALL returns on error. Make sure to always free the memory. @@ -1071,7 +1045,7 @@ static void WriteString(const FunctionCallbackInfo& args) { inline ~Delete() { delete[] pointer_; } char* const pointer_; }; - Delete delete_on_return(ownership == FSReqWrap::MOVE ? buf : nullptr); + Delete delete_on_return(buf); SYNC_CALL(write, nullptr, fd, &uvbuf, 1, pos) return args.GetReturnValue().Set(SYNC_RESULT); } @@ -1373,7 +1347,7 @@ void InitFs(Local target, Local wrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "FSReqWrap"); fst->SetClassName(wrapString); - target->Set(wrapString, fst->GetFunction()); + target->Set(context, wrapString, fst->GetFunction()).FromJust(); } } // namespace fs diff --git a/src/node_file.h b/src/node_file.h index db85451b67a..878d02c8ba4 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -17,60 +17,39 @@ using v8::Value; namespace fs { -class FSReqWrap: public ReqWrap { +class FSReqWrap : public ReqWrap { public: - enum Ownership { COPY, MOVE }; + FSReqWrap(Environment* env, Local req) + : ReqWrap(env, req, AsyncWrap::PROVIDER_FSREQWRAP) { + Wrap(object(), this); + } - inline static FSReqWrap* New(Environment* env, - Local req, - const char* syscall, - const char* data = nullptr, - enum encoding encoding = UTF8, - Ownership ownership = COPY); + virtual ~FSReqWrap() { + ClearWrap(object()); + } - inline void Dispose(); + void Init(const char* syscall, + const char* data = nullptr, + size_t len = 0, + enum encoding encoding = UTF8); + virtual void FillStatsArray(const uv_stat_t* stat); virtual void Reject(Local reject); virtual void Resolve(Local value); - - void ReleaseEarly() { - if (data_ != inline_data()) { - delete[] data_; - data_ = nullptr; - } - } + virtual void ResolveStat(); const char* syscall() const { return syscall_; } const char* data() const { return data_; } - const enum encoding encoding_; + enum encoding encoding() const { return encoding_; } size_t self_size() const override { return sizeof(*this); } - protected: - FSReqWrap(Environment* env, - Local req, - const char* syscall, - const char* data, - enum encoding encoding) - : ReqWrap(env, req, AsyncWrap::PROVIDER_FSREQWRAP), - encoding_(encoding), - syscall_(syscall), - data_(data) { - Wrap(object(), this); - } - - virtual ~FSReqWrap() { - ReleaseEarly(); - ClearWrap(object()); - } - - void* operator new(size_t size) = delete; - void* operator new(size_t size, char* storage) { return storage; } - char* inline_data() { return reinterpret_cast(this + 1); } - private: + enum encoding encoding_ = UTF8; const char* syscall_; - const char* data_; + + const char* data_ = nullptr; + MaybeStackBuffer buffer_; DISALLOW_COPY_AND_ASSIGN(FSReqWrap); }; diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js index 58d6b774698..f5b6843ef92 100644 --- a/test/sequential/test-async-wrap-getasyncid.js +++ b/test/sequential/test-async-wrap-getasyncid.js @@ -112,9 +112,8 @@ if (common.hasCrypto) { // eslint-disable-line crypto-check const req = new FSReqWrap(); req.oncomplete = () => { }; - testUninitialized(req, 'FSReqWrap'); - binding.access(path.toNamespacedPath('../'), fs.F_OK, req); testInitialized(req, 'FSReqWrap'); + binding.access(path.toNamespacedPath('../'), fs.F_OK, req); const StatWatcher = binding.StatWatcher; testInitialized(new StatWatcher(), 'StatWatcher'); From f6ae451a9682cebb05603bf06cb3161b4068bfdf Mon Sep 17 00:00:00 2001 From: sreepurnajasti Date: Thu, 21 Dec 2017 10:55:21 +0530 Subject: [PATCH 10/24] benchmark: remove redundant + PR-URL: https://github.com/nodejs/node/pull/17803 Refs: https://github.com/nodejs/code-and-learn/issues/72 Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater Reviewed-By: Rich Trott --- benchmark/assert/deepequal-object.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/benchmark/assert/deepequal-object.js b/benchmark/assert/deepequal-object.js index 2efa9452af8..4ad37044083 100644 --- a/benchmark/assert/deepequal-object.js +++ b/benchmark/assert/deepequal-object.js @@ -26,9 +26,9 @@ function createObj(source, add = '') { } function main(conf) { - const size = +conf.size; + const size = conf.size; // TODO: Fix this "hack" - const n = (+conf.n) / size; + const n = conf.n / size; var i; const source = Array.apply(null, Array(size)); From f576341dc28199cc1f92a4ac66b6aa82e22cb486 Mon Sep 17 00:00:00 2001 From: Sho Miyamoto Date: Fri, 12 Jan 2018 23:06:51 +0900 Subject: [PATCH 11/24] test: replace assert.equal with assert.strictEqual PR-URL: https://github.com/nodejs/node/pull/18119 Reviewed-By: Shingo Inoue Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- test/es-module/test-esm-dynamic-import.js | 4 ++-- test/fixtures/es-module-loaders/loader-shared-dep.mjs | 2 +- test/fixtures/es-module-loaders/not-found-assert-loader.mjs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/es-module/test-esm-dynamic-import.js b/test/es-module/test-esm-dynamic-import.js index a099a2ddb8a..997eed289eb 100644 --- a/test/es-module/test-esm-dynamic-import.js +++ b/test/es-module/test-esm-dynamic-import.js @@ -15,7 +15,7 @@ targetURL.pathname = absolutePath; function expectErrorProperty(result, propertyKey, value) { Promise.resolve(result) .catch(common.mustCall(error => { - assert.equal(error[propertyKey], value); + assert.strictEqual(error[propertyKey], value); })); } @@ -51,7 +51,7 @@ function expectOkNamespace(result) { function expectFsNamespace(result) { Promise.resolve(result) .then(common.mustCall(ns => { - assert.equal(typeof ns.default.writeFile, 'function'); + assert.strictEqual(typeof ns.default.writeFile, 'function'); })); } diff --git a/test/fixtures/es-module-loaders/loader-shared-dep.mjs b/test/fixtures/es-module-loaders/loader-shared-dep.mjs index e2a1cbd75d2..1a19e4c8927 100644 --- a/test/fixtures/es-module-loaders/loader-shared-dep.mjs +++ b/test/fixtures/es-module-loaders/loader-shared-dep.mjs @@ -2,6 +2,6 @@ import dep from './loader-dep.js'; import assert from 'assert'; export function resolve(specifier, base, defaultResolve) { - assert.equal(dep.format, 'esm'); + assert.strictEqual(dep.format, 'esm'); return defaultResolve(specifier, base); } diff --git a/test/fixtures/es-module-loaders/not-found-assert-loader.mjs b/test/fixtures/es-module-loaders/not-found-assert-loader.mjs index 7718cc7c4ba..d15f294fe67 100644 --- a/test/fixtures/es-module-loaders/not-found-assert-loader.mjs +++ b/test/fixtures/es-module-loaders/not-found-assert-loader.mjs @@ -12,7 +12,7 @@ export async function resolve (specifier, base, defaultResolve) { await defaultResolve(specifier, base); } catch (e) { - assert.equal(e.code, 'MODULE_NOT_FOUND'); + assert.strictEqual(e.code, 'MODULE_NOT_FOUND'); return { format: 'builtin', url: 'fs' From 5e17f54d79a345cc66a2105201f25f38d6b8d9e3 Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Wed, 17 Jan 2018 20:36:25 -0500 Subject: [PATCH 12/24] test: refactor test-http-parser Use common's mustCall (for some reason was implementing its own?), and other small fixes. PR-URL: https://github.com/nodejs/node/pull/18219 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Anna Henningsen Reviewed-By: Ruben Bridgewater --- test/parallel/test-http-parser.js | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/test/parallel/test-http-parser.js b/test/parallel/test-http-parser.js index 81aadf26169..df3a87f73c8 100644 --- a/test/parallel/test-http-parser.js +++ b/test/parallel/test-http-parser.js @@ -20,15 +20,11 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. 'use strict'; -const common = require('../common'); +const { mustCall, mustNotCall } = require('../common'); const assert = require('assert'); -const binding = process.binding('http_parser'); -const methods = binding.methods; -const HTTPParser = binding.HTTPParser; - -const REQUEST = HTTPParser.REQUEST; -const RESPONSE = HTTPParser.RESPONSE; +const { methods, HTTPParser } = process.binding('http_parser'); +const { REQUEST, RESPONSE } = HTTPParser; const kOnHeaders = HTTPParser.kOnHeaders | 0; const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0; @@ -55,7 +51,7 @@ function newParser(type) { parser[kOnHeadersComplete] = function() { }; - parser[kOnBody] = common.mustNotCall('kOnBody should not be called'); + parser[kOnBody] = mustNotCall('kOnBody should not be called'); parser[kOnMessageComplete] = function() { }; @@ -64,21 +60,6 @@ function newParser(type) { } -function mustCall(f, times) { - let actual = 0; - - process.setMaxListeners(256); - process.on('exit', function() { - assert.strictEqual(actual, times || 1); - }); - - return function() { - actual++; - return f.apply(this, Array.prototype.slice.call(arguments)); - }; -} - - function expectBody(expected) { return mustCall(function(buf, start, len) { const body = String(buf.slice(start, start + len)); From 45307fde5c82488d1a12c1eeee24714d49745706 Mon Sep 17 00:00:00 2001 From: sreepurnajasti Date: Thu, 18 Jan 2018 15:13:45 +0530 Subject: [PATCH 13/24] lib: use american spelling as per style guide PR-URL: https://github.com/nodejs/node/pull/18226 Reviewed-By: Gireesh Punathil Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum Reviewed-By: Jon Moss Reviewed-By: Ruben Bridgewater --- lib/buffer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/buffer.js b/lib/buffer.js index 03f0cb3377e..75b8cedd1db 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -1229,7 +1229,7 @@ Buffer.prototype.readInt32BE = function readInt32BE(offset, noAssert) { // // An all-bits-one exponent is either a positive or negative infinity, if // the fraction is zero, or NaN when it is non-zero. The standard allows -// both quiet and signalling NaNs but since NaN is a canonical value in +// both quiet and signaling NaNs but since NaN is a canonical value in // JavaScript, we cannot (and do not) distinguish between the two. // // Other exponents are regular numbers and are computed by subtracting the bias From fe94394b99a39a4ed2c0589f825b6e4958fed629 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 17 Jan 2018 15:32:46 +0100 Subject: [PATCH 14/24] build: remove unused vars from configure PR-URL: https://github.com/nodejs/node/pull/18206 Reviewed-By: Colin Ihrig Reviewed-By: Richard Lau Reviewed-By: James M Snell Reviewed-By: Jon Moss Reviewed-By: Ruben Bridgewater --- configure | 5 ----- 1 file changed, 5 deletions(-) diff --git a/configure b/configure index 1029e92dfd5..db50ce0eedc 100755 --- a/configure +++ b/configure @@ -899,8 +899,6 @@ def configure_node(o): if options.systemtap_includes: o['include_dirs'] += [options.systemtap_includes] o['variables']['node_use_dtrace'] = b(use_dtrace) - o['variables']['uv_use_dtrace'] = b(use_dtrace) - o['variables']['uv_parent_path'] = '/deps/uv/' elif options.with_dtrace: raise Exception( 'DTrace is currently only supported on SunOS, MacOS or Linux systems.') @@ -977,7 +975,6 @@ def configure_node(o): o['variables']['library_files'] = options.linked_module o['variables']['asan'] = int(options.enable_asan or 0) - o['variables']['debug_devtools'] = 'node' if options.use_xcode and options.use_ninja: raise Exception('--xcode and --ninja cannot be used together.') @@ -1356,8 +1353,6 @@ def configure_intl(o): # this is the input '.dat' file to use .. icudt*.dat # may be little-endian if from a icu-project.org tarball o['variables']['icu_data_in'] = icu_data_in - # this is the icudt*.dat file which node will be using (platform endianness) - o['variables']['icu_data_file'] = icu_data_file if not os.path.isfile(icu_data_path): print('Error: ICU prebuilt data file %s does not exist.' % icu_data_path) print('See the README.md.') From ea7400c1bd61672090a2ca7e72512a8fa16ad79d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 17 Jan 2018 22:55:23 +0100 Subject: [PATCH 15/24] tty: fix console printing on Windows This broke writing non-ASCII data to the console on Windows because the result would be codepage-dependent. This partially reverts 8b751f7eb7b05a0b27f52e2288a636fdd78e9ecb. Fixes: https://github.com/nodejs/node/issues/18189 Refs: https://github.com/nodejs/node/pull/18019 PR-URL: https://github.com/nodejs/node/pull/18214 Fixes: https://github.com/nodejs/node/issues/18189 Refs: https://github.com/nodejs/node/pull/18019 Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- lib/tty.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/tty.js b/lib/tty.js index 28f2a411840..2a62fcb859a 100644 --- a/lib/tty.js +++ b/lib/tty.js @@ -24,7 +24,6 @@ const util = require('util'); const net = require('net'); const { TTY, isTTY } = process.binding('tty_wrap'); -const { makeSyncWrite } = require('internal/net'); const { inherits } = util; const errnoException = util._errnoException; const errors = require('internal/errors'); @@ -92,8 +91,6 @@ function WriteStream(fd) { // even though it was originally intended to change in v1.0.2 (Libuv 1.2.1). // Ref: https://github.com/nodejs/node/pull/1771#issuecomment-119351671 this._handle.setBlocking(true); - this._writev = null; - this._write = makeSyncWrite(fd); var winSize = new Array(2); var err = this._handle.getWindowSize(winSize); From 5f6478759b6bd25c927a1ceb53e78dc2db61dec6 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 17 Jan 2018 15:32:47 +0100 Subject: [PATCH 16/24] src: fix -Wunused-but-set-variable warnings PR-URL: https://github.com/nodejs/node/pull/18205 Reviewed-By: Anatoli Papirovski Reviewed-By: Evan Lucas Reviewed-By: Colin Ihrig Reviewed-By: Richard Lau Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Michael Dawson Reviewed-By: Ruben Bridgewater --- src/node_file.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index c8cdc795915..30bc4e1f380 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -516,7 +516,6 @@ static void InternalModuleStat(const FunctionCallbackInfo& args) { static void Stat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - Local context = env->context(); CHECK_GE(args.Length(), 1); @@ -540,7 +539,6 @@ static void Stat(const FunctionCallbackInfo& args) { static void LStat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - Local context = env->context(); CHECK_GE(args.Length(), 1); From 11a26e1b2e49c2b72ed0510d16b00722edcd0b0c Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 17 Jan 2018 15:32:47 +0100 Subject: [PATCH 17/24] src: fix -Wimplicit-fallthrough warning PR-URL: https://github.com/nodejs/node/pull/18205 Reviewed-By: Anatoli Papirovski Reviewed-By: Evan Lucas Reviewed-By: Colin Ihrig Reviewed-By: Richard Lau Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Michael Dawson Reviewed-By: Ruben Bridgewater --- src/node_i18n.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node_i18n.cc b/src/node_i18n.cc index 041eda94f3b..71ae6a00033 100644 --- a/src/node_i18n.cc +++ b/src/node_i18n.cc @@ -788,7 +788,8 @@ static int GetColumnWidth(UChar32 codepoint, if (ambiguous_as_full_width) { return 2; } - // Fall through if ambiguous_as_full_width if false. + // If ambiguous_as_full_width is false: + // Fall through case U_EA_NEUTRAL: if (u_hasBinaryProperty(codepoint, UCHAR_EMOJI_PRESENTATION)) { return 2; From 2a61ce5996cb0de47ea985b3c1da199872f36ddb Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Wed, 17 Jan 2018 12:22:59 +0530 Subject: [PATCH 18/24] src: validate args length in Access and Close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a follow-up of https://github.com/nodejs/node/pull/17914. When Access and Close functions are called in Sync mode, the number of items in args is validated. These are the only two places in this file where this validation doesn't take place. PR-URL: https://github.com/nodejs/node/pull/18203 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Joyee Cheung Reviewed-By: Daniel Bevenius Reviewed-By: Tobias Nießen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- src/node_file.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/node_file.cc b/src/node_file.cc index 30bc4e1f380..3edf09b6479 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -403,6 +403,7 @@ void Access(const FunctionCallbackInfo& args) { AsyncCall(env, args, "access", UTF8, AfterNoArgs, uv_fs_access, *path, mode); } else { // access(path, mode, undefined, ctx) + CHECK_EQ(args.Length(), 4); fs_req_wrap req_wrap; SyncCall(env, args[3], &req_wrap, "access", uv_fs_access, *path, mode); } @@ -424,6 +425,7 @@ void Close(const FunctionCallbackInfo& args) { AsyncCall(env, args, "close", UTF8, AfterNoArgs, uv_fs_close, fd); } else { // close(fd, undefined, ctx) + CHECK_EQ(args.Length(), 3); fs_req_wrap req_wrap; SyncCall(env, args[2], &req_wrap, "close", uv_fs_close, fd); } From 0a1adc061e827f09f0cd063f0117cef1221be69d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 16 Jan 2018 09:34:20 -0800 Subject: [PATCH 19/24] http2: add checks for server close callback Verify that server close callbacks are being called PR-URL: https://github.com/nodejs/node/pull/18182 Refs: https://github.com/nodejs/node/issues/18176 Reviewed-By: Anna Henningsen Reviewed-By: Ruben Bridgewater --- test/parallel/test-http2-create-client-secure-session.js | 3 ++- test/parallel/test-http2-create-client-session.js | 4 +++- test/parallel/test-http2-createwritereq.js | 2 +- test/parallel/test-http2-misbehaving-flow-control.js | 4 +++- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-http2-create-client-secure-session.js b/test/parallel/test-http2-create-client-secure-session.js index 6120a586020..b0111e15b69 100644 --- a/test/parallel/test-http2-create-client-secure-session.js +++ b/test/parallel/test-http2-create-client-secure-session.js @@ -38,6 +38,7 @@ function onStream(stream, headers) { function verifySecureSession(key, cert, ca, opts) { const server = h2.createSecureServer({ cert, key }); server.on('stream', common.mustCall(onStream)); + server.on('close', common.mustCall()); server.listen(0, common.mustCall(() => { opts = opts || { }; opts.secureContext = tls.createSecureContext({ ca }); @@ -72,7 +73,7 @@ function verifySecureSession(key, cert, ca, opts) { assert.strictEqual(jsonData.servername, opts.servername || 'localhost'); assert.strictEqual(jsonData.alpnProtocol, 'h2'); - server.close(); + server.close(common.mustCall()); client[kSocket].destroy(); })); })); diff --git a/test/parallel/test-http2-create-client-session.js b/test/parallel/test-http2-create-client-session.js index b5be6bc8581..963db2faa17 100644 --- a/test/parallel/test-http2-create-client-session.js +++ b/test/parallel/test-http2-create-client-session.js @@ -29,6 +29,8 @@ function onStream(stream, headers, flags) { stream.end(body.slice(20)); } +server.on('close', common.mustCall()); + server.listen(0); server.on('listening', common.mustCall(() => { @@ -46,7 +48,7 @@ server.on('listening', common.mustCall(() => { const countdown = new Countdown(count, () => { client.close(); - server.close(); + server.close(common.mustCall()); }); for (let n = 0; n < count; n++) { diff --git a/test/parallel/test-http2-createwritereq.js b/test/parallel/test-http2-createwritereq.js index 1d2b3167628..1575424d160 100644 --- a/test/parallel/test-http2-createwritereq.js +++ b/test/parallel/test-http2-createwritereq.js @@ -60,7 +60,7 @@ server.listen(0, common.mustCall(function() { testsFinished++; if (testsFinished === testsToRun) { - server.close(); + server.close(common.mustCall()); } })); diff --git a/test/parallel/test-http2-misbehaving-flow-control.js b/test/parallel/test-http2-misbehaving-flow-control.js index 8a0b411b8de..161a88ea1fb 100644 --- a/test/parallel/test-http2-misbehaving-flow-control.js +++ b/test/parallel/test-http2-misbehaving-flow-control.js @@ -72,7 +72,7 @@ server.on('stream', (stream) => { message: 'Stream closed with error code 3' })); stream.on('close', common.mustCall(() => { - server.close(); + server.close(common.mustCall()); client.destroy(); })); stream.resume(); @@ -80,6 +80,8 @@ server.on('stream', (stream) => { stream.end('ok'); }); +server.on('close', common.mustCall()); + server.listen(0, () => { client = net.connect(server.address().port, () => { client.write(preamble); From 3815f0c880624d15237d7fd79afe26299ea0b455 Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Mon, 15 Jan 2018 13:13:23 -0600 Subject: [PATCH 20/24] doc: cjs format is now commonjs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/18165 Reviewed-By: Guy Bedford Reviewed-By: Tobias Nießen Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- doc/api/esm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 20e582d30b8..0e86c381f1a 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -132,7 +132,7 @@ module. This can be one of the following: | `format` | Description | | --- | --- | | `"esm"` | Load a standard JavaScript module | -| `"cjs"` | Load a node-style CommonJS module | +| `"commonjs"` | Load a node-style CommonJS module | | `"builtin"` | Load a node builtin CommonJS module | | `"json"` | Load a JSON file | | `"addon"` | Load a [C++ Addon][addons] | From 36b13eb0e9e850fb8239327c23a2da864aabf6c9 Mon Sep 17 00:00:00 2001 From: Mars Wong Date: Tue, 9 Jan 2018 19:15:59 +0800 Subject: [PATCH 21/24] doc: improve process.platform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/18057 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil Reviewed-By: Luigi Pinca Reviewed-By: Rich Trott Reviewed-By: Tobias Nießen Reviewed-By: Ruben Bridgewater --- doc/api/process.md | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/doc/api/process.md b/doc/api/process.md index 44b48b4f4b6..405d6296353 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -1439,13 +1439,26 @@ added: v0.1.16 * {string} The `process.platform` property returns a string identifying the operating -system platform on which the Node.js process is running. For instance -`'darwin'`, `'freebsd'`, `'linux'`, `'sunos'` or `'win32'` +system platform on which the Node.js process is running. + +Currently possible values are: + +* `'aix'` +* `'darwin'` +* `'freebsd'` +* `'linux'` +* `'openbsd'` +* `'sunos'` +* `'win32'` ```js console.log(`This platform is ${process.platform}`); ``` +The value `'android'` may also be returned if the Node.js is built on the +Android operating system. However, Android support in Node.js +[is experimental][Supported platforms]. + ## process.ppid