From d327893193c68583e05f358929ab8008ed099776 Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Sun, 6 May 2018 13:05:21 +0530 Subject: [PATCH 001/208] doc: refactor mode constants parts in fs.md 1. removed extra mode constants doc. 2. creates bookmark to the common File Access Contants block. PR-URL: https://github.com/nodejs/node/pull/20558 Fixes: https://github.com/nodejs/node/issues/20049 Reviewed-By: Trivikram Kamat Reviewed-By: Vse Mozhet Byt --- doc/api/fs.md | 55 +++++++++++++++++---------------------------------- 1 file changed, 18 insertions(+), 37 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 30d6729d48c13d..286ffb3a63fb3a 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -746,17 +746,9 @@ changes: Tests a user's permissions for the file or directory specified by `path`. The `mode` argument is an optional integer that specifies the accessibility -checks to be performed. The following constants define the possible values of -`mode`. It is possible to create a mask consisting of the bitwise OR of two or -more values (e.g. `fs.constants.W_OK | fs.constants.R_OK`). - -* `fs.constants.F_OK` - `path` is visible to the calling process. This is useful -for determining if a file exists, but says nothing about `rwx` permissions. -Default if no `mode` is specified. -* `fs.constants.R_OK` - `path` can be read by the calling process. -* `fs.constants.W_OK` - `path` can be written by the calling process. -* `fs.constants.X_OK` - `path` can be executed by the calling process. This has -no effect on Windows (will behave like `fs.constants.F_OK`). +checks to be performed. Check [File Access Constants][] for possible values +of `mode`. It is possible to create a mask consisting of the bitwise OR of +two or more values (e.g. `fs.constants.W_OK | fs.constants.R_OK`). The final argument, `callback`, is a callback function that is invoked with a possible error argument. If any of the accessibility checks fail, the error @@ -889,19 +881,12 @@ changes: * `path` {string|Buffer|URL} * `mode` {integer} **Default:** `fs.constants.F_OK` -Synchronously tests a user's permissions for the file or directory specified by -`path`. The `mode` argument is an optional integer that specifies the -accessibility checks to be performed. The following constants define the -possible values of `mode`. It is possible to create a mask consisting of the -bitwise OR of two or more values (e.g. `fs.constants.W_OK | fs.constants.R_OK`). - -* `fs.constants.F_OK` - `path` is visible to the calling process. This is useful -for determining if a file exists, but says nothing about `rwx` permissions. -Default if no `mode` is specified. -* `fs.constants.R_OK` - `path` can be read by the calling process. -* `fs.constants.W_OK` - `path` can be written by the calling process. -* `fs.constants.X_OK` - `path` can be executed by the calling process. This has -no effect on Windows (will behave like `fs.constants.F_OK`). +Synchronously tests a user's permissions for the file or directory specified +by `path`. The `mode` argument is an optional integer that specifies the +accessibility checks to be performed. Check [File Access Constants][] for +possible values of `mode`. It is possible to create a mask consisting of +the bitwise OR of two or more values +(e.g. `fs.constants.W_OK | fs.constants.R_OK`). If any of the accessibility checks fail, an `Error` will be thrown. Otherwise, the method will return `undefined`. @@ -3679,17 +3664,9 @@ added: v10.0.0 Tests a user's permissions for the file or directory specified by `path`. The `mode` argument is an optional integer that specifies the accessibility -checks to be performed. The following constants define the possible values of -`mode`. It is possible to create a mask consisting of the bitwise OR of two or -more values (e.g. `fs.constants.W_OK | fs.constants.R_OK`). - -* `fs.constants.F_OK` - `path` is visible to the calling process. This is useful -for determining if a file exists, but says nothing about `rwx` permissions. -Default if no `mode` is specified. -* `fs.constants.R_OK` - `path` can be read by the calling process. -* `fs.constants.W_OK` - `path` can be written by the calling process. -* `fs.constants.X_OK` - `path` can be executed by the calling process. This has -no effect on Windows (will behave like `fs.constants.F_OK`). +checks to be performed. Check [File Access Constants][] for possible values +of `mode`. It is possible to create a mask consisting of the bitwise OR of +two or more values (e.g. `fs.constants.W_OK | fs.constants.R_OK`). If the accessibility check is successful, the `Promise` is resolved with no value. If any of the accessibility checks fail, the `Promise` is rejected @@ -4329,7 +4306,9 @@ The following constants are meant for use with [`fs.access()`][]. F_OK - Flag indicating that the file is visible to the calling process. + Flag indicating that the file is visible to the calling process. + This is useful for determining if a file exists, but says nothing + about rwx permissions. Default if no mode is specified. R_OK @@ -4343,7 +4322,8 @@ The following constants are meant for use with [`fs.access()`][]. X_OK Flag indicating that the file can be executed by the calling - process. + process. This has no effect on Windows + (will behave like fs.constants.F_OK). @@ -4706,3 +4686,4 @@ the file contents. [Naming Files, Paths, and Namespaces]: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx [MSDN-Using-Streams]: https://msdn.microsoft.com/en-us/library/windows/desktop/bb540537.aspx [support of file system `flags`]: #fs_file_system_flags +[File Access Constants]: #fs_file_access_constants From 4a7bb406fe7b4109786efd14700c0a7117e94803 Mon Sep 17 00:00:00 2001 From: Vse Mozhet Byt Date: Sun, 6 May 2018 03:59:23 +0300 Subject: [PATCH 002/208] doc, tools: unify stability signatures PR-URL: https://github.com/nodejs/node/pull/20552 Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca --- doc/api/documentation.md | 36 ++++++++++++++++++------------------ tools/doc/html.js | 3 +-- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/doc/api/documentation.md b/doc/api/documentation.md index cbc7a34be80e56..a5909e3268d947 100644 --- a/doc/api/documentation.md +++ b/doc/api/documentation.md @@ -34,24 +34,24 @@ and in the process of being redesigned. The stability indices are as follows: -```txt -Stability: 0 - Deprecated. This feature is known to be problematic, and changes -may be planned. Do not rely on it. Use of the feature may cause warnings to be -emitted. Backwards compatibility across major versions should not be expected. -``` - -```txt -Stability: 1 - Experimental. This feature is still under active development and -subject to non-backwards compatible changes, or even removal, in any future -version. Use of the feature is not recommended in production environments. -Experimental features are not subject to the Node.js Semantic Versioning model. -``` - -```txt -Stability: 2 - Stable. The API has proven satisfactory. Compatibility with the -npm ecosystem is a high priority, and will not be broken unless absolutely -necessary. -``` +> Stability: 0 - Deprecated. This feature is known to be problematic, and +> changes may be planned. Do not rely on it. Use of the feature may cause +> warnings to be emitted. Backwards compatibility across major versions should +> not be expected. + + + +> Stability: 1 - Experimental. This feature is still under active development +> and subject to non-backwards compatible changes, or even removal, in any +> future version. Use of the feature is not recommended in production +> environments. Experimental features are not subject to the Node.js Semantic +> Versioning model. + + + +> Stability: 2 - Stable. The API has proven satisfactory. Compatibility with the +> npm ecosystem is a high priority, and will not be broken unless absolutely +> necessary. Caution must be used when making use of `Experimental` features, particularly within modules that may be used as dependencies (or dependencies of diff --git a/tools/doc/html.js b/tools/doc/html.js index 7712290b4c61ed..00fd48d8e6631f 100644 --- a/tools/doc/html.js +++ b/tools/doc/html.js @@ -256,8 +256,7 @@ function preprocessElements(input) { state = null; return; } - if ((tok.type === 'paragraph' && state === 'MAYBE_STABILITY_BQ') || - tok.type === 'code') { + if (tok.type === 'paragraph' && state === 'MAYBE_STABILITY_BQ') { if (tok.text.match(/Stability:.*/g)) { const stabilityMatch = tok.text.match(STABILITY_TEXT_REG_EXP); const stability = Number(stabilityMatch[2]); From b3b267a87ca9fa146faefe396754e9469b2480e5 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Tue, 8 May 2018 17:59:14 +0530 Subject: [PATCH 003/208] doc: add params for ClientHttp2Session:altsvc Add parameters for the callback for the ClientHttp2Session:altsvc event inline with the pattern in the rest of the documentation. Refs: https://github.com/nodejs/help/issues/877#issuecomment-381253464 PR-URL: https://github.com/nodejs/node/pull/20598 Reviewed-By: Vse Mozhet Byt Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Trivikram Kamat Reviewed-By: Colin Ihrig --- doc/api/http2.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index d737471e8670ee..4f668f5dd15d69 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -664,9 +664,9 @@ added: v8.4.0 added: v9.4.0 --> -* `alt`: {string} -* `origin`: {string} -* `streamId`: {number} +* `alt` {string} +* `origin` {string} +* `streamId` {number} The `'altsvc'` event is emitted whenever an `ALTSVC` frame is received by the client. The event is emitted with the `ALTSVC` value, origin, and stream From 88bc6da6e9b07ed0bb28e31b95f675da0124f326 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Wed, 9 May 2018 01:16:57 +0530 Subject: [PATCH 004/208] doc: add parameters for Http2Stream:error event Add parameters for the callback for the Http2Stream:error event inline with the pattern in the rest of the documentation. Refs: https://github.com/nodejs/help/issues/877#issuecomment-381253464 PR-URL: https://github.com/nodejs/node/pull/20610 Reviewed-By: Vse Mozhet Byt Reviewed-By: Trivikram Kamat Reviewed-By: Colin Ihrig --- doc/api/http2.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/api/http2.md b/doc/api/http2.md index 4f668f5dd15d69..560e183dca6da1 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -846,6 +846,8 @@ the `http2stream.rstCode` property. If the code is any value other than added: v8.4.0 --> +* `error` {Error} + The `'error'` event is emitted when an error occurs during the processing of an `Http2Stream`. From 9177f734e3aa7557bfe8b85d6f4c68e4649be683 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 8 May 2018 13:17:24 +0200 Subject: [PATCH 005/208] doc: update VM section text This commit updates the VM section with suggestion for a minor improvement (hopefully) of the text. PR-URL: https://github.com/nodejs/node/pull/20595 Reviewed-By: Vse Mozhet Byt Reviewed-By: Gus Caplan Reviewed-By: James M Snell Reviewed-By: Rich Trott Reviewed-By: Luigi Pinca --- doc/api/vm.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/vm.md b/doc/api/vm.md index 9e1249dc4ed8bb..cc9b3135381dad 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -17,8 +17,8 @@ The sandboxed code uses a different V8 Context, meaning that it has a different global object than the rest of the code. One can provide the context by ["contextifying"][contextified] a sandbox -object. The sandboxed code treats any property on the sandbox like a -global variable. Any changes on global variables caused by the sandboxed +object. The sandboxed code treats any property in the sandbox like a +global variable. Any changes to global variables caused by the sandboxed code are reflected in the sandbox object. ```js From e01e06076342101e2e9a32e5ffbd607f7da03fd8 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Mon, 7 May 2018 10:24:42 +0200 Subject: [PATCH 006/208] src: rename handle parameter object This commit renames the handle parameter for the BaseObject constructor to object instead of handle. The motivation for doing this is that when stepping through an inheritance chain it can sometimes be a little confusing when HandleWrap is in involved. HandleWrap has a handle parameter but calls the object that is passed to AsyncWrap object, but then when you end up in BaseObject it is named handle. PR-URL: https://github.com/nodejs/node/pull/20570 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- src/base_object-inl.h | 10 +++++----- src/base_object.h | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 11ba1c88da0486..786e1f26b48256 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -31,12 +31,12 @@ namespace node { -BaseObject::BaseObject(Environment* env, v8::Local handle) - : persistent_handle_(env->isolate(), handle), +BaseObject::BaseObject(Environment* env, v8::Local object) + : persistent_handle_(env->isolate(), object), env_(env) { - CHECK_EQ(false, handle.IsEmpty()); - CHECK_GT(handle->InternalFieldCount(), 0); - handle->SetAlignedPointerInInternalField(0, static_cast(this)); + CHECK_EQ(false, object.IsEmpty()); + CHECK_GT(object->InternalFieldCount(), 0); + object->SetAlignedPointerInInternalField(0, static_cast(this)); } diff --git a/src/base_object.h b/src/base_object.h index 7d8281238b1c1d..2a4967c1aaf303 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -34,9 +34,9 @@ class Environment; class BaseObject { public: - // Associates this object with `handle`. It uses the 0th internal field for + // Associates this object with `object`. It uses the 0th internal field for // that, and in particular aborts if there is no such field. - inline BaseObject(Environment* env, v8::Local handle); + inline BaseObject(Environment* env, v8::Local object); virtual inline ~BaseObject(); // Returns the wrapped object. Returns an empty handle when From 983cb269e0ab74e152ef4ee562fbd512d2636f94 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Mon, 7 May 2018 14:06:56 +0200 Subject: [PATCH 007/208] src: don't create Undefined if not needed This commit moves the creation of argv and only creates an undefined value if the passed in status was not 0. The variable name client_handle was already used in this function but I've change that usage so that this variable name matches the onconnection callback functions parameter name clientHandle. PR-URL: https://github.com/nodejs/node/pull/20573 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Gus Caplan Reviewed-By: Anna Henningsen --- src/connection_wrap.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/connection_wrap.cc b/src/connection_wrap.cc index e0052a643b8734..01d0388d4de3ce 100644 --- a/src/connection_wrap.cc +++ b/src/connection_wrap.cc @@ -45,10 +45,7 @@ void ConnectionWrap::OnConnection(uv_stream_t* handle, // uv_close() on the handle. CHECK_EQ(wrap_data->persistent().IsEmpty(), false); - Local argv[] = { - Integer::New(env->isolate(), status), - Undefined(env->isolate()) - }; + Local client_handle; if (status == 0) { // Instantiate the client javascript object and handle. @@ -59,17 +56,20 @@ void ConnectionWrap::OnConnection(uv_stream_t* handle, // Unwrap the client javascript object. WrapType* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, client_obj); - uv_stream_t* client_handle = - reinterpret_cast(&wrap->handle_); + uv_stream_t* client = reinterpret_cast(&wrap->handle_); // uv_accept can fail if the new connection has already been closed, in // which case an EAGAIN (resource temporarily unavailable) will be // returned. - if (uv_accept(handle, client_handle)) + if (uv_accept(handle, client)) return; // Successful accept. Call the onconnection callback in JavaScript land. - argv[1] = client_obj; + client_handle = client_obj; + } else { + client_handle = Undefined(env->isolate()); } + + Local argv[] = { Integer::New(env->isolate(), status), client_handle }; wrap_data->MakeCallback(env->onconnection_string(), arraysize(argv), argv); } From 1f34c04bd08b67981db6534fb357fafb1484e068 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Mon, 7 May 2018 15:37:57 +0200 Subject: [PATCH 008/208] net: remove typo in setTimeout comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit removes `lear` from the code comment in setTimeout. I'm not 100% sure this is a typo but I've struggled to think what it could mean. Hopefully someone else might be able to shed some light on this. PR-URL: https://github.com/nodejs/node/pull/20576 Reviewed-By: James M Snell Reviewed-By: Jeremiah Senkpiel Reviewed-By: Ruben Bridgewater Reviewed-By: Tobias Nießen Reviewed-By: Anna Henningsen Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- lib/net.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/net.js b/lib/net.js index 6c475619117dc5..b4936558a82790 100644 --- a/lib/net.js +++ b/lib/net.js @@ -411,7 +411,7 @@ Socket.prototype.setTimeout = function(msecs, callback) { // Type checking identical to timers.enroll() msecs = validateTimerDuration(msecs); - // Attempt to clear an existing timer lear in both cases - + // Attempt to clear an existing timer in both cases - // even if it will be rescheduled we don't want to leak an existing timer. clearTimeout(this[kTimeout]); From 105f6062024fd5c5604d1d5253a72c53b1f5c529 Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Mon, 7 May 2018 08:36:43 -0500 Subject: [PATCH 009/208] v8: backport 9fb02b526f1cd3b859a530a01adb08bc0d089f4f Refs: https://github.com/v8/v8/commit/9fb02b526f1cd3b859a530a01adb08bc0d089f4f Original commit message: Allow function callbacks to have Proxy as receiver. R=verwaest@chromium.org Bug: v8:5773 Change-Id: Ifd29a1116ee8c86b8d8d24485bbfd19e260ab66b Reviewed-on: chromium-review.googlesource.com/1046088 Commit-Queue: Yang Guo Reviewed-by: Camillo Bruni Cr-Commit-Position: refs/heads/master@{#53015} PR-URL: https://github.com/nodejs/node/pull/20575 Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis --- common.gypi | 2 +- deps/v8/src/builtins/builtins-api.cc | 41 ++++++++++++++-------------- deps/v8/test/cctest/test-api.cc | 28 +++++++++++++++++++ 3 files changed, 50 insertions(+), 21 deletions(-) diff --git a/common.gypi b/common.gypi index 3bb10d611fb1d5..adeaf85c12bac1 100644 --- a/common.gypi +++ b/common.gypi @@ -27,7 +27,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.6', + 'v8_embedder_string': '-node.7', # Enable disassembler for `--print-code` v8 options 'v8_enable_disassembler': 1, diff --git a/deps/v8/src/builtins/builtins-api.cc b/deps/v8/src/builtins/builtins-api.cc index 971fb7c678499f..bb66b082f3edf3 100644 --- a/deps/v8/src/builtins/builtins-api.cc +++ b/deps/v8/src/builtins/builtins-api.cc @@ -21,17 +21,21 @@ namespace { // Returns the holder JSObject if the function can legally be called with this // receiver. Returns nullptr if the call is illegal. // TODO(dcarney): CallOptimization duplicates this logic, merge. -JSObject* GetCompatibleReceiver(Isolate* isolate, FunctionTemplateInfo* info, - JSObject* receiver) { +JSReceiver* GetCompatibleReceiver(Isolate* isolate, FunctionTemplateInfo* info, + JSReceiver* receiver) { Object* recv_type = info->signature(); // No signature, return holder. if (!recv_type->IsFunctionTemplateInfo()) return receiver; + // A Proxy cannot have been created from the signature template. + if (!receiver->IsJSObject()) return nullptr; + + JSObject* js_obj_receiver = JSObject::cast(receiver); FunctionTemplateInfo* signature = FunctionTemplateInfo::cast(recv_type); // Check the receiver. Fast path for receivers with no hidden prototypes. - if (signature->IsTemplateFor(receiver)) return receiver; - if (!receiver->map()->has_hidden_prototype()) return nullptr; - for (PrototypeIterator iter(isolate, receiver, kStartAtPrototype, + if (signature->IsTemplateFor(js_obj_receiver)) return receiver; + if (!js_obj_receiver->map()->has_hidden_prototype()) return nullptr; + for (PrototypeIterator iter(isolate, js_obj_receiver, kStartAtPrototype, PrototypeIterator::END_AT_NON_HIDDEN); !iter.IsAtEnd(); iter.Advance()) { JSObject* current = iter.GetCurrent(); @@ -45,8 +49,8 @@ MUST_USE_RESULT MaybeHandle HandleApiCallHelper( Isolate* isolate, Handle function, Handle new_target, Handle fun_data, Handle receiver, BuiltinArguments args) { - Handle js_receiver; - JSObject* raw_holder; + Handle js_receiver; + JSReceiver* raw_holder; if (is_construct) { DCHECK(args.receiver()->IsTheHole(isolate)); if (fun_data->instance_template()->IsUndefined(isolate)) { @@ -68,21 +72,18 @@ MUST_USE_RESULT MaybeHandle HandleApiCallHelper( raw_holder = *js_receiver; } else { DCHECK(receiver->IsJSReceiver()); - - if (!receiver->IsJSObject()) { - // This function cannot be called with the given receiver. Abort! - THROW_NEW_ERROR( - isolate, NewTypeError(MessageTemplate::kIllegalInvocation), Object); - } - - js_receiver = Handle::cast(receiver); + js_receiver = Handle::cast(receiver); if (!fun_data->accept_any_receiver() && - js_receiver->IsAccessCheckNeeded() && - !isolate->MayAccess(handle(isolate->context()), js_receiver)) { - isolate->ReportFailedAccessCheck(js_receiver); - RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object); - return isolate->factory()->undefined_value(); + js_receiver->IsAccessCheckNeeded()) { + // Proxies never need access checks. + DCHECK(js_receiver->IsJSObject()); + Handle js_obj_receiver = Handle::cast(js_receiver); + if (!isolate->MayAccess(handle(isolate->context()), js_obj_receiver)) { + isolate->ReportFailedAccessCheck(js_obj_receiver); + RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object); + return isolate->factory()->undefined_value(); + } } raw_holder = GetCompatibleReceiver(isolate, *fun_data, *js_receiver); diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc index fe1aca5a0fb440..40ba60ffb36607 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -1088,6 +1088,34 @@ THREADED_PROFILED_TEST(FunctionTemplate) { TestFunctionTemplateAccessor(construct_callback, Return239Callback); } +static void FunctionCallbackForProxyTest( + const v8::FunctionCallbackInfo& info) { + info.GetReturnValue().Set(info.This()); +} + +THREADED_TEST(FunctionTemplateWithProxy) { + LocalContext env; + v8::Isolate* isolate = env->GetIsolate(); + v8::HandleScope scope(isolate); + + v8::Local function_template = + v8::FunctionTemplate::New(isolate, FunctionCallbackForProxyTest); + v8::Local function = + function_template->GetFunction(env.local()).ToLocalChecked(); + CHECK((*env)->Global()->Set(env.local(), v8_str("f"), function).FromJust()); + v8::Local proxy = + CompileRun("var proxy = new Proxy({}, {}); proxy"); + CHECK(proxy->IsProxy()); + + v8::Local result = CompileRun("f(proxy)"); + CHECK(result->Equals(env.local(), (*env)->Global()).FromJust()); + + result = CompileRun("f.call(proxy)"); + CHECK(result->Equals(env.local(), proxy).FromJust()); + + result = CompileRun("Reflect.apply(f, proxy, [1])"); + CHECK(result->Equals(env.local(), proxy).FromJust()); +} static void SimpleCallback(const v8::FunctionCallbackInfo& info) { ApiTestFuzzer::Fuzz(); From 5803973206af7c60c6cbc186afdc38d5c8fae65f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 5 May 2018 19:25:07 +0200 Subject: [PATCH 010/208] src: minor refactor to string_search.h - Use `std::max` instead of a custom variant - Use member method pointers to avoid an extra layer of indirection - Stop transferring `Vector` into the `node` namespace PR-URL: https://github.com/nodejs/node/pull/20546 Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius Reviewed-By: Tiancheng "Timothy" Gu --- src/string_search.h | 145 +++++++++++++++++--------------------------- 1 file changed, 54 insertions(+), 91 deletions(-) diff --git a/src/string_search.h b/src/string_search.h index 51f72062a99466..6a45eb366623d9 100644 --- a/src/string_search.h +++ b/src/string_search.h @@ -9,18 +9,11 @@ #include "node_internals.h" #include +#include namespace node { namespace stringsearch { - -// Returns the maximum of the two parameters. -template -T Max(T a, T b) { - return a < b ? b : a; -} - - static const uint32_t kMaxOneByteCharCodeU = 0xff; template @@ -98,7 +91,9 @@ class StringSearchBase { template class StringSearch : private StringSearchBase { public: - explicit StringSearch(Vector pattern) + typedef stringsearch::Vector Vector; + + explicit StringSearch(Vector pattern) : pattern_(pattern), start_(0) { if (pattern.length() >= kBMMaxShift) { start_ = pattern.length() - kBMMaxShift; @@ -108,17 +103,17 @@ class StringSearch : private StringSearchBase { CHECK_GT(pattern_length, 0); if (pattern_length < kBMMinPatternLength) { if (pattern_length == 1) { - strategy_ = &SingleCharSearch; + strategy_ = &StringSearch::SingleCharSearch; return; } - strategy_ = &LinearSearch; + strategy_ = &StringSearch::LinearSearch; return; } - strategy_ = &InitialSearch; + strategy_ = &StringSearch::InitialSearch; } - size_t Search(Vector subject, size_t index) { - return strategy_(this, subject, index); + size_t Search(Vector subject, size_t index) { + return (this->*strategy_)(subject, index); } static inline int AlphabetSize() { @@ -136,31 +131,12 @@ class StringSearch : private StringSearchBase { } private: - typedef size_t (*SearchFunction)( - StringSearch*, - Vector, - size_t); - - static size_t SingleCharSearch(StringSearch* search, - Vector subject, - size_t start_index); - - static size_t LinearSearch(StringSearch* search, - Vector subject, - size_t start_index); - - static size_t InitialSearch(StringSearch* search, - Vector subject, - size_t start_index); - - static size_t BoyerMooreHorspoolSearch( - StringSearch* search, - Vector subject, - size_t start_index); - - static size_t BoyerMooreSearch(StringSearch* search, - Vector subject, - size_t start_index); + typedef size_t (StringSearch::*SearchFunction)(Vector, size_t); + size_t SingleCharSearch(Vector subject, size_t start_index); + size_t LinearSearch(Vector subject, size_t start_index); + size_t InitialSearch(Vector subject, size_t start_index); + size_t BoyerMooreHorspoolSearch(Vector subject, size_t start_index); + size_t BoyerMooreSearch(Vector subject, size_t start_index); void PopulateBoyerMooreHorspoolTable(); @@ -197,7 +173,7 @@ class StringSearch : private StringSearchBase { } // The pattern to search for. - Vector pattern_; + Vector pattern_; // Pointer to implementation of the search. SearchFunction strategy_; // Cache value of Max(0, pattern_length() - kBMMaxShift) @@ -213,8 +189,8 @@ inline T AlignDown(T value, U alignment) { inline uint8_t GetHighestValueByte(uint16_t character) { - return Max(static_cast(character & 0xFF), - static_cast(character >> 8)); + return std::max(static_cast(character & 0xFF), + static_cast(character >> 8)); } @@ -319,11 +295,10 @@ inline size_t FindFirstCharacter(Vector pattern, template size_t StringSearch::SingleCharSearch( - StringSearch* search, - Vector subject, + Vector subject, size_t index) { - CHECK_EQ(1, search->pattern_.length()); - return FindFirstCharacter(search->pattern_, subject, index); + CHECK_EQ(1, pattern_.length()); + return FindFirstCharacter(pattern_, subject, index); } //--------------------------------------------------------------------- @@ -333,22 +308,19 @@ size_t StringSearch::SingleCharSearch( // Simple linear search for short patterns. Never bails out. template size_t StringSearch::LinearSearch( - StringSearch* search, - Vector subject, + Vector subject, size_t index) { - Vector pattern = search->pattern_; - CHECK_GT(pattern.length(), 1); - const size_t pattern_length = pattern.length(); - const size_t n = subject.length() - pattern_length; + CHECK_GT(pattern_.length(), 1); + const size_t n = subject.length() - pattern_.length(); for (size_t i = index; i <= n; i++) { - i = FindFirstCharacter(pattern, subject, i); + i = FindFirstCharacter(pattern_, subject, i); if (i == subject.length()) return subject.length(); CHECK_LE(i, n); bool matches = true; - for (size_t j = 1; j < pattern_length; j++) { - if (pattern[j] != subject[i + j]) { + for (size_t j = 1; j < pattern_.length(); j++) { + if (pattern_[j] != subject[i + j]) { matches = false; break; } @@ -366,19 +338,17 @@ size_t StringSearch::LinearSearch( template size_t StringSearch::BoyerMooreSearch( - StringSearch* search, - Vector subject, + Vector subject, size_t start_index) { - Vector pattern = search->pattern_; const size_t subject_length = subject.length(); - const size_t pattern_length = pattern.length(); + const size_t pattern_length = pattern_.length(); // Only preprocess at most kBMMaxShift last characters of pattern. - size_t start = search->start_; + size_t start = start_; - int* bad_char_occurrence = search->bad_char_table(); - int* good_suffix_shift = search->good_suffix_shift_table(); + int* bad_char_occurrence = bad_char_table(); + int* good_suffix_shift = good_suffix_shift_table(); - Char last_char = pattern[pattern_length - 1]; + Char last_char = pattern_[pattern_length - 1]; size_t index = start_index; // Continue search from i. while (index <= subject_length - pattern_length) { @@ -391,7 +361,7 @@ size_t StringSearch::BoyerMooreSearch( return subject.length(); } } - while (pattern[j] == (c = subject[index + j])) { + while (pattern_[j] == (c = subject[index + j])) { if (j == 0) { return index; } @@ -420,7 +390,6 @@ size_t StringSearch::BoyerMooreSearch( template void StringSearch::PopulateBoyerMooreTable() { const size_t pattern_length = pattern_.length(); - Vector pattern = pattern_; // Only look at the last kBMMaxShift characters of pattern (from start_ // to pattern_length). const size_t start = start_; @@ -448,8 +417,8 @@ void StringSearch::PopulateBoyerMooreTable() { { size_t i = pattern_length; while (i > start) { - Char c = pattern[i - 1]; - while (suffix <= pattern_length && c != pattern[suffix - 1]) { + Char c = pattern_[i - 1]; + while (suffix <= pattern_length && c != pattern_[suffix - 1]) { if (static_cast(shift_table[suffix]) == length) { shift_table[suffix] = suffix - i; } @@ -458,7 +427,7 @@ void StringSearch::PopulateBoyerMooreTable() { suffix_table[--i] = --suffix; if (suffix == pattern_length) { // No suffix to extend, so we check against last_char only. - while ((i > start) && (pattern[i - 1] != last_char)) { + while ((i > start) && (pattern_[i - 1] != last_char)) { if (static_cast(shift_table[pattern_length]) == length) { shift_table[pattern_length] = pattern_length - i; } @@ -489,17 +458,15 @@ void StringSearch::PopulateBoyerMooreTable() { template size_t StringSearch::BoyerMooreHorspoolSearch( - StringSearch* search, - Vector subject, + Vector subject, size_t start_index) { - Vector pattern = search->pattern_; const size_t subject_length = subject.length(); - const size_t pattern_length = pattern.length(); - int* char_occurrences = search->bad_char_table(); + const size_t pattern_length = pattern_.length(); + int* char_occurrences = bad_char_table(); int64_t badness = -pattern_length; // How bad we are doing without a good-suffix table. - Char last_char = pattern[pattern_length - 1]; + Char last_char = pattern_[pattern_length - 1]; int last_char_shift = pattern_length - 1 - CharOccurrence(char_occurrences, static_cast(last_char)); @@ -519,7 +486,7 @@ size_t StringSearch::BoyerMooreHorspoolSearch( } } j--; - while (pattern[j] == (subject[index + j])) { + while (pattern_[j] == (subject[index + j])) { if (j == 0) { return index; } @@ -532,9 +499,9 @@ size_t StringSearch::BoyerMooreHorspoolSearch( // compared to reading each character exactly once. badness += (pattern_length - j) - last_char_shift; if (badness > 0) { - search->PopulateBoyerMooreTable(); - search->strategy_ = &BoyerMooreSearch; - return BoyerMooreSearch(search, subject, index); + PopulateBoyerMooreTable(); + strategy_ = &StringSearch::BoyerMooreSearch; + return BoyerMooreSearch(subject, index); } } return subject.length(); @@ -575,11 +542,9 @@ void StringSearch::PopulateBoyerMooreHorspoolTable() { // isn't found very early in the subject. Upgrades to BoyerMooreHorspool. template size_t StringSearch::InitialSearch( - StringSearch* search, - Vector subject, + Vector subject, size_t index) { - Vector pattern = search->pattern_; - const size_t pattern_length = pattern.length(); + const size_t pattern_length = pattern_.length(); // Badness is a count of how much work we have done. When we have // done enough work we decide it's probably worth switching to a better // algorithm. @@ -590,13 +555,13 @@ size_t StringSearch::InitialSearch( for (size_t i = index, n = subject.length() - pattern_length; i <= n; i++) { badness++; if (badness <= 0) { - i = FindFirstCharacter(pattern, subject, i); + i = FindFirstCharacter(pattern_, subject, i); if (i == subject.length()) return subject.length(); CHECK_LE(i, n); size_t j = 1; do { - if (pattern[j] != subject[i + j]) { + if (pattern_[j] != subject[i + j]) { break; } j++; @@ -606,9 +571,9 @@ size_t StringSearch::InitialSearch( } badness += j; } else { - search->PopulateBoyerMooreHorspoolTable(); - search->strategy_ = &BoyerMooreHorspoolSearch; - return BoyerMooreHorspoolSearch(search, subject, i); + PopulateBoyerMooreHorspoolTable(); + strategy_ = &StringSearch::BoyerMooreHorspoolSearch; + return BoyerMooreHorspoolSearch(subject, i); } } return subject.length(); @@ -629,7 +594,6 @@ size_t SearchString(Vector subject, } // namespace node namespace node { -using node::stringsearch::Vector; template size_t SearchString(const Char* haystack, @@ -643,9 +607,8 @@ size_t SearchString(const Char* haystack, // code, create two vectors that are reversed views into the input strings. // For example, v_needle[0] would return the *last* character of the needle. // So we're searching for the first instance of rev(needle) in rev(haystack) - Vector v_needle = Vector( - needle, needle_length, is_forward); - Vector v_haystack = Vector( + stringsearch::Vector v_needle(needle, needle_length, is_forward); + stringsearch::Vector v_haystack( haystack, haystack_length, is_forward); size_t diff = haystack_length - needle_length; size_t relative_start_index; From 2df99ac095f056350cf8b954059a6068b6ef41c8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 4 Sep 2017 22:31:20 +0200 Subject: [PATCH 011/208] src: use lock for c-ares library init/cleanup This helps embedders wishing to use Node.js in a multi-threaded fashion and helps pave the way for thread-based worker support. Thanks to Stephen Belanger for reviewing this commit in its original PR. Refs: https://github.com/ayojs/ayo/pull/82 PR-URL: https://github.com/nodejs/node/pull/20539 Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Colin Ihrig Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius --- src/cares_wrap.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 4df47d75d43ba8..4208c02d4fe445 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -70,6 +70,8 @@ using v8::Value; namespace { +Mutex ares_library_mutex; + inline uint16_t cares_get_16bit(const unsigned char* p) { return static_cast(p[0] << 8U) | (static_cast(p[1])); } @@ -470,6 +472,7 @@ void ChannelWrap::Setup() { int r; if (!library_inited_) { + Mutex::ScopedLock lock(ares_library_mutex); // Multiple calls to ares_library_init() increase a reference counter, // so this is a no-op except for the first call to it. r = ares_library_init(ARES_LIB_INIT_ALL); @@ -483,6 +486,7 @@ void ChannelWrap::Setup() { ARES_OPT_FLAGS | ARES_OPT_SOCK_STATE_CB); if (r != ARES_SUCCESS) { + Mutex::ScopedLock lock(ares_library_mutex); ares_library_cleanup(); return env()->ThrowError(ToErrorCodeString(r)); } @@ -500,6 +504,7 @@ void ChannelWrap::Setup() { ChannelWrap::~ChannelWrap() { if (library_inited_) { + Mutex::ScopedLock lock(ares_library_mutex); // This decreases the reference counter increased by ares_library_init(). ares_library_cleanup(); } From a89cc2886ecec5bd9b8b3f803a6658afb90e21b2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 5 May 2018 17:47:02 +0200 Subject: [PATCH 012/208] src: protect global state with mutexes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Protect environment variables and inherently per-process state with mutexes, to better accommodate Node’s usage in multi-threading environments. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: https://github.com/ayojs/ayo/pull/82 PR-URL: https://github.com/nodejs/node/pull/20542 Reviewed-By: Gus Caplan Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis --- src/node.cc | 22 ++++++++++++++++++---- src/node_crypto.cc | 3 +++ src/node_internals.h | 1 + 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/node.cc b/src/node.cc index 693457d3ed0aae..704bdc334b2f9c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -171,6 +171,9 @@ using v8::Value; using AsyncHooks = Environment::AsyncHooks; +static Mutex process_mutex; +static Mutex environ_mutex; + static bool print_eval = false; static bool force_repl = false; static bool syntax_check_only = false; @@ -698,9 +701,12 @@ bool SafeGetenv(const char* key, std::string* text) { goto fail; #endif - if (const char* value = getenv(key)) { - *text = value; - return true; + { + Mutex::ScopedLock lock(environ_mutex); + if (const char* value = getenv(key)) { + *text = value; + return true; + } } fail: @@ -1358,6 +1364,7 @@ void AppendExceptionLine(Environment* env, if (!can_set_arrow || (mode == FATAL_ERROR && !err_obj->IsNativeError())) { if (env->printed_error()) return; + Mutex::ScopedLock lock(process_mutex); env->set_printed_error(true); uv_tty_reset_mode(); @@ -2624,7 +2631,6 @@ static void ProcessTitleSetter(Local property, Local value, const PropertyCallbackInfo& info) { node::Utf8Value title(info.GetIsolate(), value); - // TODO(piscisaureus): protect with a lock uv_set_process_title(*title); } @@ -2635,6 +2641,7 @@ static void EnvGetter(Local property, if (property->IsSymbol()) { return info.GetReturnValue().SetUndefined(); } + Mutex::ScopedLock lock(environ_mutex); #ifdef __POSIX__ node::Utf8Value key(isolate, property); const char* val = getenv(*key); @@ -2675,6 +2682,8 @@ static void EnvSetter(Local property, "DEP0104").IsNothing()) return; } + + Mutex::ScopedLock lock(environ_mutex); #ifdef __POSIX__ node::Utf8Value key(info.GetIsolate(), property); node::Utf8Value val(info.GetIsolate(), value); @@ -2695,6 +2704,7 @@ static void EnvSetter(Local property, static void EnvQuery(Local property, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(environ_mutex); int32_t rc = -1; // Not found unless proven otherwise. if (property->IsString()) { #ifdef __POSIX__ @@ -2724,6 +2734,7 @@ static void EnvQuery(Local property, static void EnvDeleter(Local property, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(environ_mutex); if (property->IsString()) { #ifdef __POSIX__ node::Utf8Value key(info.GetIsolate(), property); @@ -2749,6 +2760,7 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { Local argv[NODE_PUSH_VAL_TO_ARRAY_MAX]; size_t idx = 0; + Mutex::ScopedLock lock(environ_mutex); #ifdef __POSIX__ int size = 0; while (environ[size]) @@ -2864,6 +2876,7 @@ static Local GetFeatures(Environment* env) { static void DebugPortGetter(Local property, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(process_mutex); int port = debug_options.port(); #if HAVE_INSPECTOR if (port == 0) { @@ -2879,6 +2892,7 @@ static void DebugPortGetter(Local property, static void DebugPortSetter(Local property, Local value, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(process_mutex); debug_options.set_port(value->Int32Value()); } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d5f27994307696..4d07546e0a213e 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -715,6 +715,9 @@ void SecureContext::SetCert(const FunctionCallbackInfo& args) { static X509_STORE* NewRootCertStore() { static std::vector root_certs_vector; + static Mutex root_certs_vector_mutex; + Mutex::ScopedLock lock(root_certs_vector_mutex); + if (root_certs_vector.empty()) { for (size_t i = 0; i < arraysize(root_certs); i++) { BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i])); diff --git a/src/node_internals.h b/src/node_internals.h index e5ea575ebc5981..e15df78ffdfee3 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -25,6 +25,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "node.h" +#include "node_mutex.h" #include "node_persistent.h" #include "util-inl.h" #include "env-inl.h" From 4873fbaf63200ac39202a1344b62faf10f1982d6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 5 May 2018 18:27:50 +0200 Subject: [PATCH 013/208] src: remove unused freelist.h header Always easy enough to re-introduce if we do need it. PR-URL: https://github.com/nodejs/node/pull/20544 Reviewed-By: Colin Ihrig Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius Reviewed-By: Matheus Marchini Reviewed-By: Tiancheng "Timothy" Gu --- src/freelist.h | 92 -------------------------------------------------- 1 file changed, 92 deletions(-) delete mode 100644 src/freelist.h diff --git a/src/freelist.h b/src/freelist.h deleted file mode 100644 index 7dff56a35d348a..00000000000000 --- a/src/freelist.h +++ /dev/null @@ -1,92 +0,0 @@ -#ifndef SRC_FREELIST_H_ -#define SRC_FREELIST_H_ - -#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS - -#include "util.h" - -namespace node { - -struct DefaultFreelistTraits; - -template -class Freelist { - public: - typedef struct list_item { - T* item = nullptr; - list_item* next = nullptr; - } list_item; - - Freelist() {} - ~Freelist() { - while (head_ != nullptr) { - list_item* item = head_; - head_ = item->next; - FreelistTraits::Free(item->item); - free(item); - } - } - - void push(T* item) { - if (size_ > kMaximumLength) { - FreelistTraits::Free(item); - } else { - size_++; - FreelistTraits::Reset(item); - list_item* li = Calloc(1); - li->item = item; - if (head_ == nullptr) { - head_ = li; - tail_ = li; - } else { - tail_->next = li; - tail_ = li; - } - } - } - - T* pop() { - if (head_ != nullptr) { - size_--; - list_item* cur = head_; - T* item = cur->item; - head_ = cur->next; - free(cur); - return item; - } else { - return FreelistTraits::template Alloc(); - } - } - - private: - size_t size_ = 0; - list_item* head_ = nullptr; - list_item* tail_ = nullptr; -}; - -struct DefaultFreelistTraits { - template - static T* Alloc() { - return ::new (Malloc(1)) T(); - } - - template - static void Free(T* item) { - item->~T(); - free(item); - } - - template - static void Reset(T* item) { - item->~T(); - ::new (item) T(); - } -}; - -} // namespace node - -#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS - -#endif // SRC_FREELIST_H_ From 8604481b2e079ed224f9d4e97445437dbf484290 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 8 May 2018 12:24:02 +0200 Subject: [PATCH 014/208] vm: move emitExperimentalWarning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit moves emitExperimentalWarning into the second object destructoring of require internal/util. PR-URL: https://github.com/nodejs/node/pull/20593 Reviewed-By: Michaël Zasso Reviewed-By: Сковорода Никита Андреевич Reviewed-By: Gus Caplan Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell Reviewed-By: Jon Moss Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Anna Henningsen --- lib/internal/vm/module.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index 7284c8bd619901..0c9ec87a07b02f 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -1,7 +1,6 @@ 'use strict'; const { internalBinding } = require('internal/bootstrap/loaders'); -const { emitExperimentalWarning } = require('internal/util'); const { URL } = require('internal/url'); const { isContext } = process.binding('contextify'); const { @@ -17,6 +16,7 @@ const { const { getConstructorOf, customInspectSymbol, + emitExperimentalWarning } = require('internal/util'); const { SafePromise } = require('internal/safe_globals'); From d568952b8c8b3a7a827a6230d8357add3abc29c3 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 9 May 2018 10:24:01 -0400 Subject: [PATCH 015/208] doc: fix missing napi_get_typedarray_info() param Also, make the type name notation more consistent. PR-URL: https://github.com/nodejs/node/pull/20631 Reviewed-By: James M Snell Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Colin Ihrig Reviewed-By: Vse Mozhet Byt --- doc/api/n-api.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 17958c8daf255e..739213c155755a 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -1697,8 +1697,9 @@ napi_status napi_get_typedarray_info(napi_env env, - `[in] typedarray`: `napi_value` representing the `TypedArray` whose properties to query. - `[out] type`: Scalar datatype of the elements within the `TypedArray`. -- `[out] length`: `Number` of elements in the `TypedArray`. -- `[out] data`: The data buffer underlying the typed array. +- `[out] length`: The number of elements in the `TypedArray`. +- `[out] data`: The data buffer underlying the `TypedArray`. +- `[out] arraybuffer`: The `ArrayBuffer` underlying the `TypedArray`. - `[out] byte_offset`: The byte offset within the data buffer from which to start projecting the `TypedArray`. From c546746396e74395d64eb96e21c2d6bb63b236a3 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 8 May 2018 22:30:16 -0400 Subject: [PATCH 016/208] doc: add util.types.isBig{Int,Uint}64Array() These methods are exposed, even though the BigInt64Array and BigUint64Array types are currently behind the --harmony-bigint command line flag. PR-URL: https://github.com/nodejs/node/pull/20615 Fixes: https://github.com/nodejs/node/issues/20602 Reviewed-By: Gus Caplan Reviewed-By: James M Snell Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Vse Mozhet Byt Reviewed-By: Anna Henningsen --- doc/api/util.md | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/doc/api/util.md b/doc/api/util.md index 380b2fd4089759..553e1e38c14db5 100644 --- a/doc/api/util.md +++ b/doc/api/util.md @@ -1009,6 +1009,44 @@ util.types.isAsyncFunction(function foo() {}); // Returns false util.types.isAsyncFunction(async function foo() {}); // Returns true ``` +### util.types.isBigInt64Array(value) + + +* Returns: {boolean} + +Returns `true` if the value is a `BigInt64Array` instance. The +`--harmony-bigint` command line flag is required in order to use the +`BigInt64Array` type, but it is not required in order to use +`isBigInt64Array()`. + +For example: + +```js +util.types.isBigInt64Array(new BigInt64Array()); // Returns true +util.types.isBigInt64Array(new BigUint64Array()); // Returns false +``` + +### util.types.isBigUint64Array(value) + + +* Returns: {boolean} + +Returns `true` if the value is a `BigUint64Array` instance. The +`--harmony-bigint` command line flag is required in order to use the +`BigUint64Array` type, but it is not required in order to use +`isBigUint64Array()`. + +For example: + +```js +util.types.isBigUint64Array(new BigInt64Array()); // Returns false +util.types.isBigUint64Array(new BigUint64Array()); // Returns true +``` + ### util.types.isBooleanObject(value) -* `value` {any} -* `message` {any} +* `value` {any} The input that is checked for being truthy. +* `message` {string|Error} An alias of [`assert.ok()`][]. @@ -181,7 +181,7 @@ changes: --> * `actual` {any} * `expected` {any} -* `message` {any} +* `message` {string|Error} **Strict mode** @@ -235,18 +235,18 @@ const obj3 = { const obj4 = Object.create(obj1); assert.deepEqual(obj1, obj1); -// OK, object is equal to itself +// OK +// Values of b are different: assert.deepEqual(obj1, obj2); // AssertionError: { a: { b: 1 } } deepEqual { a: { b: 2 } } -// values of b are different assert.deepEqual(obj1, obj3); -// OK, objects are equal +// OK +// Prototypes are ignored: assert.deepEqual(obj1, obj4); // AssertionError: { a: { b: 1 } } deepEqual {} -// Prototypes are ignored ``` If the values are not equal, an `AssertionError` is thrown with a `message` @@ -285,7 +285,7 @@ changes: --> * `actual` {any} * `expected` {any} -* `message` {any} +* `message` {string|Error} Tests for deep equality between the `actual` and `expected` parameters. "Deep" equality means that the enumerable "own" properties of child objects @@ -406,7 +406,7 @@ added: v10.0.0 --> * `block` {Function|Promise} * `error` {RegExp|Function} -* `message` {any} +* `message` {string|Error} Awaits the `block` promise or, if `block` is a function, immediately calls the function and awaits the returned promise to complete. It will then check that @@ -460,7 +460,7 @@ changes: --> * `block` {Function} * `error` {RegExp|Function} -* `message` {any} +* `message` {string|Error} Asserts that the function `block` does not throw an error. @@ -528,7 +528,7 @@ added: v0.1.21 --> * `actual` {any} * `expected` {any} -* `message` {any} +* `message` {string|Error} **Strict mode** @@ -565,7 +565,7 @@ parameter is an instance of an [`Error`][] then it will be thrown instead of the -* `message` {any} **Default:** `'Failed'` +* `message` {string|Error} **Default:** `'Failed'` Throws an `AssertionError` with the provided error message or a default error message. If the `message` parameter is an instance of an [`Error`][] then it @@ -598,7 +598,7 @@ changes: --> * `actual` {any} * `expected` {any} -* `message` {any} +* `message` {string|Error} * `operator` {string} **Default:** `'!='` * `stackStartFunction` {Function} **Default:** `assert.fail` @@ -659,8 +659,8 @@ changes: an `AssertionError` that contains the full stack trace. - version: v10.0.0 pr-url: https://github.com/nodejs/node/pull/18247 - description: Value may now only be `undefined` or `null`. Before any truthy - input was accepted. + description: Value may now only be `undefined` or `null`. Before all falsy + values were handled the same as `null` and did not throw. --> * `value` {any} @@ -717,7 +717,7 @@ changes: --> * `actual` {any} * `expected` {any} -* `message` {any} +* `message` {string|Error} **Strict mode** @@ -753,13 +753,13 @@ assert.notDeepEqual(obj1, obj1); // AssertionError: { a: { b: 1 } } notDeepEqual { a: { b: 1 } } assert.notDeepEqual(obj1, obj2); -// OK: obj1 and obj2 are not deeply equal +// OK assert.notDeepEqual(obj1, obj3); // AssertionError: { a: { b: 1 } } notDeepEqual { a: { b: 1 } } assert.notDeepEqual(obj1, obj4); -// OK: obj1 and obj4 are not deeply equal +// OK ``` If the values are deeply equal, an `AssertionError` is thrown with a `message` @@ -798,7 +798,7 @@ changes: --> * `actual` {any} * `expected` {any} -* `message` {any} +* `message` {string|Error} Tests for deep strict inequality. Opposite of [`assert.deepStrictEqual()`][]. @@ -821,7 +821,7 @@ added: v0.1.21 --> * `actual` {any} * `expected` {any} -* `message` {any} +* `message` {string|Error} **Strict mode** @@ -863,7 +863,7 @@ changes: --> * `actual` {any} * `expected` {any} -* `message` {any} +* `message` {string|Error} Tests strict inequality between the `actual` and `expected` parameters as determined by the [SameValue Comparison][]. @@ -897,7 +897,7 @@ changes: error message. --> * `value` {any} -* `message` {any} +* `message` {string|Error} Tests if `value` is truthy. It is equivalent to `assert.equal(!!value, true, message)`. @@ -960,7 +960,7 @@ added: v10.0.0 --> * `block` {Function|Promise} * `error` {RegExp|Function|Object|Error} -* `message` {any} +* `message` {string|Error} Awaits the `block` promise or, if `block` is a function, immediately calls the function and awaits the returned promise to complete. It will then check that @@ -1022,7 +1022,7 @@ changes: --> * `actual` {any} * `expected` {any} -* `message` {any} +* `message` {string|Error} Tests strict equality between the `actual` and `expected` parameters as determined by the [SameValue Comparison][]. @@ -1065,7 +1065,7 @@ changes: --> * `block` {Function} * `error` {RegExp|Function|Object|Error} -* `message` {any} +* `message` {string|Error} Expects the function `block` to throw an error. From 5d06c1e1ae53a11d91e0b3ef0687b384e5f3c3f8 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 2 May 2018 16:21:40 +0200 Subject: [PATCH 024/208] assert: move AssertionError into own file This moves the `assert` parts from `internal/errors` into an own file. `internal/errors` got bigger and bigger and it was difficult to keep a good overview of what was going on. While doing so it also removes the `internalAssert` function and just lazy loads `assert`. PR-URL: https://github.com/nodejs/node/pull/20486 Reviewed-By: James M Snell Reviewed-By: Joyee Cheung Reviewed-By: Trivikram Kamat --- lib/assert.js | 15 +- lib/internal/assert.js | 275 ++++++++++++++++++++++++++++++ lib/internal/errors.js | 314 ++--------------------------------- node.gyp | 1 + test/parallel/test-assert.js | 2 +- 5 files changed, 301 insertions(+), 306 deletions(-) create mode 100644 lib/internal/assert.js diff --git a/lib/assert.js b/lib/assert.js index ac9fd30793aaf7..27e71c2751ed74 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -25,15 +25,12 @@ const { isDeepEqual, isDeepStrictEqual } = require('internal/util/comparisons'); -const { - AssertionError, - errorCache, - codes: { - ERR_AMBIGUOUS_ARGUMENT, - ERR_INVALID_ARG_TYPE, - ERR_INVALID_RETURN_VALUE - } -} = require('internal/errors'); +const { codes: { + ERR_AMBIGUOUS_ARGUMENT, + ERR_INVALID_ARG_TYPE, + ERR_INVALID_RETURN_VALUE +} } = require('internal/errors'); +const { AssertionError, errorCache } = require('internal/assert'); const { openSync, closeSync, readSync } = require('fs'); const { inspect, types: { isPromise, isRegExp } } = require('util'); const { EOL } = require('os'); diff --git a/lib/internal/assert.js b/lib/internal/assert.js new file mode 100644 index 00000000000000..990065a9378f14 --- /dev/null +++ b/lib/internal/assert.js @@ -0,0 +1,275 @@ +'use strict'; + +const { inspect } = require('util'); +const { codes: { + ERR_INVALID_ARG_TYPE +} } = require('internal/errors'); + +let blue = ''; +let green = ''; +let red = ''; +let white = ''; + +const READABLE_OPERATOR = { + deepStrictEqual: 'Input A expected to strictly deep-equal input B', + notDeepStrictEqual: 'Input A expected to strictly not deep-equal input B', + strictEqual: 'Input A expected to strictly equal input B', + notStrictEqual: 'Input A expected to strictly not equal input B' +}; + +function copyError(source) { + const keys = Object.keys(source); + const target = Object.create(Object.getPrototypeOf(source)); + for (const key of keys) { + target[key] = source[key]; + } + Object.defineProperty(target, 'message', { value: source.message }); + return target; +} + +function inspectValue(val) { + // The util.inspect default values could be changed. This makes sure the + // error messages contain the necessary information nevertheless. + return inspect( + val, + { + compact: false, + customInspect: false, + depth: 1000, + maxArrayLength: Infinity, + // Assert compares only enumerable properties (with a few exceptions). + showHidden: false, + // Having a long line as error is better than wrapping the line for + // comparison. + breakLength: Infinity, + // Assert does not detect proxies currently. + showProxy: false + } + ).split('\n'); +} + +function createErrDiff(actual, expected, operator) { + var other = ''; + var res = ''; + var lastPos = 0; + var end = ''; + var skipped = false; + const actualLines = inspectValue(actual); + const expectedLines = inspectValue(expected); + const msg = READABLE_OPERATOR[operator] + + `:\n${green}+ expected${white} ${red}- actual${white}`; + const skippedMsg = ` ${blue}...${white} Lines skipped`; + + // Remove all ending lines that match (this optimizes the output for + // readability by reducing the number of total changed lines). + var a = actualLines[actualLines.length - 1]; + var b = expectedLines[expectedLines.length - 1]; + var i = 0; + while (a === b) { + if (i++ < 2) { + end = `\n ${a}${end}`; + } else { + other = a; + } + actualLines.pop(); + expectedLines.pop(); + if (actualLines.length === 0 || expectedLines.length === 0) + break; + a = actualLines[actualLines.length - 1]; + b = expectedLines[expectedLines.length - 1]; + } + if (i > 3) { + end = `\n${blue}...${white}${end}`; + skipped = true; + } + if (other !== '') { + end = `\n ${other}${end}`; + other = ''; + } + + const maxLines = Math.max(actualLines.length, expectedLines.length); + var printedLines = 0; + var identical = 0; + for (i = 0; i < maxLines; i++) { + // Only extra expected lines exist + const cur = i - lastPos; + if (actualLines.length < i + 1) { + if (cur > 1 && i > 2) { + if (cur > 4) { + res += `\n${blue}...${white}`; + skipped = true; + } else if (cur > 3) { + res += `\n ${expectedLines[i - 2]}`; + printedLines++; + } + res += `\n ${expectedLines[i - 1]}`; + printedLines++; + } + lastPos = i; + other += `\n${green}+${white} ${expectedLines[i]}`; + printedLines++; + // Only extra actual lines exist + } else if (expectedLines.length < i + 1) { + if (cur > 1 && i > 2) { + if (cur > 4) { + res += `\n${blue}...${white}`; + skipped = true; + } else if (cur > 3) { + res += `\n ${actualLines[i - 2]}`; + printedLines++; + } + res += `\n ${actualLines[i - 1]}`; + printedLines++; + } + lastPos = i; + res += `\n${red}-${white} ${actualLines[i]}`; + printedLines++; + // Lines diverge + } else if (actualLines[i] !== expectedLines[i]) { + if (cur > 1 && i > 2) { + if (cur > 4) { + res += `\n${blue}...${white}`; + skipped = true; + } else if (cur > 3) { + res += `\n ${actualLines[i - 2]}`; + printedLines++; + } + res += `\n ${actualLines[i - 1]}`; + printedLines++; + } + lastPos = i; + res += `\n${red}-${white} ${actualLines[i]}`; + other += `\n${green}+${white} ${expectedLines[i]}`; + printedLines += 2; + // Lines are identical + } else { + res += other; + other = ''; + if (cur === 1 || i === 0) { + res += `\n ${actualLines[i]}`; + printedLines++; + } + identical++; + } + // Inspected object to big (Show ~20 rows max) + if (printedLines > 20 && i < maxLines - 2) { + return `${msg}${skippedMsg}\n${res}\n${blue}...${white}${other}\n` + + `${blue}...${white}`; + } + } + + // Strict equal with identical objects that are not identical by reference. + if (identical === maxLines) { + // E.g., assert.deepStrictEqual(Symbol(), Symbol()) + const base = operator === 'strictEqual' ? + 'Input objects identical but not reference equal:' : + 'Input objects not identical:'; + + // We have to get the result again. The lines were all removed before. + const actualLines = inspectValue(actual); + + // Only remove lines in case it makes sense to collapse those. + // TODO: Accept env to always show the full error. + if (actualLines.length > 30) { + actualLines[26] = `${blue}...${white}`; + while (actualLines.length > 27) { + actualLines.pop(); + } + } + + return `${base}\n\n${actualLines.join('\n')}\n`; + } + return `${msg}${skipped ? skippedMsg : ''}\n${res}${other}${end}`; +} + +class AssertionError extends Error { + constructor(options) { + if (typeof options !== 'object' || options === null) { + throw new ERR_INVALID_ARG_TYPE('options', 'Object', options); + } + var { + actual, + expected, + message, + operator, + stackStartFn + } = options; + + if (message != null) { + super(message); + } else { + if (process.stdout.isTTY) { + // Reset on each call to make sure we handle dynamically set environment + // variables correct. + if (process.stdout.getColorDepth() !== 1) { + blue = '\u001b[34m'; + green = '\u001b[32m'; + white = '\u001b[39m'; + red = '\u001b[31m'; + } else { + blue = ''; + green = ''; + white = ''; + red = ''; + } + } + // Prevent the error stack from being visible by duplicating the error + // in a very close way to the original in case both sides are actually + // instances of Error. + if (typeof actual === 'object' && actual !== null && + typeof expected === 'object' && expected !== null && + 'stack' in actual && actual instanceof Error && + 'stack' in expected && expected instanceof Error) { + actual = copyError(actual); + expected = copyError(expected); + } + + if (operator === 'deepStrictEqual' || operator === 'strictEqual') { + super(createErrDiff(actual, expected, operator)); + } else if (operator === 'notDeepStrictEqual' || + operator === 'notStrictEqual') { + // In case the objects are equal but the operator requires unequal, show + // the first object and say A equals B + const res = inspectValue(actual); + const base = `Identical input passed to ${operator}:`; + + // Only remove lines in case it makes sense to collapse those. + // TODO: Accept env to always show the full error. + if (res.length > 30) { + res[26] = `${blue}...${white}`; + while (res.length > 27) { + res.pop(); + } + } + + // Only print a single input. + if (res.length === 1) { + super(`${base} ${res[0]}`); + } else { + super(`${base}\n\n${res.join('\n')}\n`); + } + } else { + let res = inspect(actual); + let other = inspect(expected); + if (res.length > 128) + res = `${res.slice(0, 125)}...`; + if (other.length > 128) + other = `${other.slice(0, 125)}...`; + super(`${res} ${operator} ${other}`); + } + } + + this.generatedMessage = !message; + this.name = 'AssertionError [ERR_ASSERTION]'; + this.code = 'ERR_ASSERTION'; + this.actual = actual; + this.expected = expected; + this.operator = operator; + Error.captureStackTrace(this, stackStartFn); + } +} + +module.exports = { + AssertionError, + errorCache: new Map() +}; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index fc306e256c1f1a..0a46e2e7a9946c 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -15,18 +15,6 @@ const kInfo = Symbol('info'); const messages = new Map(); const codes = {}; -let blue = ''; -let green = ''; -let red = ''; -let white = ''; - -const READABLE_OPERATOR = { - deepStrictEqual: 'Input A expected to strictly deep-equal input B', - notDeepStrictEqual: 'Input A expected to strictly not deep-equal input B', - strictEqual: 'Input A expected to strictly equal input B', - notStrictEqual: 'Input A expected to strictly not equal input B' -}; - const { errmap, UV_EAI_MEMORY, @@ -37,10 +25,10 @@ const { kMaxLength } = process.binding('buffer'); const { defineProperty } = Object; // Lazily loaded -var util; -var buffer; +let util; +let assert; -var internalUtil = null; +let internalUtil = null; function lazyInternalUtil() { if (!internalUtil) { internalUtil = require('internal/util'); @@ -48,35 +36,11 @@ function lazyInternalUtil() { return internalUtil; } -function copyError(source) { - const keys = Object.keys(source); - const target = Object.create(Object.getPrototypeOf(source)); - for (const key of keys) { - target[key] = source[key]; - } - Object.defineProperty(target, 'message', { value: source.message }); - return target; -} - -function inspectValue(val) { - // The util.inspect default values could be changed. This makes sure the - // error messages contain the necessary information nevertheless. - return util.inspect( - val, - { - compact: false, - customInspect: false, - depth: 1000, - maxArrayLength: Infinity, - // Assert compares only enumerable properties (with a few exceptions). - showHidden: false, - // Having a long line as error is better than wrapping the line for - // comparison. - breakLength: Infinity, - // Assert does not detect proxies currently. - showProxy: false - } - ).split('\n'); +let buffer; +function lazyBuffer() { + if (buffer === undefined) + buffer = require('buffer').Buffer; + return buffer; } // A specialized Error that includes an additional info property with @@ -241,254 +205,14 @@ function E(sym, val, def, ...otherClasses) { codes[sym] = def; } -function lazyBuffer() { - if (buffer === undefined) - buffer = require('buffer').Buffer; - return buffer; -} - -function createErrDiff(actual, expected, operator) { - var other = ''; - var res = ''; - var lastPos = 0; - var end = ''; - var skipped = false; - if (util === undefined) util = require('util'); - const actualLines = inspectValue(actual); - const expectedLines = inspectValue(expected); - const msg = READABLE_OPERATOR[operator] + - `:\n${green}+ expected${white} ${red}- actual${white}`; - const skippedMsg = ` ${blue}...${white} Lines skipped`; - - // Remove all ending lines that match (this optimizes the output for - // readability by reducing the number of total changed lines). - var a = actualLines[actualLines.length - 1]; - var b = expectedLines[expectedLines.length - 1]; - var i = 0; - while (a === b) { - if (i++ < 2) { - end = `\n ${a}${end}`; - } else { - other = a; - } - actualLines.pop(); - expectedLines.pop(); - if (actualLines.length === 0 || expectedLines.length === 0) - break; - a = actualLines[actualLines.length - 1]; - b = expectedLines[expectedLines.length - 1]; - } - if (i > 3) { - end = `\n${blue}...${white}${end}`; - skipped = true; - } - if (other !== '') { - end = `\n ${other}${end}`; - other = ''; - } - - const maxLines = Math.max(actualLines.length, expectedLines.length); - var printedLines = 0; - var identical = 0; - for (i = 0; i < maxLines; i++) { - // Only extra expected lines exist - const cur = i - lastPos; - if (actualLines.length < i + 1) { - if (cur > 1 && i > 2) { - if (cur > 4) { - res += `\n${blue}...${white}`; - skipped = true; - } else if (cur > 3) { - res += `\n ${expectedLines[i - 2]}`; - printedLines++; - } - res += `\n ${expectedLines[i - 1]}`; - printedLines++; - } - lastPos = i; - other += `\n${green}+${white} ${expectedLines[i]}`; - printedLines++; - // Only extra actual lines exist - } else if (expectedLines.length < i + 1) { - if (cur > 1 && i > 2) { - if (cur > 4) { - res += `\n${blue}...${white}`; - skipped = true; - } else if (cur > 3) { - res += `\n ${actualLines[i - 2]}`; - printedLines++; - } - res += `\n ${actualLines[i - 1]}`; - printedLines++; - } - lastPos = i; - res += `\n${red}-${white} ${actualLines[i]}`; - printedLines++; - // Lines diverge - } else if (actualLines[i] !== expectedLines[i]) { - if (cur > 1 && i > 2) { - if (cur > 4) { - res += `\n${blue}...${white}`; - skipped = true; - } else if (cur > 3) { - res += `\n ${actualLines[i - 2]}`; - printedLines++; - } - res += `\n ${actualLines[i - 1]}`; - printedLines++; - } - lastPos = i; - res += `\n${red}-${white} ${actualLines[i]}`; - other += `\n${green}+${white} ${expectedLines[i]}`; - printedLines += 2; - // Lines are identical - } else { - res += other; - other = ''; - if (cur === 1 || i === 0) { - res += `\n ${actualLines[i]}`; - printedLines++; - } - identical++; - } - // Inspected object to big (Show ~20 rows max) - if (printedLines > 20 && i < maxLines - 2) { - return `${msg}${skippedMsg}\n${res}\n${blue}...${white}${other}\n` + - `${blue}...${white}`; - } - } - - // Strict equal with identical objects that are not identical by reference. - if (identical === maxLines) { - // E.g., assert.deepStrictEqual(Symbol(), Symbol()) - const base = operator === 'strictEqual' ? - 'Input objects identical but not reference equal:' : - 'Input objects not identical:'; - - // We have to get the result again. The lines were all removed before. - const actualLines = inspectValue(actual); - - // Only remove lines in case it makes sense to collapse those. - // TODO: Accept env to always show the full error. - if (actualLines.length > 30) { - actualLines[26] = `${blue}...${white}`; - while (actualLines.length > 27) { - actualLines.pop(); - } - } - - return `${base}\n\n${actualLines.join('\n')}\n`; - } - return `${msg}${skipped ? skippedMsg : ''}\n${res}${other}${end}`; -} - -class AssertionError extends Error { - constructor(options) { - if (typeof options !== 'object' || options === null) { - throw new codes.ERR_INVALID_ARG_TYPE('options', 'Object', options); - } - var { - actual, - expected, - message, - operator, - stackStartFn - } = options; - - if (message != null) { - super(message); - } else { - if (process.stdout.isTTY) { - // Reset on each call to make sure we handle dynamically set environment - // variables correct. - if (process.stdout.getColorDepth() !== 1) { - blue = '\u001b[34m'; - green = '\u001b[32m'; - white = '\u001b[39m'; - red = '\u001b[31m'; - } else { - blue = ''; - green = ''; - white = ''; - red = ''; - } - } - if (util === undefined) util = require('util'); - // Prevent the error stack from being visible by duplicating the error - // in a very close way to the original in case both sides are actually - // instances of Error. - if (typeof actual === 'object' && actual !== null && - typeof expected === 'object' && expected !== null && - 'stack' in actual && actual instanceof Error && - 'stack' in expected && expected instanceof Error) { - actual = copyError(actual); - expected = copyError(expected); - } - - if (operator === 'deepStrictEqual' || operator === 'strictEqual') { - super(createErrDiff(actual, expected, operator)); - } else if (operator === 'notDeepStrictEqual' || - operator === 'notStrictEqual') { - // In case the objects are equal but the operator requires unequal, show - // the first object and say A equals B - const res = inspectValue(actual); - const base = `Identical input passed to ${operator}:`; - - // Only remove lines in case it makes sense to collapse those. - // TODO: Accept env to always show the full error. - if (res.length > 30) { - res[26] = `${blue}...${white}`; - while (res.length > 27) { - res.pop(); - } - } - - // Only print a single input. - if (res.length === 1) { - super(`${base} ${res[0]}`); - } else { - super(`${base}\n\n${res.join('\n')}\n`); - } - } else { - let res = util.inspect(actual); - let other = util.inspect(expected); - if (res.length > 128) - res = `${res.slice(0, 125)}...`; - if (other.length > 128) - other = `${other.slice(0, 125)}...`; - super(`${res} ${operator} ${other}`); - } - } - - this.generatedMessage = !message; - this.name = 'AssertionError [ERR_ASSERTION]'; - this.code = 'ERR_ASSERTION'; - this.actual = actual; - this.expected = expected; - this.operator = operator; - Error.captureStackTrace(this, stackStartFn); - } -} - -// This is defined here instead of using the assert module to avoid a -// circular dependency. The effect is largely the same. -function internalAssert(condition, message) { - if (!condition) { - throw new AssertionError({ - message, - actual: false, - expected: true, - operator: '==' - }); - } -} - function getMessage(key, args) { const msg = messages.get(key); + if (util === undefined) util = require('util'); + if (assert === undefined) assert = require('assert'); if (typeof msg === 'function') { - internalAssert( + assert( msg.length <= args.length, // Default options do not count. `Code: ${key}; The provided arguments length (${args.length}) does not ` + `match the required ones (${msg.length}).` @@ -497,7 +221,7 @@ function getMessage(key, args) { } const expectedLength = (msg.match(/%[dfijoOs]/g) || []).length; - internalAssert( + assert( expectedLength === args.length, `Code: ${key}; The provided arguments length (${args.length}) does not ` + `match the required ones (${expectedLength}).` @@ -693,11 +417,9 @@ module.exports = { uvException, isStackOverflowError, getMessage, - AssertionError, SystemError, codes, - E, // This is exported only to facilitate testing. - errorCache: new Map() // This is in here only to facilitate testing. + E // This is exported only to facilitate testing. }; // To declare an error message, use the E(sym, val, def) function above. The sym @@ -1045,7 +767,7 @@ E('ERR_VM_MODULE_STATUS', 'Module status %s', Error); E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed', Error); function invalidArgType(name, expected, actual) { - internalAssert(typeof name === 'string', 'name must be a string'); + assert(typeof name === 'string', "'name' must be a string"); // determiner: 'must be' or 'must not be' let determiner; @@ -1071,7 +793,7 @@ function invalidArgType(name, expected, actual) { } function missingArgs(...args) { - internalAssert(args.length > 0, 'At least one arg needs to be specified'); + assert(args.length > 0, 'At least one arg needs to be specified'); let msg = 'The '; const len = args.length; args = args.map((a) => `"${a}"`); @@ -1091,11 +813,11 @@ function missingArgs(...args) { } function oneOf(expected, thing) { - internalAssert(typeof thing === 'string', '`thing` has to be of type string'); + assert(typeof thing === 'string', '`thing` has to be of type string'); if (Array.isArray(expected)) { const len = expected.length; - internalAssert(len > 0, - 'At least one expected value needs to be specified'); + assert(len > 0, + 'At least one expected value needs to be specified'); expected = expected.map((i) => String(i)); if (len > 2) { return `one of ${thing} ${expected.slice(0, len - 1).join(', ')}, or ` + diff --git a/node.gyp b/node.gyp index 851fdb8c04cc81..621669e514f139 100644 --- a/node.gyp +++ b/node.gyp @@ -79,6 +79,7 @@ 'lib/v8.js', 'lib/vm.js', 'lib/zlib.js', + 'lib/internal/assert.js', 'lib/internal/async_hooks.js', 'lib/internal/buffer.js', 'lib/internal/cli_table.js', diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index cbcda17ab7188d..cab6fb33e1887a 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -29,7 +29,7 @@ const common = require('../common'); const assert = require('assert'); const { EOL } = require('os'); const EventEmitter = require('events'); -const { errorCache } = require('internal/errors'); +const { errorCache } = require('internal/assert'); const { writeFileSync, unlinkSync } = require('fs'); const { inspect } = require('util'); const a = assert; From c24443670785ff98b9a9ecc19fbf53cf6d020c72 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 2 May 2018 16:30:54 +0200 Subject: [PATCH 025/208] errors: move functions to error code This makes sure the functions are actually directly beneath the specification of an error code. That way it is not necessary to jump around when looking at the functionality. PR-URL: https://github.com/nodejs/node/pull/20486 Reviewed-By: James M Snell Reviewed-By: Joyee Cheung Reviewed-By: Trivikram Kamat --- lib/internal/errors.js | 189 ++++++++++++++++++++--------------------- 1 file changed, 92 insertions(+), 97 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 0a46e2e7a9946c..b9db40abd3b4c9 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -410,6 +410,26 @@ function isStackOverflowError(err) { err.message === maxStack_ErrorMessage; } +function oneOf(expected, thing) { + assert(typeof thing === 'string', '`thing` has to be of type string'); + if (Array.isArray(expected)) { + const len = expected.length; + assert(len > 0, + 'At least one expected value needs to be specified'); + expected = expected.map((i) => String(i)); + if (len > 2) { + return `one of ${thing} ${expected.slice(0, len - 1).join(', ')}, or ` + + expected[len - 1]; + } else if (len === 2) { + return `one of ${thing} ${expected[0]} or ${expected[1]}`; + } else { + return `of ${thing} ${expected[0]}`; + } + } else { + return `of ${thing} ${String(expected)}`; + } +} + module.exports = { dnsException, errnoException, @@ -444,7 +464,15 @@ E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError); E('ERR_ASSERTION', '%s', Error); E('ERR_ASYNC_CALLBACK', '%s must be a function', TypeError); E('ERR_ASYNC_TYPE', 'Invalid name for async "type": %s', TypeError); -E('ERR_BUFFER_OUT_OF_BOUNDS', bufferOutOfBounds, RangeError); +E('ERR_BUFFER_OUT_OF_BOUNDS', + // Using a default argument here is important so the argument is not counted + // towards `Function#length`. + (name = undefined) => { + if (name) { + return `"${name}" is outside of buffer bounds`; + } + return 'Attempt to write outside buffer bounds'; + }, RangeError); E('ERR_BUFFER_TOO_LARGE', `Cannot create a Buffer larger than 0x${kMaxLength.toString(16)} bytes`, RangeError); @@ -582,7 +610,32 @@ E('ERR_INSPECTOR_CLOSED', 'Session was closed', Error); E('ERR_INSPECTOR_NOT_AVAILABLE', 'Inspector is not available', Error); E('ERR_INSPECTOR_NOT_CONNECTED', 'Session is not connected', Error); E('ERR_INVALID_ADDRESS_FAMILY', 'Invalid address family: %s', RangeError); -E('ERR_INVALID_ARG_TYPE', invalidArgType, TypeError); +E('ERR_INVALID_ARG_TYPE', + (name, expected, actual) => { + assert(typeof name === 'string', "'name' must be a string"); + + // determiner: 'must be' or 'must not be' + let determiner; + if (typeof expected === 'string' && expected.startsWith('not ')) { + determiner = 'must not be'; + expected = expected.replace(/^not /, ''); + } else { + determiner = 'must be'; + } + + let msg; + if (name.endsWith(' argument')) { + // For cases like 'first argument' + msg = `The ${name} ${determiner} ${oneOf(expected, 'type')}`; + } else { + const type = name.includes('.') ? 'property' : 'argument'; + msg = `The "${name}" ${type} ${determiner} ${oneOf(expected, 'type')}`; + } + + // TODO(BridgeAR): Improve the output by showing `null` and similar. + msg += `. Received type ${typeof actual}`; + return msg; + }, TypeError); E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => { let inspected = util.inspect(value); if (inspected.length > 128) { @@ -598,7 +651,16 @@ E('ERR_INVALID_ASYNC_ID', 'Invalid %s value: %s', RangeError); E('ERR_INVALID_BUFFER_SIZE', 'Buffer size must be a multiple of %s', RangeError); E('ERR_INVALID_CALLBACK', 'Callback must be a function', TypeError); -E('ERR_INVALID_CHAR', invalidChar, TypeError); +E('ERR_INVALID_CHAR', + // Using a default argument here is important so the argument is not counted + // towards `Function#length`. + (name, field = undefined) => { + let msg = `Invalid character in ${name}`; + if (field !== undefined) { + msg += ` ["${field}"]`; + } + return msg; + }, TypeError); E('ERR_INVALID_CURSOR_POS', 'Cannot set cursor row without setting its column', TypeError); E('ERR_INVALID_DOMAIN_NAME', 'Unable to determine the domain name', TypeError); @@ -648,7 +710,26 @@ E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected', Error); E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe', Error); E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks', Error); E('ERR_METHOD_NOT_IMPLEMENTED', 'The %s method is not implemented', Error); -E('ERR_MISSING_ARGS', missingArgs, TypeError); +E('ERR_MISSING_ARGS', + (...args) => { + assert(args.length > 0, 'At least one arg needs to be specified'); + let msg = 'The '; + const len = args.length; + args = args.map((a) => `"${a}"`); + switch (len) { + case 1: + msg += `${args[0]} argument`; + break; + case 2: + msg += `${args[0]} and ${args[1]} arguments`; + break; + default: + msg += args.slice(0, len - 1).join(', '); + msg += `, and ${args[len - 1]} arguments`; + break; + } + return `${msg} must be specified`; + }, TypeError); E('ERR_MISSING_MODULE', 'Cannot find module %s', Error); E('ERR_MODULE_RESOLUTION_LEGACY', '%s not found by import in %s.' + @@ -669,7 +750,13 @@ E('ERR_NO_CRYPTO', E('ERR_NO_ICU', '%s is not supported on Node.js compiled without ICU', TypeError); E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported', Error); -E('ERR_OUT_OF_RANGE', outOfRange, RangeError); +E('ERR_OUT_OF_RANGE', + (name, range, value) => { + let msg = `The value of "${name}" is out of range.`; + if (range !== undefined) msg += ` It must be ${range}.`; + msg += ` Received ${value}`; + return msg; + }, RangeError); E('ERR_REQUIRE_ESM', 'Must use import to load ES Module: %s', Error); E('ERR_SCRIPT_EXECUTION_INTERRUPTED', 'Script execution was interrupted by `SIGINT`.', Error); @@ -765,95 +852,3 @@ E('ERR_VM_MODULE_NOT_MODULE', 'Provided module is not an instance of Module', Error); E('ERR_VM_MODULE_STATUS', 'Module status %s', Error); E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed', Error); - -function invalidArgType(name, expected, actual) { - assert(typeof name === 'string', "'name' must be a string"); - - // determiner: 'must be' or 'must not be' - let determiner; - if (typeof expected === 'string' && expected.startsWith('not ')) { - determiner = 'must not be'; - expected = expected.replace(/^not /, ''); - } else { - determiner = 'must be'; - } - - let msg; - if (name.endsWith(' argument')) { - // For cases like 'first argument' - msg = `The ${name} ${determiner} ${oneOf(expected, 'type')}`; - } else { - const type = name.includes('.') ? 'property' : 'argument'; - msg = `The "${name}" ${type} ${determiner} ${oneOf(expected, 'type')}`; - } - - // TODO(BridgeAR): Improve the output by showing `null` and similar. - msg += `. Received type ${typeof actual}`; - return msg; -} - -function missingArgs(...args) { - assert(args.length > 0, 'At least one arg needs to be specified'); - let msg = 'The '; - const len = args.length; - args = args.map((a) => `"${a}"`); - switch (len) { - case 1: - msg += `${args[0]} argument`; - break; - case 2: - msg += `${args[0]} and ${args[1]} arguments`; - break; - default: - msg += args.slice(0, len - 1).join(', '); - msg += `, and ${args[len - 1]} arguments`; - break; - } - return `${msg} must be specified`; -} - -function oneOf(expected, thing) { - assert(typeof thing === 'string', '`thing` has to be of type string'); - if (Array.isArray(expected)) { - const len = expected.length; - assert(len > 0, - 'At least one expected value needs to be specified'); - expected = expected.map((i) => String(i)); - if (len > 2) { - return `one of ${thing} ${expected.slice(0, len - 1).join(', ')}, or ` + - expected[len - 1]; - } else if (len === 2) { - return `one of ${thing} ${expected[0]} or ${expected[1]}`; - } else { - return `of ${thing} ${expected[0]}`; - } - } else { - return `of ${thing} ${String(expected)}`; - } -} - -// Using a default argument here is important so the argument is not counted -// towards `Function#length`. -function bufferOutOfBounds(name = undefined) { - if (name) { - return `"${name}" is outside of buffer bounds`; - } - return 'Attempt to write outside buffer bounds'; -} - -// Using a default argument here is important so the argument is not counted -// towards `Function#length`. -function invalidChar(name, field = undefined) { - let msg = `Invalid character in ${name}`; - if (field !== undefined) { - msg += ` ["${field}"]`; - } - return msg; -} - -function outOfRange(name, range, value) { - let msg = `The value of "${name}" is out of range.`; - if (range !== undefined) msg += ` It must be ${range}.`; - msg += ` Received ${value}`; - return msg; -} From e93726ad10ee240c8b86a5522f69144eeb9a06f5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 9 May 2018 19:52:53 +0200 Subject: [PATCH 026/208] src: fix nullptr dereference for signal during startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a test failure when running `test/parallel/test-child-process-spawnsync-kill-signal.js` under load. What would happen is that `SignalExit()` tries to shutdown the tracing agent, which might not have been set up by the point that Node.js receives the signal. PR-URL: https://github.com/nodejs/node/pull/20637 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Tobias Nießen Reviewed-By: Ben Noordhuis --- src/node.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node.cc b/src/node.cc index 704bdc334b2f9c..bcc3971bbe2fb7 100644 --- a/src/node.cc +++ b/src/node.cc @@ -326,7 +326,8 @@ static struct { } void StopTracingAgent() { - tracing_agent_->Stop(); + if (tracing_agent_) + tracing_agent_->Stop(); } tracing::Agent* GetTracingAgent() const { From bb857d9e71e98614a6ca9efc6363be565dc4df54 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 2 May 2018 16:59:49 +0200 Subject: [PATCH 027/208] assert: make sure throws is able to handle primitives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes some possible issues with `assert.throws` and `assert.rejects` in combination with an validation object. It will now properly handle primitive values being thrown as error. It also makes sure the `generatedMessage` property is properly set if `assert.throws` or `assert.rejects` is used in combination with an validation object and improves the error performance in such cases by only creating the error once. In addition it will fix detecting regular expressions from a different context such as n-api that are passed through as validator for `assert.throws` or `assert.rejects`. Until now those were not tested. PR-URL: https://github.com/nodejs/node/pull/20482 Reviewed-By: James M Snell Reviewed-By: Michaël Zasso --- lib/assert.js | 30 +++++++++++++++---- test/message/assert_throws_stack.out | 4 +-- test/parallel/test-assert.js | 45 ++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index 27e71c2751ed74..8bcdc8cdacb3ee 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -382,16 +382,16 @@ function compareExceptionKey(actual, expected, key, message, keys) { const a = new Comparison(actual, keys); const b = new Comparison(expected, keys, actual); - const tmpLimit = Error.stackTraceLimit; - Error.stackTraceLimit = 0; const err = new AssertionError({ actual: a, expected: b, operator: 'deepStrictEqual', stackStartFn: assert.throws }); - Error.stackTraceLimit = tmpLimit; - message = err.message; + err.actual = actual; + err.expected = expected; + err.operator = 'throws'; + throw err; } innerFail({ actual, @@ -405,7 +405,7 @@ function compareExceptionKey(actual, expected, key, message, keys) { function expectedException(actual, expected, msg) { if (typeof expected !== 'function') { - if (expected instanceof RegExp) + if (isRegExp(expected)) return expected.test(actual); // assert.doesNotThrow does not accept objects. if (arguments.length === 2) { @@ -413,6 +413,26 @@ function expectedException(actual, expected, msg) { 'expected', ['Function', 'RegExp'], expected ); } + + // TODO: Disallow primitives as error argument. + // This is here to prevent a breaking change. + if (typeof expected !== 'object') { + return true; + } + + // Handle primitives properly. + if (typeof actual !== 'object' || actual === null) { + const err = new AssertionError({ + actual, + expected, + message: msg, + operator: 'deepStrictEqual', + stackStartFn: assert.throws + }); + err.operator = 'throws'; + throw err; + } + const keys = Object.keys(expected); // Special handle errors to make sure the name and the message are compared // as well. diff --git a/test/message/assert_throws_stack.out b/test/message/assert_throws_stack.out index 62a25a63673dcf..3d5f4de4cf26a6 100644 --- a/test/message/assert_throws_stack.out +++ b/test/message/assert_throws_stack.out @@ -1,6 +1,6 @@ assert.js:* - throw new AssertionError(obj); - ^ + throw err; + ^ AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B: + expected - actual diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index cab6fb33e1887a..dba40c0a0e3c42 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -740,7 +740,9 @@ common.expectsError( const frames = err.stack.split('\n'); const [, filename, line, column] = frames[1].match(/\((.+):(\d+):(\d+)\)/); // Reset the cache to check again + const size = errorCache.size; errorCache.delete(`${filename}${line - 1}${column - 1}`); + assert.strictEqual(errorCache.size, size - 1); const data = `${'\n'.repeat(line - 1)}${' '.repeat(column - 1)}` + 'ok(failed(badly));'; try { @@ -849,6 +851,7 @@ common.expectsError( { name: 'AssertionError [ERR_ASSERTION]', code: 'ERR_ASSERTION', + generatedMessage: true, message: `${start}\n${actExp}\n\n` + " Comparison {\n name: 'Error',\n- message: 'foo'" + "\n+ message: ''\n }" @@ -940,3 +943,45 @@ assert.throws( ' }' } ); + +{ + let actual = null; + const expected = { message: 'foo' }; + assert.throws( + () => assert.throws( + () => { throw actual; }, + expected + ), + { + operator: 'throws', + actual, + expected, + generatedMessage: true, + message: `${start}\n${actExp}\n\n` + + '- null\n' + + '+ {\n' + + "+ message: 'foo'\n" + + '+ }' + } + ); + + actual = 'foobar'; + const message = 'message'; + assert.throws( + () => assert.throws( + () => { throw actual; }, + { message: 'foobar' }, + message + ), + { + actual, + message, + operator: 'throws', + generatedMessage: false + } + ); +} + +// TODO: This case is only there to make sure there is no breaking change. +// eslint-disable-next-line no-restricted-syntax, no-throw-literal +assert.throws(() => { throw 4; }, 4); From db457cb6a0b699f2ca16c612546a85c68ac3823f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 May 2018 14:27:56 +0200 Subject: [PATCH 028/208] src: fix typo in util.h comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/20656 Reviewed-By: Michaël Zasso Reviewed-By: James M Snell --- src/util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util.h b/src/util.h index 133899008fb7ce..b2f357b8e87edc 100644 --- a/src/util.h +++ b/src/util.h @@ -211,7 +211,7 @@ inline v8::Local PersistentToLocal( v8::Isolate* isolate, const Persistent& persistent); -// Unchecked conversion from a non-weak Persistent to Local, +// Unchecked conversion from a non-weak Persistent to Local, // use with care! // // Do not call persistent.Reset() while the returned Local is still in From 3929516a6f2747ac50d0f94ccd3a26df9dff3930 Mon Sep 17 00:00:00 2001 From: Vse Mozhet Byt Date: Tue, 8 May 2018 17:49:38 +0300 Subject: [PATCH 029/208] doc: fix nits in doc/api_assets/style.css MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Merge rule sets for identical selectors. 2. Delete impossible selector block: we have only stability indexes 0, 1, and 2, so there can't be `.api_stability_3` class. Refs: nodejs.org/api/documentation.html#documentation_stability_index PR-URL: https://github.com/nodejs/node/pull/20601 Refs: https://nodejs.org/api/documentation.html#documentation_stability_index Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Jon Moss Reviewed-By: Trivikram Kamat Reviewed-By: Tobias Nießen --- doc/api_assets/style.css | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/doc/api_assets/style.css b/doc/api_assets/style.css index 1e7baad4ab676c..97ef2a5b892ac4 100644 --- a/doc/api_assets/style.css +++ b/doc/api_assets/style.css @@ -29,6 +29,9 @@ h6 { font-size: 1rem } h1, h2, h3, h4, h5, h6 { margin: 1.5rem 0 1rem; + text-rendering: optimizeLegibility; + font-weight: 700; + position: relative; } pre, tt, code, .pre, span.type, a.type { @@ -183,10 +186,6 @@ ol.version-picker li:last-child a { background-color: #4EBA0F; } -.api_stability_3 { - background-color: #0084B6; -} - .api_metadata { font-size: .85rem; margin-bottom: 1rem; @@ -226,6 +225,8 @@ table { th, td { border: 1px solid #aaa; + padding: .75rem 1rem .75rem 1rem; + vertical-align: top; } th { @@ -259,12 +260,6 @@ dd + dt.pre { margin-top: 1.6rem; } -h1, h2, h3, h4, h5, h6 { - text-rendering: optimizeLegibility; - font-weight: 700; - position: relative; -} - #apicontent { padding-top: 1rem; } @@ -373,9 +368,6 @@ hr { #toc .stability_0::after { background-color: #d50027; color: #fff; -} - -#toc .stability_0::after { content: "deprecated"; margin-left: .25rem; padding: 1px 3px; @@ -493,11 +485,6 @@ span > .mark:hover, span > .mark:focus, span > .mark:active { background: none; } -th, td { - padding: .75rem 1rem .75rem 1rem; - vertical-align: top; -} - th > *:last-child, td > *:last-child { margin-bottom: 0; } From 41e1dc09dedc592b5c6dded64b18bc0fad9b8211 Mon Sep 17 00:00:00 2001 From: Benjamin Coe Date: Fri, 4 May 2018 13:45:57 -0700 Subject: [PATCH 030/208] test: add regression test for #11257 Refs: https://github.com/nodejs/node/issues/11257 PR-URL: https://github.com/nodejs/node/pull/20391 Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell Reviewed-By: Michael Dawson Reviewed-By: Tiancheng "Timothy" Gu --- test/parallel/test-stdio-pipe-stderr.js | 37 +++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 test/parallel/test-stdio-pipe-stderr.js diff --git a/test/parallel/test-stdio-pipe-stderr.js b/test/parallel/test-stdio-pipe-stderr.js new file mode 100644 index 00000000000000..06e93429d03f6d --- /dev/null +++ b/test/parallel/test-stdio-pipe-stderr.js @@ -0,0 +1,37 @@ +'use strict'; +require('../common'); +const tmpdir = require('../common/tmpdir'); +const assert = require('assert'); +const fs = require('fs'); +const join = require('path').join; +const spawn = require('child_process').spawnSync; + +// Test that invoking node with require, and piping stderr to file, +// does not result in exception, +// see: https://github.com/nodejs/node/issues/11257 + +tmpdir.refresh(); +const fakeModulePath = join(tmpdir.path, 'batman.js'); +const stderrOutputPath = join(tmpdir.path, 'stderr-output.txt'); +// we need to redirect stderr to a file to produce #11257 +const stream = fs.createWriteStream(stderrOutputPath); + +// the error described in #11257 only happens when we require a +// non-built-in module. +fs.writeFileSync(fakeModulePath, '', 'utf8'); + +stream.on('open', () => { + spawn(process.execPath, { + input: `require("${fakeModulePath.replace(/\\/g, '/')}")`, + stdio: ['pipe', 'pipe', stream] + }); + const stderr = fs.readFileSync(stderrOutputPath, 'utf8').trim(); + assert.strictEqual( + stderr, + '', + `piping stderr to file should not result in exception: ${stderr}` + ); + stream.end(); + fs.unlinkSync(stderrOutputPath); + fs.unlinkSync(fakeModulePath); +}); From 56530f084499dba826f93a7b09b8d04ec4cc5df0 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Wed, 25 Apr 2018 12:45:34 -0400 Subject: [PATCH 031/208] timers: make timer.refresh() a public API Originally added in bb5575aa75fd3071724d5eccde39a3041e1af57a discussions such as https://github.com/nodejs/node/issues/20261 show the usefulness of this API to the Node.js ecosystem. PR-URL: https://github.com/nodejs/node/pull/20298 Reviewed-By: Anatoli Papirovski Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott Reviewed-By: Colin Ihrig Reviewed-By: Ujjwal Sharma Reviewed-By: Trivikram Kamat Reviewed-By: James M Snell Reviewed-By: Tiancheng "Timothy" Gu --- doc/api/timers.md | 15 +++++++++++++++ lib/internal/http2/core.js | 7 +++---- lib/internal/timers.js | 6 +++--- lib/net.js | 5 ++--- test/parallel/test-timers-refresh.js | 25 +++++++++++++++++++++---- 5 files changed, 44 insertions(+), 14 deletions(-) diff --git a/doc/api/timers.md b/doc/api/timers.md index 376b5312e5a41a..c573d2afdb9e21 100644 --- a/doc/api/timers.md +++ b/doc/api/timers.md @@ -74,6 +74,21 @@ When called, requests that the Node.js event loop *not* exit so long as the By default, all `Timeout` objects are "ref'ed", making it normally unnecessary to call `timeout.ref()` unless `timeout.unref()` had been called previously. +### timeout.refresh() + + +* Returns: {Timeout} a reference to `timeout` + +Sets the timer's start time to the current time, and reschedules the timer to +call its callback at the previously specified duration adjusted to the current +time. This is useful for refreshing a timer without allocating a new +JavaScript object. + +Using this on a timer that has already called its callback will reactivate the +timer. + ### timeout.unref() + +* Returns: {boolean} + +Returns `true` if the value is an instance of a [Module Namespace Object][]. + +For example: + + +```js +import * as ns from './a.js'; + +util.types.isModuleNamespaceObject(ns); // Returns true +``` + ### util.types.isNativeError(value) diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js index 828370f6eadd01..7dc44d5bbe28fb 100644 --- a/lib/internal/bootstrap/loaders.js +++ b/lib/internal/bootstrap/loaders.js @@ -41,10 +41,26 @@ (function bootstrapInternalLoaders(process, getBinding, getLinkedBinding, getInternalBinding) { + const { + apply: ReflectApply, + deleteProperty: ReflectDeleteProperty, + get: ReflectGet, + getOwnPropertyDescriptor: ReflectGetOwnPropertyDescriptor, + has: ReflectHas, + set: ReflectSet, + } = Reflect; + const { + prototype: { + hasOwnProperty: ObjectHasOwnProperty, + }, + create: ObjectCreate, + defineProperty: ObjectDefineProperty, + keys: ObjectKeys, + } = Object; // Set up process.moduleLoadList const moduleLoadList = []; - Object.defineProperty(process, 'moduleLoadList', { + ObjectDefineProperty(process, 'moduleLoadList', { value: moduleLoadList, configurable: true, enumerable: true, @@ -53,7 +69,7 @@ // Set up process.binding() and process._linkedBinding() { - const bindingObj = Object.create(null); + const bindingObj = ObjectCreate(null); process.binding = function binding(module) { module = String(module); @@ -77,7 +93,7 @@ // Set up internalBinding() in the closure let internalBinding; { - const bindingObj = Object.create(null); + const bindingObj = ObjectCreate(null); internalBinding = function internalBinding(module) { let mod = bindingObj[module]; if (typeof mod !== 'object') { @@ -95,6 +111,8 @@ this.filename = `${id}.js`; this.id = id; this.exports = {}; + this.reflect = undefined; + this.exportKeys = undefined; this.loaded = false; this.loading = false; } @@ -193,6 +211,12 @@ '\n});' ]; + const getOwn = (target, property, receiver) => { + return ReflectApply(ObjectHasOwnProperty, target, [property]) ? + ReflectGet(target, property, receiver) : + undefined; + }; + NativeModule.prototype.compile = function() { let source = NativeModule.getSource(this.id); source = NativeModule.wrap(source); @@ -208,6 +232,60 @@ NativeModule.require; fn(this.exports, requireFn, this, process); + if (config.experimentalModules && !NativeModule.isInternal(this.id)) { + this.exportKeys = ObjectKeys(this.exports); + + const update = (property, value) => { + if (this.reflect !== undefined && + ReflectApply(ObjectHasOwnProperty, + this.reflect.exports, [property])) + this.reflect.exports[property].set(value); + }; + + const handler = { + __proto__: null, + defineProperty: (target, prop, descriptor) => { + // Use `Object.defineProperty` instead of `Reflect.defineProperty` + // to throw the appropriate error if something goes wrong. + ObjectDefineProperty(target, prop, descriptor); + if (typeof descriptor.get === 'function' && + !ReflectHas(handler, 'get')) { + handler.get = (target, prop, receiver) => { + const value = ReflectGet(target, prop, receiver); + if (ReflectApply(ObjectHasOwnProperty, target, [prop])) + update(prop, value); + return value; + }; + } + update(prop, getOwn(target, prop)); + return true; + }, + deleteProperty: (target, prop) => { + if (ReflectDeleteProperty(target, prop)) { + update(prop, undefined); + return true; + } + return false; + }, + set: (target, prop, value, receiver) => { + const descriptor = ReflectGetOwnPropertyDescriptor(target, prop); + if (ReflectSet(target, prop, value, receiver)) { + if (descriptor && typeof descriptor.set === 'function') { + for (const key of this.exportKeys) { + update(key, getOwn(target, key, receiver)); + } + } else { + update(prop, getOwn(target, prop, receiver)); + } + return true; + } + return false; + } + }; + + this.exports = new Proxy(this.exports, handler); + } + this.loaded = true; } finally { this.loading = false; diff --git a/lib/internal/modules/esm/create_dynamic_module.js b/lib/internal/modules/esm/create_dynamic_module.js index e0c97263631432..c01844feb8b654 100644 --- a/lib/internal/modules/esm/create_dynamic_module.js +++ b/lib/internal/modules/esm/create_dynamic_module.js @@ -52,9 +52,10 @@ const createDynamicModule = (exports, url = '', evaluate) => { const module = new ModuleWrap(reexports, `${url}`); module.link(async () => reflectiveModule); module.instantiate(); + reflect.namespace = module.namespace(); return { module, - reflect + reflect, }; }; diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index d181647c4505e8..4ed6b2c3b023a9 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -59,11 +59,18 @@ translators.set('cjs', async (url) => { // through normal resolution translators.set('builtin', async (url) => { debug(`Translating BuiltinModule ${url}`); - return createDynamicModule(['default'], url, (reflect) => { - debug(`Loading BuiltinModule ${url}`); - const exports = NativeModule.require(url.slice(5)); - reflect.exports.default.set(exports); - }); + // slice 'node:' scheme + const id = url.slice(5); + NativeModule.require(id); + const module = NativeModule.getCached(id); + return createDynamicModule( + [...module.exportKeys, 'default'], url, (reflect) => { + debug(`Loading BuiltinModule ${url}`); + module.reflect = reflect; + for (const key of module.exportKeys) + reflect.exports[key].set(module.exports[key]); + reflect.exports.default.set(module.exports); + }); }); // Strategy for loading a node native module diff --git a/test/es-module/test-esm-dynamic-import.js b/test/es-module/test-esm-dynamic-import.js index c6daa95f9a6b98..91757172880f67 100644 --- a/test/es-module/test-esm-dynamic-import.js +++ b/test/es-module/test-esm-dynamic-import.js @@ -52,6 +52,7 @@ function expectFsNamespace(result) { Promise.resolve(result) .then(common.mustCall(ns => { assert.strictEqual(typeof ns.default.writeFile, 'function'); + assert.strictEqual(typeof ns.writeFile, 'function'); })); } diff --git a/test/es-module/test-esm-live-binding.mjs b/test/es-module/test-esm-live-binding.mjs new file mode 100644 index 00000000000000..d151e004dff4b2 --- /dev/null +++ b/test/es-module/test-esm-live-binding.mjs @@ -0,0 +1,143 @@ +// Flags: --experimental-modules + +import '../common'; +import assert from 'assert'; + +import fs, { readFile, readFileSync } from 'fs'; +import events, { defaultMaxListeners } from 'events'; +import util from 'util'; + +const readFileDescriptor = Reflect.getOwnPropertyDescriptor(fs, 'readFile'); +const readFileSyncDescriptor = + Reflect.getOwnPropertyDescriptor(fs, 'readFileSync'); + +const s = Symbol(); +const fn = () => s; + +Reflect.deleteProperty(fs, 'readFile'); + +assert.deepStrictEqual([fs.readFile, readFile], [undefined, undefined]); + +fs.readFile = fn; + +assert.deepStrictEqual([fs.readFile(), readFile()], [s, s]); + +Reflect.defineProperty(fs, 'readFile', { + value: fn, + configurable: true, + writable: true, +}); + +assert.deepStrictEqual([fs.readFile(), readFile()], [s, s]); + +Reflect.deleteProperty(fs, 'readFile'); + +assert.deepStrictEqual([fs.readFile, readFile], [undefined, undefined]); + +let count = 0; + +Reflect.defineProperty(fs, 'readFile', { + get() { return count; }, + configurable: true, +}); + +count++; + +assert.deepStrictEqual([readFile, fs.readFile, readFile], [0, 1, 1]); + +let otherValue; + +Reflect.defineProperty(fs, 'readFile', { // eslint-disable-line accessor-pairs + set(value) { + Reflect.deleteProperty(fs, 'readFile'); + otherValue = value; + }, + configurable: true, +}); + +Reflect.defineProperty(fs, 'readFileSync', { + get() { + return otherValue; + }, + configurable: true, +}); + +fs.readFile = 2; + +assert.deepStrictEqual([readFile, readFileSync], [undefined, 2]); + +Reflect.defineProperty(fs, 'readFile', readFileDescriptor); +Reflect.defineProperty(fs, 'readFileSync', readFileSyncDescriptor); + +const originDefaultMaxListeners = events.defaultMaxListeners; +const utilProto = util.__proto__; // eslint-disable-line no-proto + +count = 0; + +Reflect.defineProperty(Function.prototype, 'defaultMaxListeners', { + configurable: true, + enumerable: true, + get: function() { return ++count; }, + set: function(v) { + Reflect.defineProperty(this, 'defaultMaxListeners', { + configurable: true, + enumerable: true, + writable: true, + value: v, + }); + }, +}); + +assert.strictEqual(defaultMaxListeners, originDefaultMaxListeners); +assert.strictEqual(events.defaultMaxListeners, originDefaultMaxListeners); + +assert.strictEqual(++events.defaultMaxListeners, + originDefaultMaxListeners + 1); + +assert.strictEqual(defaultMaxListeners, originDefaultMaxListeners + 1); +assert.strictEqual(Function.prototype.defaultMaxListeners, 1); + +Function.prototype.defaultMaxListeners = 'foo'; + +assert.strictEqual(Function.prototype.defaultMaxListeners, 'foo'); +assert.strictEqual(events.defaultMaxListeners, originDefaultMaxListeners + 1); +assert.strictEqual(defaultMaxListeners, originDefaultMaxListeners + 1); + +count = 0; + +const p = { + get foo() { return ++count; }, + set foo(v) { + Reflect.defineProperty(this, 'foo', { + configurable: true, + enumerable: true, + writable: true, + value: v, + }); + }, +}; + +util.__proto__ = p; // eslint-disable-line no-proto + +assert.strictEqual(util.foo, 1); + +util.foo = 'bar'; + +assert.strictEqual(count, 1); +assert.strictEqual(util.foo, 'bar'); +assert.strictEqual(p.foo, 2); + +p.foo = 'foo'; + +assert.strictEqual(p.foo, 'foo'); + +events.defaultMaxListeners = originDefaultMaxListeners; +util.__proto__ = utilProto; // eslint-disable-line no-proto + +Reflect.deleteProperty(util, 'foo'); +Reflect.deleteProperty(Function.prototype, 'defaultMaxListeners'); + +assert.throws( + () => Object.defineProperty(events, 'defaultMaxListeners', { value: 3 }), + /TypeError: Cannot redefine/ +); diff --git a/test/es-module/test-esm-namespace.mjs b/test/es-module/test-esm-namespace.mjs index 04845e2b3e4da0..dcd159f6c8729a 100644 --- a/test/es-module/test-esm-namespace.mjs +++ b/test/es-module/test-esm-namespace.mjs @@ -2,5 +2,13 @@ import '../common'; import * as fs from 'fs'; import assert from 'assert'; +import Module from 'module'; -assert.deepStrictEqual(Object.keys(fs), ['default']); +const keys = Object.entries( + Object.getOwnPropertyDescriptors(new Module().require('fs'))) + .filter(([name, d]) => d.enumerable) + .map(([name]) => name) + .concat('default') + .sort(); + +assert.deepStrictEqual(Object.keys(fs).sort(), keys); diff --git a/test/fixtures/es-module-loaders/js-loader.mjs b/test/fixtures/es-module-loaders/js-loader.mjs index 2173b0b503ef45..9fa6b9eed4d029 100644 --- a/test/fixtures/es-module-loaders/js-loader.mjs +++ b/test/fixtures/es-module-loaders/js-loader.mjs @@ -1,10 +1,11 @@ -import _url from 'url'; +import { URL } from 'url'; + const builtins = new Set( Object.keys(process.binding('natives')).filter(str => /^(?!(?:internal|node|v8)\/)/.test(str)) ) -const baseURL = new _url.URL('file://'); +const baseURL = new URL('file://'); baseURL.pathname = process.cwd() + '/'; export function resolve (specifier, base = baseURL) { @@ -15,7 +16,7 @@ export function resolve (specifier, base = baseURL) { }; } // load all dependencies as esm, regardless of file extension - const url = new _url.URL(specifier, base).href; + const url = new URL(specifier, base).href; return { url, format: 'esm' From 1d7379d64112dda0ac3163f2bdcb17bfef3728d0 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Thu, 10 May 2018 11:47:15 -0400 Subject: [PATCH 035/208] doc: fix stability text for n-api MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While some places list n-api as stable, the reference in doc/api/addons.md was missed. This fixes that instance. Fixes: https://github.com/nodejs/node/issues/20645 PR-URL: https://github.com/nodejs/node/pull/20659 Reviewed-By: Vse Mozhet Byt Reviewed-By: Luigi Pinca Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Trivikram Kamat Reviewed-By: Anna Henningsen Reviewed-By: Tobias Nießen --- doc/api/addons.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/addons.md b/doc/api/addons.md index a207a71b717604..afbbdd843cb223 100644 --- a/doc/api/addons.md +++ b/doc/api/addons.md @@ -219,7 +219,7 @@ illustration of how it can be used. ## N-API -> Stability: 1 - Experimental +> Stability: 2 - Stable N-API is an API for building native Addons. It is independent from the underlying JavaScript runtime (e.g. V8) and is maintained as part of From e06c5874f6cb0bfe93ffda555136e37ebaacb6a7 Mon Sep 17 00:00:00 2001 From: musgravejw Date: Sat, 5 May 2018 01:43:12 -0400 Subject: [PATCH 036/208] doc: add global node_modules to require.resolve() PR-URL: https://github.com/nodejs/node/pull/20534 Fixes: https://github.com/nodejs/node/issues/18926 Reviewed-By: Benjamin Gruenbaum Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat --- doc/api/modules.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/doc/api/modules.md b/doc/api/modules.md index c9b83859d99079..c46b30560da81f 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -175,7 +175,7 @@ LOAD_AS_DIRECTORY(X) 2. LOAD_INDEX(X) LOAD_NODE_MODULES(X, START) -1. let DIRS=NODE_MODULES_PATHS(START) +1. let DIRS = NODE_MODULES_PATHS(START) 2. for each DIR in DIRS: a. LOAD_AS_FILE(DIR/X) b. LOAD_AS_DIRECTORY(DIR/X) @@ -183,7 +183,7 @@ LOAD_NODE_MODULES(X, START) NODE_MODULES_PATHS(START) 1. let PARTS = path split(START) 2. let I = count of PARTS - 1 -3. let DIRS = [] +3. let DIRS = [GLOBAL_FOLDERS] 4. while I >= 0, a. if PARTS[I] = "node_modules" CONTINUE b. DIR = path join(PARTS[0 .. I] + "node_modules") @@ -419,7 +419,7 @@ when people are unaware that `NODE_PATH` must be set. Sometimes a module's dependencies change, causing a different version (or even a different module) to be loaded as the `NODE_PATH` is searched. -Additionally, Node.js will search in the following locations: +Additionally, Node.js will search in the following list of GLOBAL_FOLDERS: * 1: `$HOME/.node_modules` * 2: `$HOME/.node_libraries` @@ -649,9 +649,11 @@ changes: * `request` {string} The module path to resolve. * `options` {Object} * `paths` {string[]} Paths to resolve module location from. If present, these - paths are used instead of the default resolution paths. Note that each of - these paths is used as a starting point for the module resolution algorithm, - meaning that the `node_modules` hierarchy is checked from this location. + paths are used instead of the default resolution paths, with the exception + of [GLOBAL_FOLDERS][] like `$HOME/.node_modules`, which are always + included. Note that each of these paths is used as a starting point for + the module resolution algorithm, meaning that the `node_modules` hierarchy + is checked from this location. * Returns: {string} Use the internal `require()` machinery to look up the location of a module, @@ -891,6 +893,7 @@ const builtin = require('module').builtinModules; [`Error`]: errors.html#errors_class_error [`module` object]: #modules_the_module_object [`path.dirname()`]: path.html#path_path_dirname_path +[GLOBAL_FOLDERS]: #modules_loading_from_the_global_folders [exports shortcut]: #modules_exports_shortcut [module resolution]: #modules_all_together [module wrapper]: #modules_the_module_wrapper From de06115d18720755498fd871bccca837df024acb Mon Sep 17 00:00:00 2001 From: cjihrig Date: Wed, 9 May 2018 12:36:59 -0400 Subject: [PATCH 037/208] fs: make fs.promises non-enumerable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This prevents the experimental feature warning from being emitted in cases where fs.promises is not actually used. PR-URL: https://github.com/nodejs/node/pull/20632 Fixes: https://github.com/nodejs/node/issues/20504 Reviewed-By: Michaël Zasso Reviewed-By: James M Snell Reviewed-By: Сковорода Никита Андреевич Reviewed-By: Joyee Cheung Reviewed-By: Luigi Pinca Reviewed-By: Gus Caplan Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Rich Trott Reviewed-By: Daniel Bevenius --- lib/fs.js | 2 +- test/parallel/test-fs-promises.js | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index e9d878803ddffd..ed2a241e568cf5 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -77,7 +77,7 @@ let warn = true; Object.defineProperty(fs, 'promises', { configurable: true, - enumerable: true, + enumerable: false, get() { if (warn) { warn = false; diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index 43b5da700484ff..8f1d646bb0e01e 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -5,7 +5,8 @@ const assert = require('assert'); const tmpdir = require('../common/tmpdir'); const fixtures = require('../common/fixtures'); const path = require('path'); -const fsPromises = require('fs').promises; +const fs = require('fs'); +const fsPromises = fs.promises; const { access, chmod, @@ -38,6 +39,10 @@ const tmpDir = tmpdir.path; common.crashOnUnhandledRejection(); +// fs.promises should not be enumerable as long as it causes a warning to be +// emitted. +assert.strictEqual(Object.keys(fs).includes('promises'), false); + { access(__filename, 'r') .then(common.mustCall()) From de34cfad585c056553530ce60d9c6bd62d66f5d8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 6 May 2018 18:14:59 +0200 Subject: [PATCH 038/208] test: make sure linked lists are inspectable with defaults Make sure that a long singly-linked list can be passed to `util.inspect()` without causing a stack overflow. PR-URL: https://github.com/nodejs/node/pull/20017 Reviewed-By: Rich Trott Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Colin Ihrig Reviewed-By: Joyee Cheung --- test/parallel/test-util-inspect.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js index 5cd051299d7f06..ad5f738a85976b 100644 --- a/test/parallel/test-util-inspect.js +++ b/test/parallel/test-util-inspect.js @@ -1404,3 +1404,15 @@ util.inspect(process); const args = (function() { return arguments; })('a'); assert.strictEqual(util.inspect(args), "[Arguments] { '0': 'a' }"); } + +{ + // Test that a long linked list can be inspected without throwing an error. + const list = {}; + let head = list; + // The real cutoff value is closer to 1400 stack frames as of May 2018, + // but let's be generous here – even a linked listed of length 100k should be + // inspectable in some way. + for (let i = 0; i < 100000; i++) + head = head.next = {}; + util.inspect(list); +} From b6ea5df08a42bd1d9bf71d3b00ec1c684463a5ec Mon Sep 17 00:00:00 2001 From: David Goldstein Date: Tue, 10 Apr 2018 07:40:56 +0000 Subject: [PATCH 039/208] module: add --preserve-symlinks-main Add `--preserve-symlinks-main` option which behaves like `--preserve-symlinks` but for `require.main`. PR-URL: https://github.com/nodejs/node/pull/19911 Reviewed-By: James M Snell --- doc/api/cli.md | 23 ++++++++ doc/node.1 | 5 +- lib/internal/modules/cjs/loader.js | 17 +++++- lib/internal/modules/esm/default_resolve.js | 3 +- src/node.cc | 16 +++++- src/node_config.cc | 2 + src/node_internals.h | 5 ++ .../test-esm-preserve-symlinks-main.js | 57 +++++++++++++++++++ 8 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 test/es-module/test-esm-preserve-symlinks-main.js diff --git a/doc/api/cli.md b/doc/api/cli.md index 6d055d9d03a6dd..d76c49615c9bca 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -217,6 +217,29 @@ are linked from more than one location in the dependency tree (Node.js would see those as two separate modules and would attempt to load the module multiple times, causing an exception to be thrown). +The `--preserve-symlinks` flag does not apply to the main module, which allows +`node --preserve-symlinks node_module/.bin/` to work. To apply the same +behavior for the main module, also use `--preserve-symlinks-main`. + +### `--preserve-symlinks-main` + + +Instructs the module loader to preserve symbolic links when resolving and +caching the main module (`require.main`). + +This flag exists so that the main module can be opted-in to the same behavior +that `--preserve-symlinks` gives to all other imports; they are separate flags, +however, for backward compatibility with older Node.js versions. + +Note that `--preserve-symlinks-main` does not imply `--preserve-symlinks`; it +is expected that `--preserve-symlinks-main` will be used in addition to +`--preserve-symlinks` when it is not desirable to follow symlinks before +resolving relative paths. + +See `--preserve-symlinks` for more information. + ### `--prof-process` > Stability: 0 - Deprecated: Use [`crypto.createCipheriv()`][] instead. @@ -1331,7 +1336,9 @@ Creates and returns a `Cipher` object that uses the given `algorithm` and The `options` argument controls stream behavior and is optional except when a cipher in CCM mode is used (e.g. `'aes-128-ccm'`). In that case, the `authTagLength` option is required and specifies the length of the -authentication tag in bytes, see [CCM mode][]. +authentication tag in bytes, see [CCM mode][]. In GCM mode, the `authTagLength` +option is not required but can be used to set the length of the authentication +tag that will be returned by `getAuthTag()` and defaults to 16 bytes. The `algorithm` is dependent on OpenSSL, examples are `'aes192'`, etc. On recent OpenSSL releases, `openssl list -cipher-algorithms` @@ -1362,6 +1369,10 @@ Adversaries][] for details. +```C +NODE_EXTERN napi_status napi_add_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg); +``` + +Registers `fun` as a function to be run with the `arg` parameter once the +current Node.js environment exits. + +A function can safely be specified multiple times with different +`arg` values. In that case, it will be called multiple times as well. +Providing the same `fun` and `arg` values multiple times is not allowed +and will lead the process to abort. + +The hooks will be called in reverse order, i.e. the most recently added one +will be called first. + +Removing this hook can be done by using `napi_remove_env_cleanup_hook`. +Typically, that happens when the resource for which this hook was added +is being torn down anyway. + +#### napi_remove_env_cleanup_hook + +```C +NAPI_EXTERN napi_status napi_remove_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg); +``` + +Unregisters `fun` as a function to be run with the `arg` parameter once the +current Node.js environment exits. Both the argument and the function value +need to be exact matches. + +The function must have originally been registered +with `napi_add_env_cleanup_hook`, otherwise the process will abort. + ## Module registration N-API modules are registered in a manner similar to other modules except that instead of using the `NODE_MODULE` macro the following diff --git a/src/env-inl.h b/src/env-inl.h index 6202e50548a3ce..d3c0c211d97328 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -629,6 +629,29 @@ inline void Environment::SetTemplateMethod(v8::Local that, t->SetClassName(name_string); // NODE_SET_METHOD() compatibility. } +void Environment::AddCleanupHook(void (*fn)(void*), void* arg) { + auto insertion_info = cleanup_hooks_.emplace(CleanupHookCallback { + fn, arg, cleanup_hook_counter_++ + }); + // Make sure there was no existing element with these values. + CHECK_EQ(insertion_info.second, true); +} + +void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) { + CleanupHookCallback search { fn, arg, 0 }; + cleanup_hooks_.erase(search); +} + +size_t Environment::CleanupHookCallback::Hash::operator()( + const CleanupHookCallback& cb) const { + return std::hash()(cb.arg_); +} + +bool Environment::CleanupHookCallback::Equal::operator()( + const CleanupHookCallback& a, const CleanupHookCallback& b) const { + return a.fn_ == b.fn_ && a.arg_ == b.arg_; +} + #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) #define V(TypeName, PropertyName) \ diff --git a/src/env.cc b/src/env.cc index 08d719a51011d1..aadb81092e507c 100644 --- a/src/env.cc +++ b/src/env.cc @@ -305,6 +305,35 @@ void Environment::PrintSyncTrace() const { fflush(stderr); } +void Environment::RunCleanup() { + while (!cleanup_hooks_.empty()) { + // Copy into a vector, since we can't sort an unordered_set in-place. + std::vector callbacks( + cleanup_hooks_.begin(), cleanup_hooks_.end()); + // We can't erase the copied elements from `cleanup_hooks_` yet, because we + // need to be able to check whether they were un-scheduled by another hook. + + std::sort(callbacks.begin(), callbacks.end(), + [](const CleanupHookCallback& a, const CleanupHookCallback& b) { + // Sort in descending order so that the most recently inserted callbacks + // are run first. + return a.insertion_order_counter_ > b.insertion_order_counter_; + }); + + for (const CleanupHookCallback& cb : callbacks) { + if (cleanup_hooks_.count(cb) == 0) { + // This hook was removed from the `cleanup_hooks_` set during another + // hook that was run earlier. Nothing to do here. + continue; + } + + cb.fn_(cb.arg_); + cleanup_hooks_.erase(cb); + CleanupHandles(); + } + } +} + void Environment::RunBeforeExitCallbacks() { for (ExitCallback before_exit : before_exit_functions_) { before_exit.cb_(before_exit.arg_); diff --git a/src/env.h b/src/env.h index c0d79883d0ff1c..3acb27c9545525 100644 --- a/src/env.h +++ b/src/env.h @@ -42,6 +42,7 @@ #include #include #include +#include struct nghttp2_rcbuf; @@ -775,6 +776,10 @@ class Environment { v8::Local GetNow(); + inline void AddCleanupHook(void (*fn)(void*), void* arg); + inline void RemoveCleanupHook(void (*fn)(void*), void* arg); + void RunCleanup(); + private: inline void CreateImmediate(native_immediate_callback cb, void* data, @@ -863,6 +868,32 @@ class Environment { void RunAndClearNativeImmediates(); static void CheckImmediate(uv_check_t* handle); + struct CleanupHookCallback { + void (*fn_)(void*); + void* arg_; + + // We keep track of the insertion order for these objects, so that we can + // call the callbacks in reverse order when we are cleaning up. + uint64_t insertion_order_counter_; + + // Only hashes `arg_`, since that is usually enough to identify the hook. + struct Hash { + inline size_t operator()(const CleanupHookCallback& cb) const; + }; + + // Compares by `fn_` and `arg_` being equal. + struct Equal { + inline bool operator()(const CleanupHookCallback& a, + const CleanupHookCallback& b) const; + }; + }; + + // Use an unordered_set, so that we have efficient insertion and removal. + std::unordered_set cleanup_hooks_; + uint64_t cleanup_hook_counter_ = 0; + static void EnvPromiseHook(v8::PromiseHookType type, v8::Local promise, v8::Local parent); diff --git a/src/node.cc b/src/node.cc index 09b1e34664c14c..06acd1c74bc8ed 100644 --- a/src/node.cc +++ b/src/node.cc @@ -908,6 +908,22 @@ void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) { env->AddPromiseHook(fn, arg); } +void AddEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg) { + Environment* env = Environment::GetCurrent(isolate); + env->AddCleanupHook(fun, arg); +} + + +void RemoveEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg) { + Environment* env = Environment::GetCurrent(isolate); + env->RemoveCleanupHook(fun, arg); +} + + CallbackScope::CallbackScope(Isolate* isolate, Local object, async_context asyncContext) @@ -4463,7 +4479,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data, void FreeEnvironment(Environment* env) { - env->CleanupHandles(); + env->RunCleanup(); delete env; } @@ -4561,6 +4577,8 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, env.set_trace_sync_io(false); const int exit_code = EmitExit(&env); + + env.RunCleanup(); RunAtExit(&env); v8_platform.DrainVMTasks(isolate); diff --git a/src/node.h b/src/node.h index 5a491c1abf5457..23e2e9995ce209 100644 --- a/src/node.h +++ b/src/node.h @@ -583,6 +583,19 @@ NODE_EXTERN void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg); +/* This is a lot like node::AtExit, except that the hooks added via this + * function are run before the AtExit ones and will always be registered + * for the current Environment instance. + * These functions are safe to use in an addon supporting multiple + * threads/isolates. */ +NODE_EXTERN void AddEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg); + +NODE_EXTERN void RemoveEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg); + /* Returns the id of the current execution context. If the return value is * zero then no execution has been set. This will happen if the user handles * I/O from native code. */ diff --git a/src/node_api.cc b/src/node_api.cc index 4878cf241edc04..d5437d70d933ed 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -902,6 +902,28 @@ void napi_module_register(napi_module* mod) { node::node_module_register(nm); } +napi_status napi_add_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg) { + CHECK_ENV(env); + CHECK_ARG(env, fun); + + node::AddEnvironmentCleanupHook(env->isolate, fun, arg); + + return napi_ok; +} + +napi_status napi_remove_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg) { + CHECK_ENV(env); + CHECK_ARG(env, fun); + + node::RemoveEnvironmentCleanupHook(env->isolate, fun, arg); + + return napi_ok; +} + // Warning: Keep in-sync with napi_status enum static const char* error_messages[] = {nullptr, diff --git a/src/node_api.h b/src/node_api.h index b010d32db7b086..91c2775a03ed76 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -118,6 +118,13 @@ EXTERN_C_START NAPI_EXTERN void napi_module_register(napi_module* mod); +NAPI_EXTERN napi_status napi_add_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg); +NAPI_EXTERN napi_status napi_remove_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg); + NAPI_EXTERN napi_status napi_get_last_error_info(napi_env env, const napi_extended_error_info** result); diff --git a/test/addons-napi/test_cleanup_hook/binding.cc b/test/addons-napi/test_cleanup_hook/binding.cc new file mode 100644 index 00000000000000..66d53508c69f13 --- /dev/null +++ b/test/addons-napi/test_cleanup_hook/binding.cc @@ -0,0 +1,24 @@ +#include "node_api.h" +#include "uv.h" +#include "../common.h" + +namespace { + +void cleanup(void* arg) { + printf("cleanup(%d)\n", *static_cast(arg)); +} + +int secret = 42; +int wrong_secret = 17; + +napi_value Init(napi_env env, napi_value exports) { + napi_add_env_cleanup_hook(env, cleanup, &wrong_secret); + napi_add_env_cleanup_hook(env, cleanup, &secret); + napi_remove_env_cleanup_hook(env, cleanup, &wrong_secret); + + return nullptr; +} + +} // anonymous namespace + +NAPI_MODULE(NODE_GYP_MODULE_NAME, Init) diff --git a/test/addons-napi/test_cleanup_hook/binding.gyp b/test/addons-napi/test_cleanup_hook/binding.gyp new file mode 100644 index 00000000000000..7ede63d94a0d77 --- /dev/null +++ b/test/addons-napi/test_cleanup_hook/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'defines': [ 'V8_DEPRECATION_WARNINGS=1' ], + 'sources': [ 'binding.cc' ] + } + ] +} diff --git a/test/addons-napi/test_cleanup_hook/test.js b/test/addons-napi/test_cleanup_hook/test.js new file mode 100644 index 00000000000000..354f4449045c17 --- /dev/null +++ b/test/addons-napi/test_cleanup_hook/test.js @@ -0,0 +1,12 @@ +'use strict'; +const common = require('../../common'); +const assert = require('assert'); +const child_process = require('child_process'); + +if (process.argv[2] === 'child') { + require(`./build/${common.buildType}/binding`); +} else { + const { stdout } = + child_process.spawnSync(process.execPath, [__filename, 'child']); + assert.strictEqual(stdout.toString().trim(), 'cleanup(42)'); +} From e253edb48a154aeb5464d342b1db5a8e8fda6add Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 4 Sep 2017 22:02:55 +0200 Subject: [PATCH 053/208] src: make CleanupHandles() tear down handles/reqs Previously, handles would not be closed when the current `Environment` stopped, which is acceptable in a single-`Environment`-per-process situation, but would otherwise create memory and file descriptor leaks. Also, introduce a generic way to close handles via the `Environment::CloseHandle()` function, which automatically keeps track of whether a close callback has been called yet or not. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: https://github.com/ayojs/ayo/pull/85 PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/cares_wrap.cc | 18 ++++++------------ src/connection_wrap.h | 2 -- src/env-inl.h | 22 ++++++++++++++++++++-- src/env.cc | 20 ++++++++++++-------- src/env.h | 6 +++++- src/fs_event_wrap.cc | 6 ++++-- src/handle_wrap.cc | 37 +++++++++++++++++++++++++------------ src/handle_wrap.h | 6 ++++++ src/node_stat_watcher.cc | 7 +------ src/process_wrap.cc | 2 ++ src/req_wrap-inl.h | 6 ++++++ src/req_wrap.h | 1 + src/tty_wrap.cc | 2 ++ 13 files changed, 90 insertions(+), 45 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 4208c02d4fe445..ae253d40ca94b2 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -267,9 +267,8 @@ void ares_poll_cb(uv_poll_t* watcher, int status, int events) { } -void ares_poll_close_cb(uv_handle_t* watcher) { - node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher, - reinterpret_cast(watcher)); +void ares_poll_close_cb(uv_poll_t* watcher) { + node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher, watcher); free(task); } @@ -347,8 +346,7 @@ void ares_sockstate_cb(void* data, "When an ares socket is closed we should have a handle for it"); channel->task_list()->erase(it); - uv_close(reinterpret_cast(&task->poll_watcher), - ares_poll_close_cb); + channel->env()->CloseHandle(&task->poll_watcher, ares_poll_close_cb); if (channel->task_list()->empty()) { uv_timer_stop(channel->timer_handle()); @@ -517,10 +515,7 @@ ChannelWrap::~ChannelWrap() { void ChannelWrap::CleanupTimer() { if (timer_handle_ == nullptr) return; - uv_close(reinterpret_cast(timer_handle_), - [](uv_handle_t* handle) { - delete reinterpret_cast(handle); - }); + env()->CloseHandle(timer_handle_, [](uv_timer_t* handle){ delete handle; }); timer_handle_ = nullptr; } @@ -610,8 +605,7 @@ class QueryWrap : public AsyncWrap { static_cast(this)); } - static void CaresAsyncClose(uv_handle_t* handle) { - uv_async_t* async = reinterpret_cast(handle); + static void CaresAsyncClose(uv_async_t* async) { auto data = static_cast(async->data); delete data->wrap; delete data; @@ -636,7 +630,7 @@ class QueryWrap : public AsyncWrap { free(host); } - uv_close(reinterpret_cast(handle), CaresAsyncClose); + wrap->env()->CloseHandle(handle, CaresAsyncClose); } static void Callback(void *arg, int status, int timeouts, diff --git a/src/connection_wrap.h b/src/connection_wrap.h index afb168c614aa97..72030a00901daf 100644 --- a/src/connection_wrap.h +++ b/src/connection_wrap.h @@ -23,8 +23,6 @@ class ConnectionWrap : public LibuvStreamWrap { ConnectionWrap(Environment* env, v8::Local object, ProviderType provider); - ~ConnectionWrap() { - } UVType handle_; }; diff --git a/src/env-inl.h b/src/env-inl.h index d3c0c211d97328..917ddd1b6bcb75 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -349,8 +349,26 @@ inline void Environment::RegisterHandleCleanup(uv_handle_t* handle, handle_cleanup_queue_.push_back(HandleCleanup{handle, cb, arg}); } -inline void Environment::FinishHandleCleanup(uv_handle_t* handle) { - handle_cleanup_waiting_--; +template +inline void Environment::CloseHandle(T* handle, OnCloseCallback callback) { + handle_cleanup_waiting_++; + static_assert(sizeof(T) >= sizeof(uv_handle_t), "T is a libuv handle"); + static_assert(offsetof(T, data) == offsetof(uv_handle_t, data), + "T is a libuv handle"); + static_assert(offsetof(T, close_cb) == offsetof(uv_handle_t, close_cb), + "T is a libuv handle"); + struct CloseData { + Environment* env; + OnCloseCallback callback; + void* original_data; + }; + handle->data = new CloseData { this, callback, handle->data }; + uv_close(reinterpret_cast(handle), [](uv_handle_t* handle) { + std::unique_ptr data { static_cast(handle->data) }; + data->env->handle_cleanup_waiting_--; + handle->data = data->original_data; + data->callback(reinterpret_cast(handle)); + }); } inline uv_loop_t* Environment::event_loop() const { diff --git a/src/env.cc b/src/env.cc index aadb81092e507c..6526c680ac1792 100644 --- a/src/env.cc +++ b/src/env.cc @@ -209,9 +209,7 @@ void Environment::RegisterHandleCleanups() { void* arg) { handle->data = env; - uv_close(handle, [](uv_handle_t* handle) { - static_cast(handle->data)->FinishHandleCleanup(handle); - }); + env->CloseHandle(handle, [](uv_handle_t* handle) {}); }; RegisterHandleCleanup( @@ -233,13 +231,17 @@ void Environment::RegisterHandleCleanups() { } void Environment::CleanupHandles() { - for (HandleCleanup& hc : handle_cleanup_queue_) { - handle_cleanup_waiting_++; + for (ReqWrap* request : req_wrap_queue_) + request->Cancel(); + + for (HandleWrap* handle : handle_wrap_queue_) + handle->Close(); + + for (HandleCleanup& hc : handle_cleanup_queue_) hc.cb_(this, hc.handle_, hc.arg_); - } handle_cleanup_queue_.clear(); - while (handle_cleanup_waiting_ != 0) + while (handle_cleanup_waiting_ != 0 || !handle_wrap_queue_.IsEmpty()) uv_run(event_loop(), UV_RUN_ONCE); } @@ -306,6 +308,8 @@ void Environment::PrintSyncTrace() const { } void Environment::RunCleanup() { + CleanupHandles(); + while (!cleanup_hooks_.empty()) { // Copy into a vector, since we can't sort an unordered_set in-place. std::vector callbacks( @@ -329,8 +333,8 @@ void Environment::RunCleanup() { cb.fn_(cb.arg_); cleanup_hooks_.erase(cb); - CleanupHandles(); } + CleanupHandles(); } } diff --git a/src/env.h b/src/env.h index 3acb27c9545525..79351666c1182e 100644 --- a/src/env.h +++ b/src/env.h @@ -577,10 +577,14 @@ class Environment { void RegisterHandleCleanups(); void CleanupHandles(); + + // Register clean-up cb to be called on environment destruction. inline void RegisterHandleCleanup(uv_handle_t* handle, HandleCleanupCb cb, void *arg); - inline void FinishHandleCleanup(uv_handle_t* handle); + + template + inline void CloseHandle(T* handle, OnCloseCallback callback); inline void AssignToContext(v8::Local context, const ContextInfo& info); diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index ed74f36719db79..579e446fc5c485 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -78,11 +78,12 @@ FSEventWrap::FSEventWrap(Environment* env, Local object) : HandleWrap(env, object, reinterpret_cast(&handle_), - AsyncWrap::PROVIDER_FSEVENTWRAP) {} + AsyncWrap::PROVIDER_FSEVENTWRAP) { + MarkAsUninitialized(); +} FSEventWrap::~FSEventWrap() { - CHECK_EQ(initialized_, false); } void FSEventWrap::GetInitialized(const FunctionCallbackInfo& args) { @@ -153,6 +154,7 @@ void FSEventWrap::Start(const FunctionCallbackInfo& args) { } err = uv_fs_event_start(&wrap->handle_, OnEvent, *path, flags); + wrap->MarkAsInitialized(); wrap->initialized_ = true; if (err != 0) { diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 49bf0c55bea0a1..20356b94a5775a 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -61,29 +61,40 @@ void HandleWrap::HasRef(const FunctionCallbackInfo& args) { void HandleWrap::Close(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - HandleWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - // Guard against uninitialized handle or double close. - if (!IsAlive(wrap)) - return; + wrap->Close(args[0]); +} - if (wrap->state_ != kInitialized) +void HandleWrap::Close(v8::Local close_callback) { + if (state_ != kInitialized) return; - CHECK_EQ(false, wrap->persistent().IsEmpty()); - uv_close(wrap->handle_, OnClose); - wrap->state_ = kClosing; + CHECK_EQ(false, persistent().IsEmpty()); + uv_close(handle_, OnClose); + state_ = kClosing; - if (args[0]->IsFunction()) { - wrap->object()->Set(env->onclose_string(), args[0]); - wrap->state_ = kClosingWithCallback; + if (!close_callback.IsEmpty() && close_callback->IsFunction()) { + object()->Set(env()->context(), env()->onclose_string(), close_callback) + .FromJust(); + state_ = kClosingWithCallback; } } +void HandleWrap::MarkAsInitialized() { + env()->handle_wrap_queue()->PushBack(this); + state_ = kInitialized; +} + + +void HandleWrap::MarkAsUninitialized() { + handle_wrap_queue_.Remove(); + state_ = kClosed; +} + + HandleWrap::HandleWrap(Environment* env, Local object, uv_handle_t* handle, @@ -110,6 +121,8 @@ void HandleWrap::OnClose(uv_handle_t* handle) { const bool have_close_callback = (wrap->state_ == kClosingWithCallback); wrap->state_ = kClosed; + wrap->OnClose(); + if (have_close_callback) wrap->MakeCallback(env->onclose_string(), 0, nullptr); diff --git a/src/handle_wrap.h b/src/handle_wrap.h index e7a335f5140253..fd2d002dce0338 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -70,11 +70,17 @@ class HandleWrap : public AsyncWrap { inline uv_handle_t* GetHandle() const { return handle_; } + void Close(v8::Local close_callback = v8::Local()); + protected: HandleWrap(Environment* env, v8::Local object, uv_handle_t* handle, AsyncWrap::ProviderType provider); + virtual void OnClose() {} + + void MarkAsInitialized(); + void MarkAsUninitialized(); private: friend class Environment; diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index a2cfb1088c9d25..d8f8a6a362237d 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -75,11 +75,6 @@ void StatWatcher::Initialize(Environment* env, Local target) { } -static void Delete(uv_handle_t* handle) { - delete reinterpret_cast(handle); -} - - StatWatcher::StatWatcher(Environment* env, Local wrap) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_STATWATCHER), watcher_(new uv_fs_poll_t) { @@ -93,7 +88,7 @@ StatWatcher::~StatWatcher() { if (IsActive()) { Stop(); } - uv_close(reinterpret_cast(watcher_), Delete); + env()->CloseHandle(watcher_, [](uv_fs_poll_t* handle) { delete handle; }); } diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 96d60cc900583c..6d421fe7c4d4de 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -88,6 +88,7 @@ class ProcessWrap : public HandleWrap { object, reinterpret_cast(&process_), AsyncWrap::PROVIDER_PROCESSWRAP) { + MarkAsUninitialized(); } static void ParseStdioOptions(Environment* env, @@ -256,6 +257,7 @@ class ProcessWrap : public HandleWrap { } int err = uv_spawn(env->event_loop(), &wrap->process_, &options); + wrap->MarkAsInitialized(); if (err == 0) { CHECK_EQ(wrap->process_.data, wrap); diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index 11b1389fa0e771..ab5051e41d8e89 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -7,6 +7,7 @@ #include "async_wrap-inl.h" #include "env-inl.h" #include "util-inl.h" +#include "uv.h" namespace node { @@ -37,6 +38,11 @@ ReqWrap* ReqWrap::from_req(T* req) { return ContainerOf(&ReqWrap::req_, req); } +template +void ReqWrap::Cancel() { + uv_cancel(reinterpret_cast(&req_)); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/req_wrap.h b/src/req_wrap.h index 656be38dcea943..4d6a89d743c591 100644 --- a/src/req_wrap.h +++ b/src/req_wrap.h @@ -19,6 +19,7 @@ class ReqWrap : public AsyncWrap { inline ~ReqWrap() override; inline void Dispatched(); // Call this after the req has been dispatched. T* req() { return &req_; } + inline void Cancel(); static ReqWrap* from_req(T* req); diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index c5abc6bf9b9b91..d01caba4a558f2 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -172,6 +172,8 @@ TTYWrap::TTYWrap(Environment* env, reinterpret_cast(&handle_), AsyncWrap::PROVIDER_TTYWRAP) { *init_err = uv_tty_init(env->event_loop(), &handle_, fd, readable); + if (*init_err != 0) + MarkAsUninitialized(); } } // namespace node From 75aad9069b857a4778250d15f7a4f1ca3dc54467 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 25 Sep 2017 21:16:02 +0200 Subject: [PATCH 054/208] src: unify ReqWrap libuv calling This allows easier tracking of whether there are active `ReqWrap`s. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: https://github.com/ayojs/ayo/pull/85 PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/cares_wrap.cc | 24 +++++------ src/node_file.cc | 40 ++++++++--------- src/pipe_wrap.cc | 9 ++-- src/req_wrap-inl.h | 104 +++++++++++++++++++++++++++++++++++++++++++++ src/req_wrap.h | 13 +++++- src/tcp_wrap.cc | 18 ++++---- src/udp_wrap.cc | 13 +++--- 7 files changed, 165 insertions(+), 56 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index ae253d40ca94b2..f829eb2b01d5c9 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -515,7 +515,7 @@ ChannelWrap::~ChannelWrap() { void ChannelWrap::CleanupTimer() { if (timer_handle_ == nullptr) return; - env()->CloseHandle(timer_handle_, [](uv_timer_t* handle){ delete handle; }); + env()->CloseHandle(timer_handle_, [](uv_timer_t* handle) { delete handle; }); timer_handle_ = nullptr; } @@ -1927,13 +1927,11 @@ void GetAddrInfo(const FunctionCallbackInfo& args) { hints.ai_socktype = SOCK_STREAM; hints.ai_flags = flags; - int err = uv_getaddrinfo(env->event_loop(), - req_wrap->req(), - AfterGetAddrInfo, - *hostname, - nullptr, - &hints); - req_wrap->Dispatched(); + int err = req_wrap->Dispatch(uv_getaddrinfo, + AfterGetAddrInfo, + *hostname, + nullptr, + &hints); if (err) delete req_wrap; @@ -1957,12 +1955,10 @@ void GetNameInfo(const FunctionCallbackInfo& args) { GetNameInfoReqWrap* req_wrap = new GetNameInfoReqWrap(env, req_wrap_obj); - int err = uv_getnameinfo(env->event_loop(), - req_wrap->req(), - AfterGetNameInfo, - (struct sockaddr*)&addr, - NI_NAMEREQD); - req_wrap->Dispatched(); + int err = req_wrap->Dispatch(uv_getnameinfo, + AfterGetNameInfo, + reinterpret_cast(&addr), + NI_NAMEREQD); if (err) delete req_wrap; diff --git a/src/node_file.cc b/src/node_file.cc index 97b957eed66b31..713dcbf6331898 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -89,6 +89,11 @@ using v8::Value; TRACE_EVENT_END(TRACING_CATEGORY_NODE2(fs, sync), TRACE_NAME(syscall), \ ##__VA_ARGS__); +// We sometimes need to convert a C++ lambda function to a raw C-style function. +// This is helpful, because ReqWrap::Dispatch() does not recognize lambda +// functions, and thus does not wrap them properly. +typedef void(*uv_fs_callback_t)(uv_fs_t*); + // The FileHandle object wraps a file descriptor and will close it on garbage // collection if necessary. If that happens, a process warning will be // emitted (or a fatal exception will occur if the fd cannot be closed.) @@ -216,7 +221,7 @@ inline MaybeLocal FileHandle::ClosePromise() { if (!closed_ && !closing_) { closing_ = true; CloseReq* req = new CloseReq(env(), promise, object()); - auto AfterClose = [](uv_fs_t* req) { + auto AfterClose = uv_fs_callback_t{[](uv_fs_t* req) { CloseReq* close = static_cast(req->data); CHECK_NE(close, nullptr); close->file_handle()->AfterClose(); @@ -227,9 +232,8 @@ inline MaybeLocal FileHandle::ClosePromise() { close->Resolve(); } delete close; - }; - req->Dispatched(); - int ret = uv_fs_close(env()->event_loop(), req->req(), fd_, AfterClose); + }}; + int ret = req->Dispatch(uv_fs_close, fd_, AfterClose); if (ret < 0) { req->Reject(UVException(isolate, ret, "close")); delete req; @@ -309,17 +313,15 @@ int FileHandle::ReadStart() { recommended_read = read_length_; read_wrap->buffer_ = EmitAlloc(recommended_read); - read_wrap->Dispatched(); current_read_ = std::move(read_wrap); - uv_fs_read(env()->event_loop(), - current_read_->req(), - fd_, - ¤t_read_->buffer_, - 1, - read_offset_, - [](uv_fs_t* req) { + current_read_->Dispatch(uv_fs_read, + fd_, + ¤t_read_->buffer_, + 1, + read_offset_, + uv_fs_callback_t{[](uv_fs_t* req) { FileHandle* handle; { FileHandleReadWrap* req_wrap = FileHandleReadWrap::from_req(req); @@ -342,8 +344,10 @@ int FileHandle::ReadStart() { // once we’re exiting the current scope. constexpr size_t wanted_freelist_fill = 100; auto& freelist = handle->env()->file_handle_read_wrap_freelist(); - if (freelist.size() < wanted_freelist_fill) + if (freelist.size() < wanted_freelist_fill) { + read_wrap->Reset(); freelist.emplace_back(std::move(read_wrap)); + } if (result >= 0) { // Read at most as many bytes as we originally planned to. @@ -370,7 +374,7 @@ int FileHandle::ReadStart() { // Start over, if EmitRead() didn’t tell us to stop. if (handle->reading_) handle->ReadStart(); - }); + }}); return 0; } @@ -389,8 +393,7 @@ ShutdownWrap* FileHandle::CreateShutdownWrap(Local object) { int FileHandle::DoShutdown(ShutdownWrap* req_wrap) { FileHandleCloseWrap* wrap = static_cast(req_wrap); closing_ = true; - wrap->Dispatched(); - uv_fs_close(env()->event_loop(), wrap->req(), fd_, [](uv_fs_t* req) { + wrap->Dispatch(uv_fs_close, fd_, uv_fs_callback_t{[](uv_fs_t* req) { FileHandleCloseWrap* wrap = static_cast( FileHandleCloseWrap::from_req(req)); FileHandle* handle = static_cast(wrap->stream()); @@ -399,7 +402,7 @@ int FileHandle::DoShutdown(ShutdownWrap* req_wrap) { int result = req->result; uv_fs_req_cleanup(req); wrap->Done(result); - }); + }}); return 0; } @@ -616,8 +619,7 @@ inline FSReqBase* AsyncDestCall(Environment* env, enum encoding enc, uv_fs_cb after, Func fn, Args... fn_args) { 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(); + int err = req_wrap->Dispatch(fn, fn_args..., after); if (err < 0) { uv_fs_t* uv_req = req_wrap->req(); uv_req->result = err; diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index da7cb9e3ab55ba..7ec5bdf15be9cc 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -224,11 +224,10 @@ void PipeWrap::Connect(const FunctionCallbackInfo& args) { ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_PIPECONNECTWRAP); - uv_pipe_connect(req_wrap->req(), - &wrap->handle_, - *name, - AfterConnect); - req_wrap->Dispatched(); + req_wrap->Dispatch(uv_pipe_connect, + &wrap->handle_, + *name, + AfterConnect); args.GetReturnValue().Set(0); // uv_pipe_connect() doesn't return errors. } diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index ab5051e41d8e89..54abf74430f6af 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -43,6 +43,110 @@ void ReqWrap::Cancel() { uv_cancel(reinterpret_cast(&req_)); } +// Below is dark template magic designed to invoke libuv functions that +// initialize uv_req_t instances in a unified fashion, to allow easier +// tracking of active/inactive requests. + +// Invoke a generic libuv function that initializes uv_req_t instances. +// This is, unfortunately, necessary since they come in three different +// variants that can not all be invoked in the same way: +// - int uv_foo(uv_loop_t* loop, uv_req_t* request, ...); +// - int uv_foo(uv_req_t* request, ...); +// - void uv_foo(uv_req_t* request, ...); +template +struct CallLibuvFunction; + +// Detect `int uv_foo(uv_loop_t* loop, uv_req_t* request, ...);`. +template +struct CallLibuvFunction { + using T = int(*)(uv_loop_t*, ReqT*, Args...); + template + static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { + return fn(loop, req, args...); + } +}; + +// Detect `int uv_foo(uv_req_t* request, ...);`. +template +struct CallLibuvFunction { + using T = int(*)(ReqT*, Args...); + template + static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { + return fn(req, args...); + } +}; + +// Detect `void uv_foo(uv_req_t* request, ...);`. +template +struct CallLibuvFunction { + using T = void(*)(ReqT*, Args...); + template + static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { + fn(req, args...); + return 0; + } +}; + +// This is slightly darker magic: This template is 'applied' to each parameter +// passed to the libuv function. If the parameter type (aka `T`) is a +// function type, it is assumed that this it is the request callback, and a +// wrapper that calls the original callback is created. +// If not, the parameter is passed through verbatim. +template +struct MakeLibuvRequestCallback { + static T For(ReqWrap* req_wrap, T v) { + static_assert(!std::is_function::value, + "MakeLibuvRequestCallback missed a callback"); + return v; + } +}; + +// Match the `void callback(uv_req_t*, ...);` signature that all libuv +// callbacks use. +template +struct MakeLibuvRequestCallback { + using F = void(*)(ReqT* req, Args... args); + + static void Wrapper(ReqT* req, Args... args) { + ReqWrap* req_wrap = ContainerOf(&ReqWrap::req_, req); + F original_callback = reinterpret_cast(req_wrap->original_callback_); + original_callback(req, args...); + } + + static F For(ReqWrap* req_wrap, F v) { + CHECK_EQ(req_wrap->original_callback_, nullptr); + req_wrap->original_callback_ = + reinterpret_cast::callback_t>(v); + return Wrapper; + } +}; + +template +template +int ReqWrap::Dispatch(LibuvFunction fn, Args... args) { + Dispatched(); + + // This expands as: + // + // return fn(env()->event_loop(), req(), arg1, arg2, Wrapper, arg3, ...) + // ^ ^ ^ + // | | | + // \-- Omitted if `fn` has no | | + // first `uv_loop_t*` argument | | + // | | + // A function callback whose first argument | | + // matches the libuv request type is replaced ---/ | + // by the `Wrapper` method defined above | + // | + // Other (non-function) arguments are passed -----/ + // through verbatim + return CallLibuvFunction::Call( + fn, + env()->event_loop(), + req(), + MakeLibuvRequestCallback::For(this, args)...); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/req_wrap.h b/src/req_wrap.h index 4d6a89d743c591..d181817218227b 100644 --- a/src/req_wrap.h +++ b/src/req_wrap.h @@ -17,17 +17,28 @@ class ReqWrap : public AsyncWrap { v8::Local object, AsyncWrap::ProviderType provider); inline ~ReqWrap() override; - inline void Dispatched(); // Call this after the req has been dispatched. + // Call this after the req has been dispatched, if that did not already + // happen by using Dispatch(). + inline void Dispatched(); T* req() { return &req_; } inline void Cancel(); static ReqWrap* from_req(T* req); + template + inline int Dispatch(LibuvFunction fn, Args... args); + private: friend class Environment; friend int GenDebugSymbols(); + template + friend struct MakeLibuvRequestCallback; + ListNode req_wrap_queue_; + typedef void (*callback_t)(); + callback_t original_callback_ = nullptr; + protected: // req_wrap_queue_ needs to be at a fixed offset from the start of the class // because it is used by ContainerOf to calculate the address of the embedding diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index 3ccd157159c287..70c60fa47cc68f 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -287,11 +287,10 @@ void TCPWrap::Connect(const FunctionCallbackInfo& args) { AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(wrap); ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP); - err = uv_tcp_connect(req_wrap->req(), - &wrap->handle_, - reinterpret_cast(&addr), - AfterConnect); - req_wrap->Dispatched(); + err = req_wrap->Dispatch(uv_tcp_connect, + &wrap->handle_, + reinterpret_cast(&addr), + AfterConnect); if (err) delete req_wrap; } @@ -323,11 +322,10 @@ void TCPWrap::Connect6(const FunctionCallbackInfo& args) { AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(wrap); ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP); - err = uv_tcp_connect(req_wrap->req(), - &wrap->handle_, - reinterpret_cast(&addr), - AfterConnect); - req_wrap->Dispatched(); + err = req_wrap->Dispatch(uv_tcp_connect, + &wrap->handle_, + reinterpret_cast(&addr), + AfterConnect); if (err) delete req_wrap; } diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 414fe07eab6da8..1d1ded449bd221 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -380,15 +380,14 @@ void UDPWrap::DoSend(const FunctionCallbackInfo& args, int family) { } if (err == 0) { - err = uv_udp_send(req_wrap->req(), - &wrap->handle_, - *bufs, - count, - reinterpret_cast(&addr), - OnSend); + err = req_wrap->Dispatch(uv_udp_send, + &wrap->handle_, + *bufs, + count, + reinterpret_cast(&addr), + OnSend); } - req_wrap->Dispatched(); if (err) delete req_wrap; From 89954087482d0e42c94cb90796ef4e5fa0de87fe Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 25 Sep 2017 22:53:17 +0200 Subject: [PATCH 055/208] src: keep track of open requests Workers cannot shut down while requests are open, so keep a counter that is increased whenever libuv requests are made and decreased whenever their callback is called. This also applies to other embedders, who may want to shut down an `Environment` instance early. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Fixes: https://github.com/nodejs/node/issues/20517 Refs: https://github.com/ayojs/ayo/pull/85 PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/env-inl.h | 9 +++++++++ src/env.cc | 6 ++++-- src/env.h | 6 +++++- src/node_api.cc | 5 +++++ src/node_crypto.cc | 13 +++++++++++-- src/node_zlib.cc | 13 ++++++++++--- src/req_wrap-inl.h | 43 ++++++++++++++++++++++++++++--------------- src/req_wrap.h | 2 ++ src/util.h | 10 +++++++++- 9 files changed, 83 insertions(+), 24 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 917ddd1b6bcb75..f115656353cff3 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -371,6 +371,15 @@ inline void Environment::CloseHandle(T* handle, OnCloseCallback callback) { }); } +void Environment::IncreaseWaitingRequestCounter() { + request_waiting_++; +} + +void Environment::DecreaseWaitingRequestCounter() { + request_waiting_--; + CHECK_GE(request_waiting_, 0); +} + inline uv_loop_t* Environment::event_loop() const { return isolate_data()->event_loop(); } diff --git a/src/env.cc b/src/env.cc index 6526c680ac1792..e5b9c0fd6aad4e 100644 --- a/src/env.cc +++ b/src/env.cc @@ -107,7 +107,6 @@ Environment::Environment(IsolateData* isolate_data, #if HAVE_INSPECTOR inspector_agent_(new inspector::Agent(this)), #endif - handle_cleanup_waiting_(0), http_parser_buffer_(nullptr), fs_stats_field_array_(isolate_, kFsStatsFieldsLength * 2), context_(context->GetIsolate(), context) { @@ -241,8 +240,11 @@ void Environment::CleanupHandles() { hc.cb_(this, hc.handle_, hc.arg_); handle_cleanup_queue_.clear(); - while (handle_cleanup_waiting_ != 0 || !handle_wrap_queue_.IsEmpty()) + while (handle_cleanup_waiting_ != 0 || + request_waiting_ != 0 || + !handle_wrap_queue_.IsEmpty()) { uv_run(event_loop(), UV_RUN_ONCE); + } } void Environment::StartProfilerIdleNotifier() { diff --git a/src/env.h b/src/env.h index 79351666c1182e..de3014249ea852 100644 --- a/src/env.h +++ b/src/env.h @@ -601,6 +601,9 @@ class Environment { inline uv_check_t* immediate_check_handle(); inline uv_idle_t* immediate_idle_handle(); + inline void IncreaseWaitingRequestCounter(); + inline void DecreaseWaitingRequestCounter(); + inline AsyncHooks* async_hooks(); inline ImmediateInfo* immediate_info(); inline TickInfo* tick_info(); @@ -833,7 +836,8 @@ class Environment { HandleWrapQueue handle_wrap_queue_; ReqWrapQueue req_wrap_queue_; std::list handle_cleanup_queue_; - int handle_cleanup_waiting_; + int handle_cleanup_waiting_ = 0; + int request_waiting_ = 0; double* heap_statistics_buffer_ = nullptr; double* heap_space_statistics_buffer_ = nullptr; diff --git a/src/node_api.cc b/src/node_api.cc index d5437d70d933ed..91a47a12d92751 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -3388,6 +3388,9 @@ class Work : public node::AsyncResource { // Establish a handle scope here so that every callback doesn't have to. // Also it is needed for the exception-handling below. v8::HandleScope scope(env->isolate); + node::Environment* env_ = node::Environment::GetCurrent(env->isolate); + env_->DecreaseWaitingRequestCounter(); + CallbackScope callback_scope(work); NAPI_CALL_INTO_MODULE(env, @@ -3488,6 +3491,8 @@ napi_status napi_queue_async_work(napi_env env, napi_async_work work) { uvimpl::Work* w = reinterpret_cast(work); + node::Environment* env_ = node::Environment::GetCurrent(env->isolate); + env_->IncreaseWaitingRequestCounter(); CALL_UV(env, uv_queue_work(event_loop, w->Request(), uvimpl::Work::ExecuteCallback, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index a80bd95956fa30..f818f9ee4b1ef0 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4645,9 +4645,12 @@ void PBKDF2Request::After() { void PBKDF2Request::After(uv_work_t* work_req, int status) { - CHECK_EQ(status, 0); std::unique_ptr req( ContainerOf(&PBKDF2Request::work_req_, work_req)); + req->env()->DecreaseWaitingRequestCounter(); + if (status == UV_ECANCELED) + return; + CHECK_EQ(status, 0); req->After(); } @@ -4698,6 +4701,7 @@ void PBKDF2(const FunctionCallbackInfo& args) { if (args[5]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[5]).FromJust(); + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), req.release()->work_req(), PBKDF2Request::Work, @@ -4837,10 +4841,13 @@ void RandomBytesCheck(RandomBytesRequest* req, Local (*argv)[2]) { void RandomBytesAfter(uv_work_t* work_req, int status) { - CHECK_EQ(status, 0); std::unique_ptr req( ContainerOf(&RandomBytesRequest::work_req_, work_req)); Environment* env = req->env(); + env->DecreaseWaitingRequestCounter(); + if (status == UV_ECANCELED) + return; + CHECK_EQ(status, 0); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); Local argv[2]; @@ -4880,6 +4887,7 @@ void RandomBytes(const FunctionCallbackInfo& args) { if (args[1]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[1]).FromJust(); + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), req.release()->work_req(), RandomBytesWork, @@ -4919,6 +4927,7 @@ void RandomBytesBuffer(const FunctionCallbackInfo& args) { if (args[3]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[3]).FromJust(); + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), req.release()->work_req(), RandomBytesWork, diff --git a/src/node_zlib.cc b/src/node_zlib.cc index ec447638e2ae62..3249905dfbfaf9 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -214,6 +214,7 @@ class ZCtx : public AsyncWrap { } // async version + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), work_req, ZCtx::Process, ZCtx::After); } @@ -361,10 +362,17 @@ class ZCtx : public AsyncWrap { // v8 land! static void After(uv_work_t* work_req, int status) { - CHECK_EQ(status, 0); - ZCtx* ctx = ContainerOf(&ZCtx::work_req_, work_req); Environment* env = ctx->env(); + ctx->write_in_progress_ = false; + + env->DecreaseWaitingRequestCounter(); + if (status == UV_ECANCELED) { + ctx->Close(); + return; + } + + CHECK_EQ(status, 0); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -374,7 +382,6 @@ class ZCtx : public AsyncWrap { ctx->write_result_[0] = ctx->strm_.avail_out; ctx->write_result_[1] = ctx->strm_.avail_in; - ctx->write_in_progress_ = false; // call the write() cb Local cb = PersistentToLocal(env->isolate(), diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index 54abf74430f6af..e3b26c1f5c6030 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -20,6 +20,8 @@ ReqWrap::ReqWrap(Environment* env, // FIXME(bnoordhuis) The fact that a reinterpret_cast is needed is // arguably a good indicator that there should be more than one queue. env->req_wrap_queue()->PushBack(reinterpret_cast*>(this)); + + Reset(); } template @@ -33,6 +35,12 @@ void ReqWrap::Dispatched() { req_.data = this; } +template +void ReqWrap::Reset() { + original_callback_ = nullptr; + req_.data = nullptr; +} + template ReqWrap* ReqWrap::from_req(T* req) { return ContainerOf(&ReqWrap::req_, req); @@ -40,7 +48,8 @@ ReqWrap* ReqWrap::from_req(T* req) { template void ReqWrap::Cancel() { - uv_cancel(reinterpret_cast(&req_)); + if (req_.data == this) // Only cancel if already dispatched. + uv_cancel(reinterpret_cast(&req_)); } // Below is dark template magic designed to invoke libuv functions that @@ -95,7 +104,7 @@ struct CallLibuvFunction { template struct MakeLibuvRequestCallback { static T For(ReqWrap* req_wrap, T v) { - static_assert(!std::is_function::value, + static_assert(!is_callable::value, "MakeLibuvRequestCallback missed a callback"); return v; } @@ -109,6 +118,7 @@ struct MakeLibuvRequestCallback { static void Wrapper(ReqT* req, Args... args) { ReqWrap* req_wrap = ContainerOf(&ReqWrap::req_, req); + req_wrap->env()->DecreaseWaitingRequestCounter(); F original_callback = reinterpret_cast(req_wrap->original_callback_); original_callback(req, args...); } @@ -128,23 +138,26 @@ int ReqWrap::Dispatch(LibuvFunction fn, Args... args) { // This expands as: // - // return fn(env()->event_loop(), req(), arg1, arg2, Wrapper, arg3, ...) - // ^ ^ ^ - // | | | - // \-- Omitted if `fn` has no | | - // first `uv_loop_t*` argument | | - // | | - // A function callback whose first argument | | - // matches the libuv request type is replaced ---/ | - // by the `Wrapper` method defined above | - // | - // Other (non-function) arguments are passed -----/ - // through verbatim - return CallLibuvFunction::Call( + // int err = fn(env()->event_loop(), req(), arg1, arg2, Wrapper, arg3, ...) + // ^ ^ ^ + // | | | + // \-- Omitted if `fn` has no | | + // first `uv_loop_t*` argument | | + // | | + // A function callback whose first argument | | + // matches the libuv request type is replaced ---/ | + // by the `Wrapper` method defined above | + // | + // Other (non-function) arguments are passed -----/ + // through verbatim + int err = CallLibuvFunction::Call( fn, env()->event_loop(), req(), MakeLibuvRequestCallback::For(this, args)...); + if (err >= 0) + env()->IncreaseWaitingRequestCounter(); + return err; } } // namespace node diff --git a/src/req_wrap.h b/src/req_wrap.h index d181817218227b..8f8d0cf2885594 100644 --- a/src/req_wrap.h +++ b/src/req_wrap.h @@ -20,6 +20,8 @@ class ReqWrap : public AsyncWrap { // Call this after the req has been dispatched, if that did not already // happen by using Dispatch(). inline void Dispatched(); + // Call this after a request has finished, if re-using this object is planned. + inline void Reset(); T* req() { return &req_; } inline void Cancel(); diff --git a/src/util.h b/src/util.h index eb15839241872f..9595ee8f07a1c6 100644 --- a/src/util.h +++ b/src/util.h @@ -447,8 +447,16 @@ struct MallocedBuffer { MallocedBuffer& operator=(const MallocedBuffer&) = delete; }; -} // namespace node +// Test whether some value can be called with (). +template +struct is_callable : std::is_function { }; + +template +struct is_callable::value + >::type> : std::true_type { }; +} // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS From 9e2554ce9879c183b510dc110a0e6916234c4159 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 9 Sep 2017 22:29:08 +0200 Subject: [PATCH 056/208] src: use cleanup hooks to tear down BaseObjects Clean up after `BaseObject` instances when the `Environment` is being shut down. This takes care of closing non-libuv resources like `zlib` instances, which do not require asynchronous shutdown. Many thanks for Stephen Belanger, Timothy Gu and Alexey Orlenko for reviewing the original version of this commit in the Ayo.js project. Refs: https://github.com/ayojs/ayo/pull/88 PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/base_object-inl.h | 9 +++++++++ src/base_object.h | 2 ++ src/env.cc | 6 ++++++ src/inspector_agent.cc | 2 ++ src/node.cc | 3 ++- src/req_wrap-inl.h | 1 - 6 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 786e1f26b48256..3bd854639b2c6d 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -37,10 +37,13 @@ BaseObject::BaseObject(Environment* env, v8::Local object) CHECK_EQ(false, object.IsEmpty()); CHECK_GT(object->InternalFieldCount(), 0); object->SetAlignedPointerInInternalField(0, static_cast(this)); + env_->AddCleanupHook(DeleteMe, static_cast(this)); } BaseObject::~BaseObject() { + env_->RemoveCleanupHook(DeleteMe, static_cast(this)); + if (persistent_handle_.IsEmpty()) { // This most likely happened because the weak callback below cleared it. return; @@ -80,6 +83,12 @@ T* BaseObject::FromJSObject(v8::Local object) { } +void BaseObject::DeleteMe(void* data) { + BaseObject* self = static_cast(data); + delete self; +} + + void BaseObject::MakeWeak() { persistent_handle_.SetWeak( this, diff --git a/src/base_object.h b/src/base_object.h index 2a4967c1aaf303..e0b60843401681 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -71,6 +71,8 @@ class BaseObject { private: BaseObject(); + static inline void DeleteMe(void* data); + // persistent_handle_ needs to be at a fixed offset from the start of the // class because it is used by src/node_postmortem_metadata.cc to calculate // offsets and generate debug symbols for BaseObject, which assumes that the diff --git a/src/env.cc b/src/env.cc index e5b9c0fd6aad4e..ab5de3e2ee1b1d 100644 --- a/src/env.cc +++ b/src/env.cc @@ -133,6 +133,10 @@ Environment::Environment(IsolateData* isolate_data, } Environment::~Environment() { + // Make sure there are no re-used libuv wrapper objects. + // CleanupHandles() should have removed all of them. + CHECK(file_handle_read_wrap_freelist_.empty()); + v8::HandleScope handle_scope(isolate()); #if HAVE_INSPECTOR @@ -245,6 +249,8 @@ void Environment::CleanupHandles() { !handle_wrap_queue_.IsEmpty()) { uv_run(event_loop(), UV_RUN_ONCE); } + + file_handle_read_wrap_freelist_.clear(); } void Environment::StartProfilerIdleNotifier() { diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 4e0c04a7b95527..391d3bc037927e 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -576,6 +576,8 @@ std::unique_ptr Agent::Connect( void Agent::WaitForDisconnect() { CHECK_NE(client_, nullptr); + // TODO(addaleax): Maybe this should use an at-exit hook for the Environment + // or something similar? client_->contextDestroyed(parent_env_->context()); if (io_ != nullptr) { io_->WaitForDisconnect(); diff --git a/src/node.cc b/src/node.cc index 06acd1c74bc8ed..70787f2f2f7bc4 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4578,12 +4578,13 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, const int exit_code = EmitExit(&env); + WaitForInspectorDisconnect(&env); + env.RunCleanup(); RunAtExit(&env); v8_platform.DrainVMTasks(isolate); v8_platform.CancelVMTasks(isolate); - WaitForInspectorDisconnect(&env); #if defined(LEAK_SANITIZER) __lsan_do_leak_check(); #endif diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index e3b26c1f5c6030..7e9e2d9fbbf912 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -26,7 +26,6 @@ ReqWrap::ReqWrap(Environment* env, template ReqWrap::~ReqWrap() { - CHECK_EQ(req_.data, this); // Assert that someone has called Dispatched(). CHECK_EQ(false, persistent().IsEmpty()); } From 5b0d2e7b19209b7c97ea43076b16764a997b62ae Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 20 Sep 2017 14:43:19 +0200 Subject: [PATCH 057/208] src: add can_call_into_js flag This prevents calls back into JS from the shutdown phase. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: https://github.com/ayojs/ayo/pull/82 PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/async_wrap.cc | 7 +++++-- src/env-inl.h | 8 ++++++++ src/env.h | 7 +++++++ src/node.cc | 9 +++++++++ src/node_contextify.cc | 2 ++ 5 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index f7a6d4e68dd483..b20e2b746f7f89 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -141,6 +141,7 @@ static void DestroyAsyncIdsCallback(Environment* env, void* data) { do { std::vector destroy_async_id_list; destroy_async_id_list.swap(*env->destroy_async_id_list()); + if (!env->can_call_into_js()) return; for (auto async_id : destroy_async_id_list) { // Want each callback to be cleaned up after itself, instead of cleaning // them all up after the while() loop completes. @@ -166,7 +167,7 @@ void Emit(Environment* env, double async_id, AsyncHooks::Fields type, Local fn) { AsyncHooks* async_hooks = env->async_hooks(); - if (async_hooks->fields()[type] == 0) + if (async_hooks->fields()[type] == 0 || !env->can_call_into_js()) return; v8::HandleScope handle_scope(env->isolate()); @@ -625,8 +626,10 @@ void AsyncWrap::EmitTraceEventDestroy() { } void AsyncWrap::EmitDestroy(Environment* env, double async_id) { - if (env->async_hooks()->fields()[AsyncHooks::kDestroy] == 0) + if (env->async_hooks()->fields()[AsyncHooks::kDestroy] == 0 || + !env->can_call_into_js()) { return; + } if (env->destroy_async_id_list()->empty()) { env->SetUnrefImmediate(DestroyAsyncIdsCallback, nullptr); diff --git a/src/env-inl.h b/src/env-inl.h index f115656353cff3..0268879f5c7823 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -559,6 +559,14 @@ void Environment::SetUnrefImmediate(native_immediate_callback cb, CreateImmediate(cb, data, obj, false); } +inline bool Environment::can_call_into_js() const { + return can_call_into_js_; +} + +inline void Environment::set_can_call_into_js(bool can_call_into_js) { + can_call_into_js_ = can_call_into_js; +} + inline performance::performance_state* Environment::performance_state() { return performance_state_.get(); } diff --git a/src/env.h b/src/env.h index de3014249ea852..15d417ba606860 100644 --- a/src/env.h +++ b/src/env.h @@ -679,6 +679,12 @@ class Environment { const char* path = nullptr, const char* dest = nullptr); + // If this flag is set, calls into JS (if they would be observable + // from userland) must be avoided. This flag does not indicate whether + // calling into JS is allowed from a VM perspective at this point. + inline bool can_call_into_js() const; + inline void set_can_call_into_js(bool can_call_into_js); + inline void ThrowError(const char* errmsg); inline void ThrowTypeError(const char* errmsg); inline void ThrowRangeError(const char* errmsg); @@ -821,6 +827,7 @@ class Environment { std::unique_ptr performance_state_; std::unordered_map performance_marks_; + bool can_call_into_js_ = true; #if HAVE_INSPECTOR std::unique_ptr inspector_agent_; diff --git a/src/node.cc b/src/node.cc index 70787f2f2f7bc4..5429015a149c79 100644 --- a/src/node.cc +++ b/src/node.cc @@ -958,6 +958,11 @@ InternalCallbackScope::InternalCallbackScope(Environment* env, CHECK(!object.IsEmpty()); } + if (!env->can_call_into_js()) { + failed_ = true; + return; + } + HandleScope handle_scope(env->isolate()); // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(Environment::GetCurrent(env->isolate()), env); @@ -1001,6 +1006,7 @@ void InternalCallbackScope::Close() { Environment::TickInfo* tick_info = env_->tick_info(); + if (!env_->can_call_into_js()) return; if (!tick_info->has_scheduled()) { env_->isolate()->RunMicrotasks(); } @@ -1018,6 +1024,8 @@ void InternalCallbackScope::Close() { Local process = env_->process_object(); + if (!env_->can_call_into_js()) return; + if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { env_->tick_info()->set_has_thrown(true); failed_ = true; @@ -4580,6 +4588,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, WaitForInspectorDisconnect(&env); + env.set_can_call_into_js(false); env.RunCleanup(); RunAtExit(&env); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 6e7027a29ba8a0..da1eecf1666d80 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -819,6 +819,8 @@ class ContextifyScript : public BaseObject { const bool display_errors, const bool break_on_sigint, const FunctionCallbackInfo& args) { + if (!env->can_call_into_js()) + return false; if (!ContextifyScript::InstanceOf(env, args.Holder())) { env->ThrowTypeError( "Script methods can only be called on script instances."); From f2ad1d5d221912bd2b22b9396c857719269df1c5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 15 Mar 2018 21:39:57 +0100 Subject: [PATCH 058/208] tools: remove `--quiet` from run-valgrind.py This should no longer be an issue, now that we clean up resources when exiting. PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- tools/run-valgrind.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tools/run-valgrind.py b/tools/run-valgrind.py index ae1a3194cac4a6..cad3e7ec6954d2 100755 --- a/tools/run-valgrind.py +++ b/tools/run-valgrind.py @@ -37,9 +37,6 @@ 'valgrind', '--error-exitcode=1', '--smc-check=all', - # Node.js does not clean up on exit so don't complain about - # memory leaks but do complain about invalid memory access. - '--quiet', ] if len(sys.argv) < 2: From c8fe8e8f5d37091ae078ea6dd5042229e77f2c05 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 17 Mar 2018 22:14:51 +0100 Subject: [PATCH 059/208] process: create stdin with `manualStart: true` Otherwise Node.js will try to read data from the handle. This causes issues when Node.js is already reading from the same handle, but a different associated stream (e.g. a possible IPC channel). PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- lib/internal/process/stdio.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/internal/process/stdio.js b/lib/internal/process/stdio.js index 75ede6a8e7e157..9a16a5ab15f166 100644 --- a/lib/internal/process/stdio.js +++ b/lib/internal/process/stdio.js @@ -77,13 +77,15 @@ function setupStdio() { stdin = new net.Socket({ handle: process.channel, readable: true, - writable: false + writable: false, + manualStart: true }); } else { stdin = new net.Socket({ fd: fd, readable: true, - writable: false + writable: false, + manualStart: true }); } // Make sure the stdin can't be `.end()`-ed From 97d939a5f0ce17d5bbd936a7ec2f0c826ccb4457 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 17 Mar 2018 22:34:38 +0100 Subject: [PATCH 060/208] src: store fd for libuv streams on Windows On Windows, we can't just look up a FD for libuv streams and return it in `GetFD()`. However, we do sometimes construct streams from their FDs; in those cases, it should be okay to store the value on a class field. PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/pipe_wrap.cc | 1 + src/stream_wrap.cc | 6 ++++-- src/stream_wrap.h | 18 ++++++++++++++++++ src/tcp_wrap.cc | 1 + src/tty_wrap.cc | 1 + 5 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index 7ec5bdf15be9cc..f3a88a2ecc80c3 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -204,6 +204,7 @@ void PipeWrap::Open(const FunctionCallbackInfo& args) { int fd = args[0]->Int32Value(); int err = uv_pipe_open(&wrap->handle_, fd); + wrap->set_fd(fd); if (err != 0) env->isolate()->ThrowException(UVException(err, "uv_pipe_open")); diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index ad708c9ed28def..0e700ba39a6b95 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -115,12 +115,14 @@ void LibuvStreamWrap::AddMethods(Environment* env, int LibuvStreamWrap::GetFD() { +#ifdef _WIN32 + return fd_; +#else int fd = -1; -#if !defined(_WIN32) if (stream() != nullptr) uv_fileno(reinterpret_cast(stream()), &fd); -#endif return fd; +#endif } diff --git a/src/stream_wrap.h b/src/stream_wrap.h index a97e8ba10f91d5..7847ebe754614a 100644 --- a/src/stream_wrap.h +++ b/src/stream_wrap.h @@ -88,6 +88,14 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { v8::Local target, int flags = StreamBase::kFlagNone); + protected: + inline void set_fd(int fd) { +#ifdef _WIN32 + fd_ = fd; +#endif + } + + private: static void GetWriteQueueSize( const v8::FunctionCallbackInfo& info); @@ -101,6 +109,16 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { static void AfterUvShutdown(uv_shutdown_t* req, int status); uv_stream_t* const stream_; + +#ifdef _WIN32 + // We don't always have an FD that we could look up on the stream_ + // object itself on Windows. However, for some cases, we open handles + // using FDs; In that case, we can store and provide the value. + // This became necessary because it allows to detect situations + // where multiple handles refer to the same stdio FDs (in particular, + // a possible IPC channel and a regular process.std??? stream). + int fd_ = -1; +#endif }; diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index 70c60fa47cc68f..8200353b17bf99 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -212,6 +212,7 @@ void TCPWrap::Open(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(UV_EBADF)); int fd = static_cast(args[0]->IntegerValue()); uv_tcp_open(&wrap->handle_, fd); + wrap->set_fd(fd); } diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index d01caba4a558f2..cd8589cc7fc2e7 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -172,6 +172,7 @@ TTYWrap::TTYWrap(Environment* env, reinterpret_cast(&handle_), AsyncWrap::PROVIDER_TTYWRAP) { *init_err = uv_tty_init(env->event_loop(), &handle_, fd, readable); + set_fd(fd); if (*init_err != 0) MarkAsUninitialized(); } From 9e1dcdc5bd9cd7128c61e8c721fc8ae9d97c272c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Apr 2018 00:16:09 +0200 Subject: [PATCH 061/208] src: remove NodeCategorySet destructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This currently crashes during environment cleanup because the object would be torn down while there are enabled categories. I’m not sure about the exact semantics here, but since the object cannot be garbage collected at this point anyway because it’s `Persistent` handle is strong, removing the destructor at least doesn’t make anything worse than it is right now (i.e. the destructor would never have been called before anyway). PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/node_trace_events.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/node_trace_events.cc b/src/node_trace_events.cc index 0c0699f7be9e1f..363d046de1e947 100644 --- a/src/node_trace_events.cc +++ b/src/node_trace_events.cc @@ -21,11 +21,6 @@ using v8::Value; class NodeCategorySet : public BaseObject { public: - ~NodeCategorySet() override { - // Verify that the thing was properly disabled before gc - CHECK_NE(enabled_, true); - } - static void New(const FunctionCallbackInfo& args); static void Enable(const FunctionCallbackInfo& args); static void Disable(const FunctionCallbackInfo& args); From 61415dccc4ba9ecba45dafad1d2612279f389617 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 24 Mar 2018 23:11:10 +0100 Subject: [PATCH 062/208] lib: defer pausing stdin to the next tick This is done to match the stream implementation, which also only actually stops reading in the next tick after the `'pause'` event is emitted. PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- lib/internal/process/stdio.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/internal/process/stdio.js b/lib/internal/process/stdio.js index 9a16a5ab15f166..ce84142938f066 100644 --- a/lib/internal/process/stdio.js +++ b/lib/internal/process/stdio.js @@ -109,15 +109,22 @@ function setupStdio() { stdin._handle.readStop(); } - // if the user calls stdin.pause(), then we need to stop reading - // immediately, so that the process can close down. + // If the user calls stdin.pause(), then we need to stop reading + // once the stream implementation does so (one nextTick later), + // so that the process can close down. stdin.on('pause', () => { + process.nextTick(onpause); + }); + + function onpause() { if (!stdin._handle) return; - stdin._readableState.reading = false; - stdin._handle.reading = false; - stdin._handle.readStop(); - }); + if (stdin._handle.reading && !stdin._readableState.flowing) { + stdin._readableState.reading = false; + stdin._handle.reading = false; + stdin._handle.readStop(); + } + } return stdin; } From 7153bec9552438227e76dc1f51e84fb34ec841ff Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 24 Mar 2018 23:12:22 +0100 Subject: [PATCH 063/208] src: always call ReadStop() before Close() For libuv-backed streams, always explicitly stop reading before closing the handle. PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/handle_wrap.h | 3 ++- src/stream_wrap.cc | 5 +++++ src/stream_wrap.h | 2 ++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/handle_wrap.h b/src/handle_wrap.h index fd2d002dce0338..b2b09f5010d1f7 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -70,7 +70,8 @@ class HandleWrap : public AsyncWrap { inline uv_handle_t* GetHandle() const { return handle_; } - void Close(v8::Local close_callback = v8::Local()); + virtual void Close( + v8::Local close_callback = v8::Local()); protected: HandleWrap(Environment* env, diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 0e700ba39a6b95..cdcbe574f9ae5f 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -373,6 +373,11 @@ void LibuvStreamWrap::AfterUvWrite(uv_write_t* req, int status) { req_wrap->Done(status); } +void LibuvStreamWrap::Close(v8::Local close_callback) { + ReadStop(); + HandleWrap::Close(close_callback); +} + } // namespace node NODE_BUILTIN_MODULE_CONTEXT_AWARE(stream_wrap, diff --git a/src/stream_wrap.h b/src/stream_wrap.h index 7847ebe754614a..94a161b6d54e07 100644 --- a/src/stream_wrap.h +++ b/src/stream_wrap.h @@ -76,6 +76,8 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { ShutdownWrap* CreateShutdownWrap(v8::Local object) override; WriteWrap* CreateWriteWrap(v8::Local object) override; + void Close(v8::Local close_callback) override; + protected: LibuvStreamWrap(Environment* env, v8::Local object, From 2347ce8870c2cf4bb953f69277fa8618a1f4ed5b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 9 May 2018 17:40:24 +0200 Subject: [PATCH 064/208] src: unify thread pool work Instead of using the libuv mechanism directly, provide an internal `ThreadPoolWork` wrapper that takes care of increasing/decreasing the waiting request counter. PR-URL: https://github.com/nodejs/node/pull/19377 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/node_api.cc | 73 +++++++++++++------------------- src/node_crypto.cc | 99 ++++++++++++++------------------------------ src/node_internals.h | 35 ++++++++++++++++ src/node_zlib.cc | 29 ++++++------- 4 files changed, 108 insertions(+), 128 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 91a47a12d92751..b456ade2d4ca74 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -3338,7 +3338,7 @@ static napi_status ConvertUVErrorCode(int code) { } // Wrapper around uv_work_t which calls user-provided callbacks. -class Work : public node::AsyncResource { +class Work : public node::AsyncResource, public node::ThreadPoolWork { private: explicit Work(napi_env env, v8::Local async_resource, @@ -3349,15 +3349,14 @@ class Work : public node::AsyncResource { : AsyncResource(env->isolate, async_resource, *v8::String::Utf8Value(env->isolate, async_resource_name)), - _env(env), - _data(data), - _execute(execute), - _complete(complete) { - memset(&_request, 0, sizeof(_request)); - _request.data = this; + ThreadPoolWork(node::Environment::GetCurrent(env->isolate)), + _env(env), + _data(data), + _execute(execute), + _complete(complete) { } - ~Work() { } + virtual ~Work() { } public: static Work* New(napi_env env, @@ -3374,47 +3373,36 @@ class Work : public node::AsyncResource { delete work; } - static void ExecuteCallback(uv_work_t* req) { - Work* work = static_cast(req->data); - work->_execute(work->_env, work->_data); + void DoThreadPoolWork() override { + _execute(_env, _data); } - static void CompleteCallback(uv_work_t* req, int status) { - Work* work = static_cast(req->data); + void AfterThreadPoolWork(int status) { + if (_complete == nullptr) + return; - if (work->_complete != nullptr) { - napi_env env = work->_env; + // Establish a handle scope here so that every callback doesn't have to. + // Also it is needed for the exception-handling below. + v8::HandleScope scope(_env->isolate); - // Establish a handle scope here so that every callback doesn't have to. - // Also it is needed for the exception-handling below. - v8::HandleScope scope(env->isolate); - node::Environment* env_ = node::Environment::GetCurrent(env->isolate); - env_->DecreaseWaitingRequestCounter(); + CallbackScope callback_scope(this); - CallbackScope callback_scope(work); + NAPI_CALL_INTO_MODULE(_env, + _complete(_env, ConvertUVErrorCode(status), _data), + [this] (v8::Local local_err) { + // If there was an unhandled exception in the complete callback, + // report it as a fatal exception. (There is no JavaScript on the + // callstack that can possibly handle it.) + v8impl::trigger_fatal_exception(_env, local_err); + }); - NAPI_CALL_INTO_MODULE(env, - work->_complete(env, ConvertUVErrorCode(status), work->_data), - [env] (v8::Local local_err) { - // If there was an unhandled exception in the complete callback, - // report it as a fatal exception. (There is no JavaScript on the - // callstack that can possibly handle it.) - v8impl::trigger_fatal_exception(env, local_err); - }); - - // Note: Don't access `work` after this point because it was - // likely deleted by the complete callback. - } - } - - uv_work_t* Request() { - return &_request; + // Note: Don't access `work` after this point because it was + // likely deleted by the complete callback. } private: napi_env _env; void* _data; - uv_work_t _request; napi_async_execute_callback _execute; napi_async_complete_callback _complete; }; @@ -3491,12 +3479,7 @@ napi_status napi_queue_async_work(napi_env env, napi_async_work work) { uvimpl::Work* w = reinterpret_cast(work); - node::Environment* env_ = node::Environment::GetCurrent(env->isolate); - env_->IncreaseWaitingRequestCounter(); - CALL_UV(env, uv_queue_work(event_loop, - w->Request(), - uvimpl::Work::ExecuteCallback, - uvimpl::Work::CompleteCallback)); + w->ScheduleWork(); return napi_clear_last_error(env); } @@ -3507,7 +3490,7 @@ napi_status napi_cancel_async_work(napi_env env, napi_async_work work) { uvimpl::Work* w = reinterpret_cast(work); - CALL_UV(env, uv_cancel(reinterpret_cast(w->Request()))); + CALL_UV(env, w->CancelWork()); return napi_clear_last_error(env); } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index f818f9ee4b1ef0..25b47b798d8c9f 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4562,7 +4562,7 @@ bool ECDH::IsKeyPairValid() { } -class PBKDF2Request : public AsyncWrap { +class PBKDF2Request : public AsyncWrap, public ThreadPoolWork { public: PBKDF2Request(Environment* env, Local object, @@ -4572,6 +4572,7 @@ class PBKDF2Request : public AsyncWrap { int keylen, int iteration_count) : AsyncWrap(env, object, AsyncWrap::PROVIDER_PBKDF2REQUEST), + ThreadPoolWork(env), digest_(digest), success_(false), pass_(std::move(pass)), @@ -4580,21 +4581,14 @@ class PBKDF2Request : public AsyncWrap { iteration_count_(iteration_count) { } - uv_work_t* work_req() { - return &work_req_; - } - size_t self_size() const override { return sizeof(*this); } - static void Work(uv_work_t* work_req); - void Work(); + void DoThreadPoolWork() override; + void AfterThreadPoolWork(int status) override; - static void After(uv_work_t* work_req, int status); void After(Local (*argv)[2]); - void After(); private: - uv_work_t work_req_; const EVP_MD* digest_; bool success_; MallocedBuffer pass_; @@ -4604,7 +4598,7 @@ class PBKDF2Request : public AsyncWrap { }; -void PBKDF2Request::Work() { +void PBKDF2Request::DoThreadPoolWork() { success_ = PKCS5_PBKDF2_HMAC( pass_.data, pass_.size, @@ -4617,12 +4611,6 @@ void PBKDF2Request::Work() { } -void PBKDF2Request::Work(uv_work_t* work_req) { - PBKDF2Request* req = ContainerOf(&PBKDF2Request::work_req_, work_req); - req->Work(); -} - - void PBKDF2Request::After(Local (*argv)[2]) { if (success_) { (*argv)[0] = Null(env()->isolate()); @@ -4635,7 +4623,12 @@ void PBKDF2Request::After(Local (*argv)[2]) { } -void PBKDF2Request::After() { +void PBKDF2Request::AfterThreadPoolWork(int status) { + std::unique_ptr req(this); + if (status == UV_ECANCELED) + return; + CHECK_EQ(status, 0); + HandleScope handle_scope(env()->isolate()); Context::Scope context_scope(env()->context()); Local argv[2]; @@ -4644,17 +4637,6 @@ void PBKDF2Request::After() { } -void PBKDF2Request::After(uv_work_t* work_req, int status) { - std::unique_ptr req( - ContainerOf(&PBKDF2Request::work_req_, work_req)); - req->env()->DecreaseWaitingRequestCounter(); - if (status == UV_ECANCELED) - return; - CHECK_EQ(status, 0); - req->After(); -} - - void PBKDF2(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -4701,14 +4683,10 @@ void PBKDF2(const FunctionCallbackInfo& args) { if (args[5]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[5]).FromJust(); - env->IncreaseWaitingRequestCounter(); - uv_queue_work(env->event_loop(), - req.release()->work_req(), - PBKDF2Request::Work, - PBKDF2Request::After); + req.release()->ScheduleWork(); } else { env->PrintSyncTrace(); - req->Work(); + req->DoThreadPoolWork(); Local argv[2]; req->After(&argv); @@ -4721,7 +4699,7 @@ void PBKDF2(const FunctionCallbackInfo& args) { // Only instantiate within a valid HandleScope. -class RandomBytesRequest : public AsyncWrap { +class RandomBytesRequest : public AsyncWrap, public ThreadPoolWork { public: enum FreeMode { FREE_DATA, DONT_FREE_DATA }; @@ -4731,16 +4709,13 @@ class RandomBytesRequest : public AsyncWrap { char* data, FreeMode free_mode) : AsyncWrap(env, object, AsyncWrap::PROVIDER_RANDOMBYTESREQUEST), + ThreadPoolWork(env), error_(0), size_(size), data_(data), free_mode_(free_mode) { } - uv_work_t* work_req() { - return &work_req_; - } - inline size_t size() const { return size_; } @@ -4778,7 +4753,8 @@ class RandomBytesRequest : public AsyncWrap { size_t self_size() const override { return sizeof(*this); } - uv_work_t work_req_; + void DoThreadPoolWork() override; + void AfterThreadPoolWork(int status) override; private: unsigned long error_; // NOLINT(runtime/int) @@ -4788,21 +4764,17 @@ class RandomBytesRequest : public AsyncWrap { }; -void RandomBytesWork(uv_work_t* work_req) { - RandomBytesRequest* req = - ContainerOf(&RandomBytesRequest::work_req_, work_req); - +void RandomBytesRequest::DoThreadPoolWork() { // Ensure that OpenSSL's PRNG is properly seeded. CheckEntropy(); - const int r = RAND_bytes(reinterpret_cast(req->data()), - req->size()); + const int r = RAND_bytes(reinterpret_cast(data_), size_); // RAND_bytes() returns 0 on error. if (r == 0) { - req->set_error(ERR_get_error()); // NOLINT(runtime/int) + set_error(ERR_get_error()); // NOLINT(runtime/int) } else if (r == -1) { - req->set_error(static_cast(-1)); // NOLINT(runtime/int) + set_error(static_cast(-1)); // NOLINT(runtime/int) } } @@ -4840,19 +4812,16 @@ void RandomBytesCheck(RandomBytesRequest* req, Local (*argv)[2]) { } -void RandomBytesAfter(uv_work_t* work_req, int status) { - std::unique_ptr req( - ContainerOf(&RandomBytesRequest::work_req_, work_req)); - Environment* env = req->env(); - env->DecreaseWaitingRequestCounter(); +void RandomBytesRequest::AfterThreadPoolWork(int status) { + std::unique_ptr req(this); if (status == UV_ECANCELED) return; CHECK_EQ(status, 0); - HandleScope handle_scope(env->isolate()); - Context::Scope context_scope(env->context()); + HandleScope handle_scope(env()->isolate()); + Context::Scope context_scope(env()->context()); Local argv[2]; - RandomBytesCheck(req.get(), &argv); - req->MakeCallback(env->ondone_string(), arraysize(argv), argv); + RandomBytesCheck(this, &argv); + MakeCallback(env()->ondone_string(), arraysize(argv), argv); } @@ -4860,7 +4829,7 @@ void RandomBytesProcessSync(Environment* env, std::unique_ptr req, Local (*argv)[2]) { env->PrintSyncTrace(); - RandomBytesWork(req->work_req()); + req->DoThreadPoolWork(); RandomBytesCheck(req.get(), argv); if (!(*argv)[0]->IsNull()) @@ -4887,11 +4856,7 @@ void RandomBytes(const FunctionCallbackInfo& args) { if (args[1]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[1]).FromJust(); - env->IncreaseWaitingRequestCounter(); - uv_queue_work(env->event_loop(), - req.release()->work_req(), - RandomBytesWork, - RandomBytesAfter); + req.release()->ScheduleWork(); args.GetReturnValue().Set(obj); } else { Local argv[2]; @@ -4927,11 +4892,7 @@ void RandomBytesBuffer(const FunctionCallbackInfo& args) { if (args[3]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[3]).FromJust(); - env->IncreaseWaitingRequestCounter(); - uv_queue_work(env->event_loop(), - req.release()->work_req(), - RandomBytesWork, - RandomBytesAfter); + req.release()->ScheduleWork(); args.GetReturnValue().Set(obj); } else { Local argv[2]; diff --git a/src/node_internals.h b/src/node_internals.h index d52d38aa138ca1..c10369c5631dd6 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -508,6 +508,41 @@ class InternalCallbackScope { bool closed_ = false; }; +class ThreadPoolWork { + public: + explicit inline ThreadPoolWork(Environment* env) : env_(env) {} + inline void ScheduleWork(); + inline int CancelWork(); + + virtual void DoThreadPoolWork() = 0; + virtual void AfterThreadPoolWork(int status) = 0; + + private: + Environment* env_; + uv_work_t work_req_; +}; + +void ThreadPoolWork::ScheduleWork() { + env_->IncreaseWaitingRequestCounter(); + int status = uv_queue_work( + env_->event_loop(), + &work_req_, + [](uv_work_t* req) { + ThreadPoolWork* self = ContainerOf(&ThreadPoolWork::work_req_, req); + self->DoThreadPoolWork(); + }, + [](uv_work_t* req, int status) { + ThreadPoolWork* self = ContainerOf(&ThreadPoolWork::work_req_, req); + self->env_->DecreaseWaitingRequestCounter(); + self->AfterThreadPoolWork(status); + }); + CHECK_EQ(status, 0); +} + +int ThreadPoolWork::CancelWork() { + return uv_cancel(reinterpret_cast(&work_req_)); +} + static inline const char *errno_string(int errorno) { #define ERRNO_CASE(e) case e: return #e; switch (errorno) { diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 3249905dfbfaf9..c77e6d3297df5d 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -70,10 +70,11 @@ enum node_zlib_mode { /** * Deflate/Inflate */ -class ZCtx : public AsyncWrap { +class ZCtx : public AsyncWrap, public ThreadPoolWork { public: ZCtx(Environment* env, Local wrap, node_zlib_mode mode) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_ZLIB), + ThreadPoolWork(env), dictionary_(nullptr), dictionary_len_(0), err_(0), @@ -191,9 +192,6 @@ class ZCtx : public AsyncWrap { CHECK(Buffer::IsWithinBounds(out_off, out_len, Buffer::Length(out_buf))); out = reinterpret_cast(Buffer::Data(out_buf) + out_off); - // build up the work request - uv_work_t* work_req = &(ctx->work_req_); - ctx->strm_.avail_in = in_len; ctx->strm_.next_in = in; ctx->strm_.avail_out = out_len; @@ -203,7 +201,7 @@ class ZCtx : public AsyncWrap { if (!async) { // sync version env->PrintSyncTrace(); - Process(work_req); + ctx->DoThreadPoolWork(); if (CheckError(ctx)) { ctx->write_result_[0] = ctx->strm_.avail_out; ctx->write_result_[1] = ctx->strm_.avail_in; @@ -214,18 +212,24 @@ class ZCtx : public AsyncWrap { } // async version - env->IncreaseWaitingRequestCounter(); - uv_queue_work(env->event_loop(), work_req, ZCtx::Process, ZCtx::After); + ctx->ScheduleWork(); } + // TODO(addaleax): Make these methods non-static. It's a significant bunch + // of churn that's better left for a separate PR. + void DoThreadPoolWork() { + Process(this); + } + + void AfterThreadPoolWork(int status) { + After(this, status); + } // thread pool! // This function may be called multiple times on the uv_work pool // for a single write() call, until all of the input bytes have // been consumed. - static void Process(uv_work_t* work_req) { - ZCtx *ctx = ContainerOf(&ZCtx::work_req_, work_req); - + static void Process(ZCtx* ctx) { const Bytef* next_expected_header_byte = nullptr; // If the avail_out is left at 0, then it means that it ran out @@ -361,12 +365,10 @@ class ZCtx : public AsyncWrap { // v8 land! - static void After(uv_work_t* work_req, int status) { - ZCtx* ctx = ContainerOf(&ZCtx::work_req_, work_req); + static void After(ZCtx* ctx, int status) { Environment* env = ctx->env(); ctx->write_in_progress_ = false; - env->DecreaseWaitingRequestCounter(); if (status == UV_ECANCELED) { ctx->Close(); return; @@ -685,7 +687,6 @@ class ZCtx : public AsyncWrap { int strategy_; z_stream strm_; int windowBits_; - uv_work_t work_req_; bool write_in_progress_; bool pending_close_; unsigned int refs_; From 57dfd64f8ff10594423948e69212479b99b4685e Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Thu, 10 May 2018 19:24:16 +0200 Subject: [PATCH 065/208] src: add missing override to ThreadPoolWork funcs Currently the following warnings are displayed when compiling: ../src/node_api.cc:3380:8: warning: 'AfterThreadPoolWork' overrides a member function but is not marked 'override' [-Winconsistent-missing-override] void AfterThreadPoolWork(int status) { ^ ../src/node_internals.h:513:16: note: overridden virtual function is here virtual void AfterThreadPoolWork(int status) = 0; ^ 1 warning generated. ../src/node_zlib.cc:220:8: warning: 'DoThreadPoolWork' overrides a member function but is not marked 'override' [-Winconsistent-missing-override] void DoThreadPoolWork() { ^ ../src/node_internals.h:512:16: note: overridden virtual function is here virtual void DoThreadPoolWork() = 0; ^ ../src/node_zlib.cc:224:8: warning: 'AfterThreadPoolWork' overrides a member function but is not marked 'override' [-Winconsistent-missing-override] void AfterThreadPoolWork(int status) { ^ ../src/node_internals.h:513:16: note: overridden virtual function is here virtual void AfterThreadPoolWork(int status) = 0; ^ 2 warnings generated. This commit adds override to the functions. PR-URL: https://github.com/nodejs/node/pull/20663 Reviewed-By: Anna Henningsen Reviewed-By: Michael Dawson --- src/node_api.cc | 2 +- src/node_zlib.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index b456ade2d4ca74..fb3ea5df2ce006 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -3377,7 +3377,7 @@ class Work : public node::AsyncResource, public node::ThreadPoolWork { _execute(_env, _data); } - void AfterThreadPoolWork(int status) { + void AfterThreadPoolWork(int status) override { if (_complete == nullptr) return; diff --git a/src/node_zlib.cc b/src/node_zlib.cc index c77e6d3297df5d..ac5083862de4e3 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -217,11 +217,11 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { // TODO(addaleax): Make these methods non-static. It's a significant bunch // of churn that's better left for a separate PR. - void DoThreadPoolWork() { + void DoThreadPoolWork() override { Process(this); } - void AfterThreadPoolWork(int status) { + void AfterThreadPoolWork(int status) override { After(this, status); } From 87f3f5af2e03e1291019701670b80287d3c368db Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 May 2018 22:18:36 +0200 Subject: [PATCH 066/208] test: plug AliasedBuffer cctest memory leak No need to heap-allocate values here. PR-URL: https://github.com/nodejs/node/pull/20665 Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: Daniel Bevenius --- test/cctest/test_aliased_buffer.cc | 79 ++++++++++++++---------------- 1 file changed, 37 insertions(+), 42 deletions(-) diff --git a/test/cctest/test_aliased_buffer.cc b/test/cctest/test_aliased_buffer.cc index 7afa466133b757..b2f29d3b7cb1ab 100644 --- a/test/cctest/test_aliased_buffer.cc +++ b/test/cctest/test_aliased_buffer.cc @@ -8,28 +8,26 @@ using node::AliasedBuffer; class AliasBufferTest : public NodeTestFixture {}; template -void CreateOracleValues(NativeT* buf, size_t count) { - for (size_t i = 0, j = count; i < count; i++, j--) { - buf[i] = static_cast(j); +void CreateOracleValues(std::vector* buf) { + for (size_t i = 0, j = buf->size(); i < buf->size(); i++, j--) { + (*buf)[i] = static_cast(j); } } template void WriteViaOperator(AliasedBuffer* aliasedBuffer, - size_t size, - NativeT* oracle) { + const std::vector& oracle) { // write through the API - for (size_t i = 0; i < size; i++) { + for (size_t i = 0; i < oracle.size(); i++) { (*aliasedBuffer)[i] = oracle[i]; } } template void WriteViaSetValue(AliasedBuffer* aliasedBuffer, - size_t size, - NativeT* oracle) { + const std::vector& oracle) { // write through the API - for (size_t i = 0; i < size; i++) { + for (size_t i = 0; i < oracle.size(); i++) { aliasedBuffer->SetValue(i, oracle[i]); } } @@ -38,10 +36,9 @@ template void ReadAndValidate(v8::Isolate* isolate, v8::Local context, AliasedBuffer* aliasedBuffer, - size_t size, - NativeT* oracle) { + const std::vector& oracle) { // read through the API - for (size_t i = 0; i < size; i++) { + for (size_t i = 0; i < oracle.size(); i++) { NativeT v1 = (*aliasedBuffer)[i]; NativeT v2 = aliasedBuffer->GetValue(i); EXPECT_TRUE(v1 == oracle[i]); @@ -49,16 +46,16 @@ void ReadAndValidate(v8::Isolate* isolate, } // validate size of JS Buffer - EXPECT_TRUE(aliasedBuffer->GetJSArray()->Length() == size); + EXPECT_TRUE(aliasedBuffer->GetJSArray()->Length() == oracle.size()); EXPECT_TRUE( aliasedBuffer->GetJSArray()->ByteLength() == - (size * sizeof(NativeT))); + (oracle.size() * sizeof(NativeT))); // validate operator * and GetBuffer are the same EXPECT_TRUE(aliasedBuffer->GetNativeBuffer() == *(*aliasedBuffer)); // read through the JS API - for (size_t i = 0; i < size; i++) { + for (size_t i = 0; i < oracle.size(); i++) { v8::Local v8TypedArray = aliasedBuffer->GetJSArray(); v8::MaybeLocal v = v8TypedArray->Get(context, i); EXPECT_TRUE(v.IsEmpty() == false); @@ -80,21 +77,19 @@ void ReadWriteTest(v8::Isolate* isolate) { const size_t size = 100; AliasedBuffer ab(isolate, size); - NativeT* oracle = new NativeT[size]; - CreateOracleValues(oracle, size); - WriteViaOperator(&ab, size, oracle); - ReadAndValidate(isolate, context, &ab, size, oracle); + std::vector oracle(size); + CreateOracleValues(&oracle); + WriteViaOperator(&ab, oracle); + ReadAndValidate(isolate, context, &ab, oracle); - WriteViaSetValue(&ab, size, oracle); + WriteViaSetValue(&ab, oracle); // validate copy constructor { AliasedBuffer ab2(ab); - ReadAndValidate(isolate, context, &ab2, size, oracle); + ReadAndValidate(isolate, context, &ab2, oracle); } - ReadAndValidate(isolate, context, &ab, size, oracle); - - delete[] oracle; + ReadAndValidate(isolate, context, &ab, oracle); } template< @@ -124,28 +119,28 @@ void SharedBufferTest( AliasedBuffer ab_C( isolate, sizeInBytes_A + sizeInBytes_B, count_C, rootBuffer); - NativeT_A* oracle_A = new NativeT_A[count_A]; - NativeT_B* oracle_B = new NativeT_B[count_B]; - NativeT_C* oracle_C = new NativeT_C[count_C]; - CreateOracleValues(oracle_A, count_A); - CreateOracleValues(oracle_B, count_B); - CreateOracleValues(oracle_C, count_C); + std::vector oracle_A(count_A); + std::vector oracle_B(count_B); + std::vector oracle_C(count_C); + CreateOracleValues(&oracle_A); + CreateOracleValues(&oracle_B); + CreateOracleValues(&oracle_C); - WriteViaOperator(&ab_A, count_A, oracle_A); - WriteViaOperator(&ab_B, count_B, oracle_B); - WriteViaOperator(&ab_C, count_C, oracle_C); + WriteViaOperator(&ab_A, oracle_A); + WriteViaOperator(&ab_B, oracle_B); + WriteViaOperator(&ab_C, oracle_C); - ReadAndValidate(isolate, context, &ab_A, count_A, oracle_A); - ReadAndValidate(isolate, context, &ab_B, count_B, oracle_B); - ReadAndValidate(isolate, context, &ab_C, count_C, oracle_C); + ReadAndValidate(isolate, context, &ab_A, oracle_A); + ReadAndValidate(isolate, context, &ab_B, oracle_B); + ReadAndValidate(isolate, context, &ab_C, oracle_C); - WriteViaSetValue(&ab_A, count_A, oracle_A); - WriteViaSetValue(&ab_B, count_B, oracle_B); - WriteViaSetValue(&ab_C, count_C, oracle_C); + WriteViaSetValue(&ab_A, oracle_A); + WriteViaSetValue(&ab_B, oracle_B); + WriteViaSetValue(&ab_C, oracle_C); - ReadAndValidate(isolate, context, &ab_A, count_A, oracle_A); - ReadAndValidate(isolate, context, &ab_B, count_B, oracle_B); - ReadAndValidate(isolate, context, &ab_C, count_C, oracle_C); + ReadAndValidate(isolate, context, &ab_A, oracle_A); + ReadAndValidate(isolate, context, &ab_B, oracle_B); + ReadAndValidate(isolate, context, &ab_C, oracle_C); } TEST_F(AliasBufferTest, Uint8Array) { From b10823506dcfa9bf966acb00b1738a02da60d8dc Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 5 May 2018 21:34:50 -0700 Subject: [PATCH 067/208] meta: add initial CODEOWNERS file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/20554 Reviewed-By: Gus Caplan Reviewed-By: Trivikram Kamat Reviewed-By: Michaël Zasso Reviewed-By: Matheus Marchini --- .github/CODEOWNERS | 86 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 .github/CODEOWNERS diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 00000000000000..fcf8667c7e6431 --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,86 @@ +# Remember: order matters. Subsequent rules will override prior rules. + +*addons* @nodejs/addon-api +*assert* @nodejs/testing +*async_hook* @nodejs/async_hooks @nodejs/diagnostics +*async_wrap* @nodejs/async_hooks +*buffer* @nodejs/buffer +*child_process* @nodejs/child_process +*cluster* @nodejs/cluster +*crypto* @nodejs/crypto +*eslint* @nodejs/linting +*fs* @nodejs/fs +*tls* @nodejs/tls @nodejs/crypto +*dgram* @nodejs/dgram +*domain* @nodejs/domains +*http* @nodejs/http +*http2* @nodejs/http2 +*https* @nodejs/http @nodejs/crypto @nodejs/tls @nodejs/http2 +*inspector* @nodejs/V8-inspector +*net* @nodejs/streams +*node_api* @nodejs/n-api +*repl* @nodejs/repl +*stream* @nodejs/streams +*timer* @nodejs/timers +*trace_events* @nodejs/trace-events +*url* @nodejs/url +*util* @nodejs/util +*vm* @nodejs/vm +*zlib* @nodejs/zlib + +*esm* @nodejs/modules +/lib/internal/modules/esm/ @nodejs/modules +/lib/internal/bootstrap/loaders.js @nodejs/modules + +/src/node.cc @nodejs/process +/src/node_file.* @nodejs/fs +/src/node_stat_watcher.* @nodejs/fs +/lib/internal/bootstrap/ @nodejs/process +/lib/internal/process/ @nodejs/process +/test/ @nodejs/testing + +/deps/node-inspect @nodejs/V8-inspector +/deps/gtest/ @nodejs/testing @nodejs/v8 @nodejs/v8-update +/deps/icu/ @nodejs/intl +/deps/http_parser/ @nodejs/http-parser @nodejs/http +/deps/zlib/ @nodejs/zlib +/deps/uv/ @nodejs/libuv +/deps/npm/ @nodejs/npm +/deps/v8/ @nodejs/v8 @nodejs/v8-update +/deps/zlib @nodejs/zlib +/deps/openssl @nodejs/crypto + +/benchmark/ @nodejs/performance @nodejs/benchmarking + +/doc/ @nodejs/documentation +*.md @nodejs/documentation +*.py @nodejs/python +*.gyp @nodejs/gyp + +Makefile @nodejs/build +vcbuild.bat @nodejs/build +BUILDING.md @nodejs/build +configure @nodejs/build +BSDMakefile @nodejs/build +android-configure @nodejs/build + +*aix* @nodejs/platform-aix +*arm* @nodejs/platform-arm +*freebsd* @nodejs/platform-freebsd +*macos* @nodejs/platform-macos +*ppc* @nodejs/platform-ppc +*smartos* @nodejs/platform-smartos +*s390* @nodejs/platform-s390 +*windows* @nodejs/platform-windows + +CODE_OF_CONDUCT.md @nodejs/tsc +COLLABORATOR_GUIDE.md @nodejs/tsc +CONTRIBUTING.md @nodejs/tsc +CPP_STYLE_GUIDE.md @nodejs/tsc +GOVERNANCE.md @nodejs/tsc +LICENSE @nodejs/tsc +README.md @nodejs/tsc + +/src/node_postmortem_metadata.cc @nodejs/diagnostics @nodejs/post-mortem +/src/*.d @nodejs/diagnostics @nodejs/platform-smartos @nodejs/platform-freebsd @nodejs/platform-macos @nodejs/dtrace-mdb +/src/node.stp @nodejs/diagnostics From 512982c0fff6ec645324e99d31e5b3c8b3307b7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 10 May 2018 15:37:13 +0200 Subject: [PATCH 068/208] doc: update AUTHORS list PR-URL: https://github.com/nodejs/node/pull/20658 Reviewed-By: Ruben Bridgewater Reviewed-By: Gus Caplan Reviewed-By: Michael Dawson Reviewed-By: Colin Ihrig Reviewed-By: Daniel Bevenius Reviewed-By: Trivikram Kamat Reviewed-By: James M Snell --- .mailmap | 4 ++++ AUTHORS | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/.mailmap b/.mailmap index 48da4f54b23828..16c9bf1ea21ef6 100644 --- a/.mailmap +++ b/.mailmap @@ -110,6 +110,7 @@ Evan Lucas FangDun Cai Fangdun Cai (Fundon) Fangshi He hefangshi Farid Neshat +Fatah N fatahn Fedor Indutny Felix Böhm Felix Geisendörfer @@ -145,6 +146,7 @@ Imran Iqbal Ionică Bizău Isaac Z. Schlueter Isaac Z. Schlueter +Isuru Siriwardana isurusiri Italo A. Casas Jackson Tian Jake Verbaten @@ -169,6 +171,7 @@ John Barboza jBarz John Barboza jBarz John Gardner Alhadis John McGuirk jmcgui05 +John Musgrave musgravejw Johnny Ray Austin Johnny Ray Jon Tippens legalcodes Jonas Pfenniger @@ -365,6 +368,7 @@ Tom Hughes-Croucher Tom Purcell tpurcell Tom White Tomoki Okahana umatoma +Tracy Hinds Tracy Travis Meisenheimer Trevor Burnham Trivikram Kamat <16024985+trivikr@users.noreply.github.com> diff --git a/AUTHORS b/AUTHORS index dfacc53994b2d8..3a156cbd2cb373 100644 --- a/AUTHORS +++ b/AUTHORS @@ -2139,5 +2139,63 @@ wangzengdi Garwah Lam jaspal-yupana Arian Santrach +Forrest Wolf +Fatah N +Divyanshu Singh +FallenRiteMonk +Rajkumar Purushothaman +Chris Miller +Dave O'Mahony +nodeav <30617226+nodeav@users.noreply.github.com> +Zhenzhen Zhan +Ryusei Yamaguchi +Kohei Hiraga +Nick Filatov +Jesse Gorzinski +Pieter Mees +Malcolm White +Gerhard Stoebich +Matei Copot +ikasumiwt +Gurin, Sebastian +Indranil Dasgupta +Harry Sarson +Snehil Verma +Joseph Gordon +Antoine du HAMEL +Rémi Berson +Alec Larson +Daven Casia +Isuru Siriwardana +Spencer Greene +Palash Nigam +SheetJS +Bryan Azofeifa +Christine E. Taylor +John Musgrave +Dhansuhu Uzumaki +Beni von Cheni +Ilya Sotov +William Cohen +Ajido +Mithun Sasidharan +kailash k yogeshwar +Daniel Hritzkiv +Mark Tiedemann +xsbchen +Kyle Martin +Denis Fäcke +Daylor Yanes +Carrie Coxwell +BeniCheni +Masashi Hirano +Brandon Ruggles +Allen Yonghuang Wang +Yichao 'Peak' Ji +Jesse W. Collins +TSUYUSATO Kitsune +daGo +Lambdac0re +Yulong Wang # Generated by tools/update-authors.sh From e732b4ce5c32467988dadd639a325350b4b96f22 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Fri, 11 May 2018 08:27:20 +0200 Subject: [PATCH 069/208] src: use unqualified names in node_contextify.cc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit removes the usage of qualified names for consistency. PR-URL: https://github.com/nodejs/node/pull/20669 Reviewed-By: Michaël Zasso Reviewed-By: Anna Henningsen Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Colin Ihrig Reviewed-By: Tobias Nießen Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- src/node_contextify.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index da1eecf1666d80..fa2c86e809542c 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -63,6 +63,7 @@ using v8::Uint8Array; using v8::UnboundScript; using v8::Value; using v8::WeakCallbackInfo; +using v8::WeakCallbackType; // The vm module executes code in a sandboxed environment with a different // global object than the rest of the code. This is achieved by applying @@ -103,7 +104,7 @@ ContextifyContext::ContextifyContext( // Allocation failure or maximum call stack size reached if (context_.IsEmpty()) return; - context_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); + context_.SetWeak(this, WeakCallback, WeakCallbackType::kParameter); } @@ -420,7 +421,7 @@ void ContextifyContext::PropertyDefinerCallback( return; Local context = ctx->context(); - v8::Isolate* isolate = context->GetIsolate(); + Isolate* isolate = context->GetIsolate(); auto attributes = PropertyAttribute::None; bool is_declared = @@ -453,13 +454,13 @@ void ContextifyContext::PropertyDefinerCallback( if (desc.has_get() || desc.has_set()) { PropertyDescriptor desc_for_sandbox( - desc.has_get() ? desc.get() : v8::Undefined(isolate).As(), - desc.has_set() ? desc.set() : v8::Undefined(isolate).As()); + desc.has_get() ? desc.get() : Undefined(isolate).As(), + desc.has_set() ? desc.set() : Undefined(isolate).As()); define_prop_on_sandbox(&desc_for_sandbox); } else { Local value = - desc.has_value() ? desc.value() : v8::Undefined(isolate).As(); + desc.has_value() ? desc.value() : Undefined(isolate).As(); if (desc.has_writable()) { PropertyDescriptor desc_for_sandbox(value, desc.writable()); From 9422909e0791462e2b23a33dc46115bc7efcc551 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Fri, 11 May 2018 08:43:03 +0200 Subject: [PATCH 070/208] src: remove unused includes from node_contextify.h MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/20670 Reviewed-By: Michaël Zasso Reviewed-By: Anna Henningsen Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- src/node_contextify.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/node_contextify.h b/src/node_contextify.h index 70ce091af3b58e..20bf2dafe2087c 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -2,8 +2,6 @@ #define SRC_NODE_CONTEXTIFY_H_ #include "node_internals.h" -#include "node_watchdog.h" -#include "base_object-inl.h" #include "node_context_data.h" namespace node { From 28ecf93dc5f2299aa072577f8d9d9e86d7f0e670 Mon Sep 17 00:00:00 2001 From: Mathias Buus Date: Fri, 6 Apr 2018 17:50:58 +0200 Subject: [PATCH 071/208] http2: destroy the socket properly and add tests Fix a bug where the socket wasn't being correctly destroyed and adjust existing tests, as well as add additional tests. PR-URL: https://github.com/nodejs/node/pull/19852 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Co-authored-by: Matteo Collina --- lib/internal/http2/core.js | 2 +- .../test-http2-many-writes-and-destroy.js | 30 +++++++++++++++++++ test/parallel/test-http2-pipe.js | 1 + .../test-http2-server-close-callback.js | 24 +++++++++++++++ ...est-http2-server-stream-session-destroy.js | 12 ++++++-- test/parallel/test-http2-session-unref.js | 14 ++++++--- .../test-http2-max-session-memory.js | 4 +++ 7 files changed, 79 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-http2-many-writes-and-destroy.js create mode 100644 test/parallel/test-http2-server-close-callback.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 0efdf9c063bf35..e7dbf9853fd49a 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1175,7 +1175,7 @@ class Http2Session extends EventEmitter { // Otherwise, destroy immediately. if (!socket.destroyed) { if (!error) { - setImmediate(socket.end.bind(socket)); + setImmediate(socket.destroy.bind(socket)); } else { socket.destroy(error); } diff --git a/test/parallel/test-http2-many-writes-and-destroy.js b/test/parallel/test-http2-many-writes-and-destroy.js new file mode 100644 index 00000000000000..78db76e001cd3a --- /dev/null +++ b/test/parallel/test-http2-many-writes-and-destroy.js @@ -0,0 +1,30 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); + +{ + const server = http2.createServer((req, res) => { + req.pipe(res); + }); + + server.listen(0, () => { + const url = `http://localhost:${server.address().port}`; + const client = http2.connect(url); + const req = client.request({ ':method': 'POST' }); + + for (let i = 0; i < 4000; i++) { + req.write(Buffer.alloc(6)); + } + + req.on('close', common.mustCall(() => { + console.log('(req onclose)'); + server.close(); + client.close(); + })); + + req.once('data', common.mustCall(() => req.destroy())); + }); +} diff --git a/test/parallel/test-http2-pipe.js b/test/parallel/test-http2-pipe.js index 2a759f9848721b..d7dd99df91edb5 100644 --- a/test/parallel/test-http2-pipe.js +++ b/test/parallel/test-http2-pipe.js @@ -33,6 +33,7 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request({ ':method': 'POST' }); + req.on('response', common.mustCall()); req.resume(); diff --git a/test/parallel/test-http2-server-close-callback.js b/test/parallel/test-http2-server-close-callback.js new file mode 100644 index 00000000000000..66887aa62bebe5 --- /dev/null +++ b/test/parallel/test-http2-server-close-callback.js @@ -0,0 +1,24 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); + +const server = http2.createServer(); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + client.on('error', (err) => { + if (err.code !== 'ECONNRESET') + throw err; + }); +})); + +server.on('session', common.mustCall((s) => { + setImmediate(() => { + server.close(common.mustCall()); + s.destroy(); + }); +})); diff --git a/test/parallel/test-http2-server-stream-session-destroy.js b/test/parallel/test-http2-server-stream-session-destroy.js index 989c72ec959679..ef4f42fe39bc3a 100644 --- a/test/parallel/test-http2-server-stream-session-destroy.js +++ b/test/parallel/test-http2-server-stream-session-destroy.js @@ -44,10 +44,16 @@ server.on('stream', common.mustCall((stream) => { server.listen(0, common.mustCall(() => { const client = h2.connect(`http://localhost:${server.address().port}`); - client.on('error', () => {}); + client.on('error', (err) => { + if (err.code !== 'ECONNRESET') + throw err; + }); const req = client.request(); req.resume(); req.on('end', common.mustCall()); - req.on('close', common.mustCall(() => server.close())); - req.on('error', () => {}); + req.on('close', common.mustCall(() => server.close(common.mustCall()))); + req.on('error', (err) => { + if (err.code !== 'ECONNRESET') + throw err; + }); })); diff --git a/test/parallel/test-http2-session-unref.js b/test/parallel/test-http2-session-unref.js index e63cd0d208e32b..f946c2d3377b87 100644 --- a/test/parallel/test-http2-session-unref.js +++ b/test/parallel/test-http2-session-unref.js @@ -34,8 +34,11 @@ server.listen(0, common.mustCall(() => { // unref destroyed client { const client = http2.connect(`http://localhost:${port}`); - client.destroy(); - client.unref(); + + client.on('connect', common.mustCall(() => { + client.destroy(); + client.unref(); + })); } // unref destroyed client @@ -43,8 +46,11 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${port}`, { createConnection: common.mustCall(() => clientSide) }); - client.destroy(); - client.unref(); + + client.on('connect', common.mustCall(() => { + client.destroy(); + client.unref(); + })); } })); server.emit('connection', serverSide); diff --git a/test/sequential/test-http2-max-session-memory.js b/test/sequential/test-http2-max-session-memory.js index d6d3bf935db801..644a20a3c88a50 100644 --- a/test/sequential/test-http2-max-session-memory.js +++ b/test/sequential/test-http2-max-session-memory.js @@ -13,6 +13,10 @@ const largeBuffer = Buffer.alloc(1e6); const server = http2.createServer({ maxSessionMemory: 1 }); server.on('stream', common.mustCall((stream) => { + stream.on('error', (err) => { + if (err.code !== 'ECONNRESET') + throw err; + }); stream.respond(); stream.end(largeBuffer); })); From ebc1b77e5a3f631711ff5765bc575b903bfde360 Mon Sep 17 00:00:00 2001 From: Jackson Tian Date: Wed, 9 May 2018 01:24:44 +0800 Subject: [PATCH 072/208] stream: no need to initial er with false initial `er` with false is unnecessarily. PR-URL: https://github.com/nodejs/node/pull/20607 Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig Reviewed-By: Weijia Wang Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat --- lib/_stream_writable.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index d21daf0541d339..fff47a703fe252 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -251,7 +251,7 @@ function writeAfterEnd(stream, cb) { // and undefined/non-string values are only allowed in object mode. function validChunk(stream, state, chunk, cb) { var valid = true; - var er = false; + var er; if (chunk === null) { er = new ERR_STREAM_NULL_VALUES(); From 2db83fdc0c3feca60a9dadacaf4fbc1e5e6c6cf9 Mon Sep 17 00:00:00 2001 From: Francesco Falanga Date: Sun, 13 May 2018 18:29:25 +0200 Subject: [PATCH 073/208] test: remove deepStrictEqual() third argument The call to assert.deepStrictEqual() has a string literal for its third argument. Unfortunately, a side effect of that is that the values of the first two arguments are not displayed if there is an AssertionError. That information is useful for debugging. PR-URL: https://github.com/nodejs/node/pull/20702 Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater Reviewed-By: Rich Trott Reviewed-By: Trivikram Kamat Reviewed-By: Anna Henningsen --- test/parallel/test-net-socket-local-address.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-net-socket-local-address.js b/test/parallel/test-net-socket-local-address.js index c4f11db76bc4f2..1aa51e0c5c1d87 100644 --- a/test/parallel/test-net-socket-local-address.js +++ b/test/parallel/test-net-socket-local-address.js @@ -17,8 +17,8 @@ const server = net.createServer((socket) => { }); server.on('close', common.mustCall(() => { - assert.deepStrictEqual(clientLocalPorts, serverRemotePorts, - 'client and server should agree on the ports used'); + // client and server should agree on the ports used + assert.deepStrictEqual(clientLocalPorts, serverRemotePorts); assert.strictEqual(2, conns); })); From 8029a2473e032c5006d2dfc3044bdce1b221dee4 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 8 May 2018 21:56:24 +0200 Subject: [PATCH 074/208] http: always emit close on req and res PR-URL: https://github.com/nodejs/node/pull/20611 Fixes: https://github.com/nodejs/node/issues/20600 Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat --- lib/_http_server.js | 2 ++ test/parallel/test-http-req-res-close.js | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 test/parallel/test-http-req-res-close.js diff --git a/lib/_http_server.js b/lib/_http_server.js index 9c8b5cb8fbfe8e..928b57b3719684 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -561,6 +561,8 @@ function resOnFinish(req, res, socket, state, server) { req._dump(); res.detachSocket(socket); + req.emit('close'); + res.emit('close'); if (res._last) { if (typeof socket.destroySoon === 'function') { diff --git a/test/parallel/test-http-req-res-close.js b/test/parallel/test-http-req-res-close.js new file mode 100644 index 00000000000000..240134cb5d0902 --- /dev/null +++ b/test/parallel/test-http-req-res-close.js @@ -0,0 +1,16 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); + +const server = http.Server(common.mustCall((req, res) => { + res.end(); + res.on('finish', common.mustCall()); + res.on('close', common.mustCall()); + req.on('close', common.mustCall()); + res.socket.on('close', () => server.close()); +})); + +server.listen(0, common.mustCall(() => { + http.get({ port: server.address().port }, common.mustCall()); +})); From 30aceedba678a2e5c38de1999cf6ab821d5f1b6e Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Fri, 11 May 2018 09:20:26 +0200 Subject: [PATCH 075/208] src: make env_ and context_ private MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit makes the currently protected members env_ and context_ private in node_contextify.h. PR-URL: https://github.com/nodejs/node/pull/20671 Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Tobias Nießen Reviewed-By: James M Snell --- src/node_contextify.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/node_contextify.h b/src/node_contextify.h index 20bf2dafe2087c..2ebfef7f27374d 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -15,10 +15,6 @@ struct ContextOptions { }; class ContextifyContext { - protected: - Environment* const env_; - Persistent context_; - public: ContextifyContext(Environment* env, v8::Local sandbox_obj, @@ -97,6 +93,8 @@ class ContextifyContext { static void IndexedPropertyDeleterCallback( uint32_t index, const v8::PropertyCallbackInfo& args); + Environment* const env_; + Persistent context_; }; } // namespace contextify From 4c6bfbdbb4b00f2bee94869ee13a78084010880d Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 23 Apr 2018 14:42:19 -0200 Subject: [PATCH 076/208] http: fix client response close & aborted Fixes: https://github.com/nodejs/node/issues/20102 Fixes: https://github.com/nodejs/node/issues/20101 Fixes: https://github.com/nodejs/node/issues/1735 - Response should always emit close. - Response should always emit aborted if aborted. - Response should always emit close after request has emitted close. PR-URL: https://github.com/nodejs/node/pull/20075 Fixes: https://github.com/nodejs/node/issues/17352 Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat --- lib/_http_client.js | 39 ++++++++----- test/parallel/test-http-response-close.js | 71 +++++++++++++++-------- 2 files changed, 69 insertions(+), 41 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 04880182dace91..3e9c6e994cd01b 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -328,26 +328,33 @@ function socketCloseListener() { // NOTE: It's important to get parser here, because it could be freed by // the `socketOnData`. - var parser = socket.parser; - if (req.res && req.res.readable) { + const parser = socket.parser; + const res = req.res; + if (res) { // Socket closed before we emitted 'end' below. - if (!req.res.complete) { - req.res.aborted = true; - req.res.emit('aborted'); + if (!res.complete) { + res.aborted = true; + res.emit('aborted'); } - var res = req.res; - res.on('end', function() { + req.emit('close'); + if (res.readable) { + res.on('end', function() { + this.emit('close'); + }); + res.push(null); + } else { res.emit('close'); - }); - res.push(null); - } else if (!req.res && !req.socket._hadError) { - // This socket error fired before we started to - // receive a response. The error needs to - // fire on the request. - req.socket._hadError = true; - req.emit('error', createHangUpError()); + } + } else { + if (!req.socket._hadError) { + // This socket error fired before we started to + // receive a response. The error needs to + // fire on the request. + req.socket._hadError = true; + req.emit('error', createHangUpError()); + } + req.emit('close'); } - req.emit('close'); // Too bad. That output wasn't getting written. // This is pretty terrible that it doesn't raise an error. diff --git a/test/parallel/test-http-response-close.js b/test/parallel/test-http-response-close.js index c58a5884d59d1a..4c36003357980b 100644 --- a/test/parallel/test-http-response-close.js +++ b/test/parallel/test-http-response-close.js @@ -23,29 +23,50 @@ const common = require('../common'); const http = require('http'); -const server = http.createServer(common.mustCall(function(req, res) { - res.writeHead(200); - res.write('a'); +{ + const server = http.createServer( + common.mustCall((req, res) => { + res.writeHead(200); + res.write('a'); + }) + ); + server.listen( + 0, + common.mustCall(() => { + http.get( + { port: server.address().port }, + common.mustCall((res) => { + res.on('data', common.mustCall(() => { + res.destroy(); + })); + res.on('close', common.mustCall(() => { + server.close(); + })); + }) + ); + }) + ); +} - req.on('close', common.mustCall(function() { - console.error('request aborted'); - })); - res.on('close', common.mustCall(function() { - console.error('response aborted'); - })); -})); -server.listen(0); - -server.on('listening', function() { - console.error('make req'); - http.get({ - port: this.address().port - }, function(res) { - console.error('got res'); - res.on('data', function(data) { - console.error('destroy res'); - res.destroy(); - server.close(); - }); - }); -}); +{ + const server = http.createServer( + common.mustCall((req, res) => { + res.writeHead(200); + res.end('a'); + }) + ); + server.listen( + 0, + common.mustCall(() => { + http.get( + { port: server.address().port }, + common.mustCall((res) => { + res.on('close', common.mustCall(() => { + server.close(); + })); + res.resume(); + }) + ); + }) + ); +} From 7a980086c84852049606e81c3715ecd076b74a3a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 28 Apr 2018 00:21:46 +0800 Subject: [PATCH 077/208] build: always use BUILDTYPE binary to run JS tests PR-URL: https://github.com/nodejs/node/pull/20362 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Richard Lau Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat --- Makefile | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/Makefile b/Makefile index 02d97b633d87d6..e74f29b84dde6b 100644 --- a/Makefile +++ b/Makefile @@ -234,7 +234,7 @@ v8: .PHONY: jstest jstest: build-addons build-addons-napi ## Runs addon tests and JS tests - $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release \ + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) \ $(CI_JS_SUITES) \ $(CI_NATIVE_SUITES) @@ -269,13 +269,13 @@ test-cov: all $(MAKE) lint test-parallel: all - $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release parallel + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) parallel test-valgrind: all - $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release --valgrind sequential parallel message + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) --valgrind sequential parallel message test-check-deopts: all - $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release --check-deopts parallel sequential + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) --check-deopts parallel sequential benchmark/misc/function_call/build/Release/binding.node: all \ benchmark/misc/function_call/binding.cc \ @@ -398,7 +398,7 @@ clear-stalled: .PHONY: test-gc test-gc: all test/gc/build/Release/binding.node - $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release gc + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) gc .PHONY: test-gc-clean test-gc-clean: @@ -425,7 +425,7 @@ CI_DOC := doctool test-ci-native: LOGLEVEL := info test-ci-native: | test/addons/.buildstamp test/addons-napi/.buildstamp $(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \ - --mode=release --flaky-tests=$(FLAKY_TESTS) \ + --mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \ $(TEST_CI_ARGS) $(CI_NATIVE_SUITES) .PHONY: test-ci-js @@ -433,7 +433,7 @@ test-ci-native: | test/addons/.buildstamp test/addons-napi/.buildstamp # Related CI job: node-test-commit-arm-fanned test-ci-js: | clear-stalled $(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \ - --mode=release --flaky-tests=$(FLAKY_TESTS) \ + --mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \ $(TEST_CI_ARGS) $(CI_JS_SUITES) # Clean up any leftover processes, error if found. ps awwx | grep Release/node | grep -v grep | cat @@ -448,7 +448,7 @@ test-ci: LOGLEVEL := info test-ci: | clear-stalled build-addons build-addons-napi doc-only out/Release/cctest --gtest_output=tap:cctest.tap $(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \ - --mode=release --flaky-tests=$(FLAKY_TESTS) \ + --mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \ $(TEST_CI_ARGS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) $(CI_DOC) # Clean up any leftover processes, error if found. ps awwx | grep Release/node | grep -v grep | cat @@ -475,7 +475,7 @@ run-ci: build-ci $(MAKE) test-ci test-release: test-build - $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) test-debug: test-build $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=debug @@ -521,7 +521,7 @@ test-npm-publish: $(NODE_EXE) .PHONY: test-addons-napi test-addons-napi: test-build-addons-napi - $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release addons-napi + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) addons-napi .PHONY: test-addons-napi-clean test-addons-napi-clean: @@ -530,7 +530,7 @@ test-addons-napi-clean: .PHONY: test-addons test-addons: test-build test-addons-napi - $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release addons + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) addons .PHONY: test-addons-clean test-addons-clean: @@ -541,19 +541,19 @@ test-addons-clean: test-timers: $(MAKE) --directory=tools faketime - $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release timers + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) timers test-timers-clean: $(MAKE) --directory=tools clean test-async-hooks: - $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release async-hooks + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) async-hooks test-with-async-hooks: $(MAKE) build-addons $(MAKE) build-addons-napi $(MAKE) cctest - NODE_TEST_WITH_ASYNC_HOOKS=1 $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=release \ + NODE_TEST_WITH_ASYNC_HOOKS=1 $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) \ $(CI_JS_SUITES) \ $(CI_NATIVE_SUITES) From 0aab92f6b2ef1751a1de04dc7fd159916761ad0d Mon Sep 17 00:00:00 2001 From: Maya Lekova Date: Wed, 9 May 2018 15:05:51 +0200 Subject: [PATCH 078/208] test: add test for async hooks parity for async/await Add a basic test ensuring parity between before-after and init-promiseResolve hooks when using async/await. Add ability to initHooks and to checkInvocations utilities to transmit promiseResolve hook as well. See: https://github.com/nodejs/node/issues/20516 PR-URL: https://github.com/nodejs/node/pull/20626 Refs: https://github.com/nodejs/node/issues/20516 Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum Reviewed-By: Benedikt Meurer --- test/async-hooks/hook-checks.js | 2 +- test/async-hooks/init-hooks.js | 15 ++++- test/async-hooks/test-async-await.js | 91 ++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 test/async-hooks/test-async-await.js diff --git a/test/async-hooks/hook-checks.js b/test/async-hooks/hook-checks.js index 2abed61555a158..44970378131c4a 100644 --- a/test/async-hooks/hook-checks.js +++ b/test/async-hooks/hook-checks.js @@ -25,7 +25,7 @@ exports.checkInvocations = function checkInvocations(activity, hooks, stage) { ); // Check that actual invocations for all hooks match the expected invocations - [ 'init', 'before', 'after', 'destroy' ].forEach(checkHook); + [ 'init', 'before', 'after', 'destroy', 'promiseResolve' ].forEach(checkHook); function checkHook(k) { const val = hooks[k]; diff --git a/test/async-hooks/init-hooks.js b/test/async-hooks/init-hooks.js index e43761df623dc4..509f443b29a671 100644 --- a/test/async-hooks/init-hooks.js +++ b/test/async-hooks/init-hooks.js @@ -25,6 +25,7 @@ class ActivityCollector { onbefore, onafter, ondestroy, + onpromiseResolve, logid = null, logtype = null } = {}) { @@ -39,13 +40,16 @@ class ActivityCollector { this.onbefore = typeof onbefore === 'function' ? onbefore : noop; this.onafter = typeof onafter === 'function' ? onafter : noop; this.ondestroy = typeof ondestroy === 'function' ? ondestroy : noop; + this.onpromiseResolve = typeof onpromiseResolve === 'function' ? + onpromiseResolve : noop; // Create the hook with which we'll collect activity data this._asyncHook = async_hooks.createHook({ init: this._init.bind(this), before: this._before.bind(this), after: this._after.bind(this), - destroy: this._destroy.bind(this) + destroy: this._destroy.bind(this), + promiseResolve: this._promiseResolve.bind(this) }); } @@ -206,6 +210,13 @@ class ActivityCollector { this.ondestroy(uid); } + _promiseResolve(uid) { + const h = this._getActivity(uid, 'promiseResolve'); + this._stamp(h, 'promiseResolve'); + this._maybeLog(uid, h && h.type, 'promiseResolve'); + this.onpromiseResolve(uid); + } + _maybeLog(uid, type, name) { if (this._logid && (type == null || this._logtype == null || this._logtype === type)) { @@ -219,6 +230,7 @@ exports = module.exports = function initHooks({ onbefore, onafter, ondestroy, + onpromiseResolve, allowNoInit, logid, logtype @@ -228,6 +240,7 @@ exports = module.exports = function initHooks({ onbefore, onafter, ondestroy, + onpromiseResolve, allowNoInit, logid, logtype diff --git a/test/async-hooks/test-async-await.js b/test/async-hooks/test-async-await.js new file mode 100644 index 00000000000000..7f88cd9b18176f --- /dev/null +++ b/test/async-hooks/test-async-await.js @@ -0,0 +1,91 @@ +'use strict'; +const common = require('../common'); + +// This test ensures async hooks are being properly called +// when using async-await mechanics. This involves: +// 1. Checking that all initialized promises are being resolved +// 2. Checking that for each 'before' corresponding hook 'after' hook is called + +const assert = require('assert'); +const initHooks = require('./init-hooks'); + +const util = require('util'); + +const sleep = util.promisify(setTimeout); +// either 'inited' or 'resolved' +const promisesInitState = new Map(); +// either 'before' or 'after' AND asyncId must be present in the other map +const promisesExecutionState = new Map(); + +const hooks = initHooks({ + oninit, + onbefore, + onafter, + ondestroy: null, // Intentionally not tested, since it will be removed soon + onpromiseResolve +}); +hooks.enable(); + +function oninit(asyncId, type, triggerAsyncId, resource) { + if (type === 'PROMISE') { + promisesInitState.set(asyncId, 'inited'); + } +} + +function onbefore(asyncId) { + if (!promisesInitState.has(asyncId)) { + return; + } + promisesExecutionState.set(asyncId, 'before'); +} + +function onafter(asyncId) { + if (!promisesInitState.has(asyncId)) { + return; + } + + assert.strictEqual(promisesExecutionState.get(asyncId), 'before', + 'after hook called for promise without prior call' + + 'to before hook'); + assert.strictEqual(promisesInitState.get(asyncId), 'resolved', + 'after hook called for promise without prior call' + + 'to resolve hook'); + promisesExecutionState.set(asyncId, 'after'); +} + +function onpromiseResolve(asyncId) { + assert(promisesInitState.has(asyncId), + 'resolve hook called for promise without prior call to init hook'); + + promisesInitState.set(asyncId, 'resolved'); +} + +const timeout = common.platformTimeout(10); + +function checkPromisesInitState() { + for (const initState of promisesInitState.values()) { + assert.strictEqual(initState, 'resolved', + 'promise initialized without being resolved'); + } +} + +function checkPromisesExecutionState() { + for (const executionState of promisesExecutionState.values()) { + assert.strictEqual(executionState, 'after', + 'mismatch between before and after hook calls'); + } +} + +process.on('beforeExit', common.mustCall(() => { + hooks.disable(); + hooks.sanityCheck('PROMISE'); + + checkPromisesInitState(); + checkPromisesExecutionState(); +})); + +async function asyncFunc() { + await sleep(timeout); +} + +asyncFunc(); From d11a4358754d9c8bf53e53714222251154f9c351 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 14 May 2018 12:18:31 +0200 Subject: [PATCH 079/208] doc: fix typo in dns docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/20711 Reviewed-By: Tobias Nießen Reviewed-By: Yuta Hiroto Reviewed-By: Khaidi Chu Reviewed-By: Ruben Bridgewater Reviewed-By: Daniel Bevenius Reviewed-By: Trivikram Kamat Reviewed-By: Michael Dawson --- doc/api/dns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/dns.md b/doc/api/dns.md index e8932be9e387cd..74504910c24107 100644 --- a/doc/api/dns.md +++ b/doc/api/dns.md @@ -66,7 +66,7 @@ An independent resolver for DNS requests. Note that creating a new resolver uses the default server settings. Setting the servers used for a resolver using [`resolver.setServers()`][`dns.setServers()`] does not affect -other resolver: +other resolvers: ```js const { Resolver } = require('dns'); From 7bff6d15b2e7bd04921a007e96988f238386319b Mon Sep 17 00:00:00 2001 From: Vse Mozhet Byt Date: Wed, 25 Apr 2018 18:24:27 +0300 Subject: [PATCH 080/208] tools: overhaul tools/doc/html.js PR-URL: https://github.com/nodejs/node/pull/20613 Reviewed-By: Trivikram Kamat --- tools/doc/html.js | 520 +++++++++++++++++++--------------------------- 1 file changed, 213 insertions(+), 307 deletions(-) diff --git a/tools/doc/html.js b/tools/doc/html.js index 00fd48d8e6631f..60cacd5b791bcb 100644 --- a/tools/doc/html.js +++ b/tools/doc/html.js @@ -29,17 +29,10 @@ const typeParser = require('./type-parser.js'); module.exports = toHTML; -const STABILITY_TEXT_REG_EXP = /(.*:)\s*(\d)([\s\S]*)/; -const DOC_CREATED_REG_EXP = //; - -// Customized heading without id attribute. +// Make `marked` to not automatically insert id attributes in headings. const renderer = new marked.Renderer(); -renderer.heading = function(text, level) { - return `${text}\n`; -}; -marked.setOptions({ - renderer: renderer -}); +renderer.heading = (text, level) => `${text}\n`; +marked.setOptions({ renderer }); const docPath = path.resolve(__dirname, '..', '..', 'doc'); @@ -47,91 +40,39 @@ const gtocPath = path.join(docPath, 'api', '_toc.md'); const gtocMD = fs.readFileSync(gtocPath, 'utf8').replace(/^@\/\/.*$/gm, ''); const gtocHTML = marked(gtocMD).replace( / ` ` type === 'heading'); + const section = firstHeading ? firstHeading.text : 'Index'; - parseText(lexed); - lexed = preprocessElements(lexed); - - // Generate the table of contents. - // This mutates the lexed contents in-place. - buildToc(lexed, filename, function(er, toc) { - if (er) return cb(er); - - const id = toID(path.basename(filename)); - - template = template.replace(/__ID__/g, id); - template = template.replace(/__FILENAME__/g, filename); - template = template.replace(/__SECTION__/g, section || 'Index'); - template = template.replace(/__VERSION__/g, nodeVersion); - template = template.replace(/__TOC__/g, toc); - template = template.replace( - /__GTOC__/g, - gtocHTML.replace(`class="nav-${id}`, `class="nav-${id} active`) - ); - - if (opts.analytics) { - template = template.replace( - '', - analyticsScript(opts.analytics) - ); - } + preprocessText(lexed); + preprocessElements(lexed); - template = template.replace(/__ALTDOCS__/, altDocs(filename)); + // Generate the table of contents. This mutates the lexed contents in-place. + const toc = buildToc(lexed, filename); - // Content has to be the last thing we do with the lexed tokens, - // because it's destructive. - const content = marked.parser(lexed); - template = template.replace(/__CONTENT__/g, content); + const id = filename.replace(/\W+/g, '-'); - cb(null, template); - }); -} + let HTML = template.replace('__ID__', id) + .replace(/__FILENAME__/g, filename) + .replace('__SECTION__', section) + .replace(/__VERSION__/g, nodeVersion) + .replace('__TOC__', toc) + .replace('__GTOC__', gtocHTML.replace( + `class="nav-${id}`, `class="nav-${id} active`)); -function analyticsScript(analytics) { - return ` + if (analytics) { + HTML = HTML.replace('', ` - `; -} - -// Replace placeholders in text tokens. -function replaceInText(text) { - return linkJsTypeDocs(linkManPages(text)); -} - -function altDocs(filename) { - if (!docCreated) { - console.error(`Failed to add alternative version links to ${filename}`); - return ''; - } - - function lte(v) { - const ns = v.num.split('.'); - if (docCreated[1] > +ns[0]) - return false; - if (docCreated[1] < +ns[0]) - return true; - return docCreated[2] <= +ns[1]; + `); } - const versions = [ - { num: '10.x' }, - { num: '9.x' }, - { num: '8.x', lts: true }, - { num: '7.x' }, - { num: '6.x', lts: true }, - { num: '5.x' }, - { num: '4.x', lts: true }, - { num: '0.12.x' }, - { num: '0.10.x' } - ]; - - const host = 'https://nodejs.org'; - const href = (v) => `${host}/docs/latest-v${v.num}/api/${filename}.html`; - - function li(v) { - let html = `
  • ${v.num}`; - - if (v.lts) - html += ' LTS'; - - return html + '
  • '; + const docCreated = input.match( + //); + if (docCreated) { + HTML = HTML.replace('__ALTDOCS__', altDocs(filename, docCreated)); + } else { + console.error(`Failed to add alternative version links to ${filename}`); + HTML = HTML.replace('__ALTDOCS__', ''); } - const lis = versions.filter(lte).map(li).join('\n'); + // Content insertion has to be the last thing we do with the lexed tokens, + // because it's destructive. + HTML = HTML.replace('__CONTENT__', marked.parser(lexed)); - if (!lis.length) - return ''; - - return ` -
  • - View another version -
      ${lis}
    -
  • - `; + cb(null, HTML); } // Handle general body-text replacements. // For example, link man page references to the actual page. -function parseText(lexed) { - lexed.forEach(function(tok) { - if (tok.type === 'table') { - if (tok.cells) { - tok.cells.forEach((row, x) => { - row.forEach((_, y) => { - if (tok.cells[x] && tok.cells[x][y]) { - tok.cells[x][y] = replaceInText(tok.cells[x][y]); - } - }); - }); +function preprocessText(lexed) { + lexed.forEach((token) => { + if (token.type === 'table') { + if (token.header) { + token.header = token.header.map(replaceInText); } - if (tok.header) { - tok.header.forEach((_, i) => { - if (tok.header[i]) { - tok.header[i] = replaceInText(tok.header[i]); - } + if (token.cells) { + token.cells.forEach((row, i) => { + token.cells[i] = row.map(replaceInText); }); } - } else if (tok.text && tok.type !== 'code') { - tok.text = replaceInText(tok.text); + } else if (token.text && token.type !== 'code') { + token.text = replaceInText(token.text); } }); } +// Replace placeholders in text tokens. +function replaceInText(text) { + if (text === '') return text; + return linkJsTypeDocs(linkManPages(text)); +} + +// Syscalls which appear in the docs, but which only exist in BSD / macOS. +const BSD_ONLY_SYSCALLS = new Set(['lchmod']); +const MAN_PAGE = /(^|\s)([a-z.]+)\((\d)([a-z]?)\)/gm; + +// Handle references to man pages, eg "open(2)" or "lchmod(2)". +// Returns modified text, with such refs replaced with HTML links, for example +// 'open(2)'. +function linkManPages(text) { + return text.replace( + MAN_PAGE, (match, beginning, name, number, optionalCharacter) => { + // Name consists of lowercase letters, + // number is a single digit with an optional lowercase letter. + const displayAs = `${name}(${number}${optionalCharacter})`; + + if (BSD_ONLY_SYSCALLS.has(name)) { + return `${beginning}${displayAs}`; + } + return `${beginning}${displayAs}`; + }); +} + +const TYPE_SIGNATURE = /\{[^}]+\}/g; +function linkJsTypeDocs(text) { + const parts = text.split('`'); + + // Handle types, for example the source Markdown might say + // "This argument should be a {number} or {string}". + for (let i = 0; i < parts.length; i += 2) { + const typeMatches = parts[i].match(TYPE_SIGNATURE); + if (typeMatches) { + typeMatches.forEach((typeMatch) => { + parts[i] = parts[i].replace(typeMatch, typeParser.toLink(typeMatch)); + }); + } + } + + return parts.join('`'); +} + // Preprocess stability blockquotes and YAML blocks. -function preprocessElements(input) { - var state = null; - const output = []; +function preprocessElements(lexed) { + const STABILITY_RE = /(.*:)\s*(\d)([\s\S]*)/; + let state = null; let headingIndex = -1; let heading = null; - output.links = input.links; - input.forEach(function(tok, index) { - if (tok.type === 'heading') { + lexed.forEach((token, index) => { + if (token.type === 'heading') { headingIndex = index; - heading = tok; + heading = token; } - if (tok.type === 'html' && common.isYAMLBlock(tok.text)) { - tok.text = parseYAML(tok.text); + if (token.type === 'html' && common.isYAMLBlock(token.text)) { + token.text = parseYAML(token.text); } - if (tok.type === 'blockquote_start') { + if (token.type === 'blockquote_start') { state = 'MAYBE_STABILITY_BQ'; - return; + lexed[index] = { type: 'space' }; } - if (tok.type === 'blockquote_end' && state === 'MAYBE_STABILITY_BQ') { + if (token.type === 'blockquote_end' && state === 'MAYBE_STABILITY_BQ') { state = null; - return; + lexed[index] = { type: 'space' }; } - if (tok.type === 'paragraph' && state === 'MAYBE_STABILITY_BQ') { - if (tok.text.match(/Stability:.*/g)) { - const stabilityMatch = tok.text.match(STABILITY_TEXT_REG_EXP); - const stability = Number(stabilityMatch[2]); + if (token.type === 'paragraph' && state === 'MAYBE_STABILITY_BQ') { + if (token.text.includes('Stability:')) { + const [, prefix, number, explication] = token.text.match(STABILITY_RE); const isStabilityIndex = index - 2 === headingIndex || // General. index - 3 === headingIndex; // With api_metadata block. if (heading && isStabilityIndex) { - heading.stability = stability; + heading.stability = number; headingIndex = -1; heading = null; } - tok.text = parseAPIHeader(tok.text).replace(/\n/g, ' '); - output.push({ type: 'html', text: tok.text }); - return; + token.text = `
    ` + + '' + + `${prefix} ${number}${explication}
    ` + .replace(/\n/g, ' '); + lexed[index] = { type: 'html', text: token.text }; } else if (state === 'MAYBE_STABILITY_BQ') { - output.push({ type: 'blockquote_start' }); state = null; + lexed[index - 1] = { type: 'blockquote_start' }; } } - output.push(tok); }); - - return output; } function parseYAML(text) { const meta = common.extractAndParseYAML(text); - const html = [''; + return html; } -function parseAPIHeader(text) { - const classNames = 'api_stability api_stability_$2'; - const docsUrl = 'documentation.html#documentation_stability_index'; - - text = text.replace( - STABILITY_TEXT_REG_EXP, - `` - ); - return text; -} - -// Section is just the first heading. -function getSection(lexed) { - for (var i = 0, l = lexed.length; i < l; i++) { - var tok = lexed[i]; - if (tok.type === 'heading') return tok.text; - } - return ''; -} - -function getMark(anchor) { - return `#`; +const numberRe = /^\d*/; +function versionSort(a, b) { + a = a.trim(); + b = b.trim(); + let i = 0; // Common prefix length. + while (i < a.length && i < b.length && a[i] === b[i]) i++; + a = a.substr(i); + b = b.substr(i); + return +b.match(numberRe)[0] - +a.match(numberRe)[0]; } -function buildToc(lexed, filename, cb) { - var toc = []; - var depth = 0; - +function buildToc(lexed, filename) { const startIncludeRefRE = /^\s*\s*$/; - const endIncludeRefRE = /^\s*\s*$/; + const endIncludeRefRE = /^\s*\s*$/; const realFilenames = [filename]; - - lexed.forEach(function(tok) { - // Keep track of the current filename along @include directives. - if (tok.type === 'html') { - let match; - if ((match = tok.text.match(startIncludeRefRE)) !== null) - realFilenames.unshift(match[1]); - else if (tok.text.match(endIncludeRefRE)) + const idCounters = Object.create(null); + let toc = ''; + let depth = 0; + + lexed.forEach((token) => { + // Keep track of the current filename along comment wrappers of inclusions. + if (token.type === 'html') { + const [, includedFileName] = token.text.match(startIncludeRefRE) || []; + if (includedFileName !== undefined) + realFilenames.unshift(includedFileName); + else if (endIncludeRefRE.test(token.text)) realFilenames.shift(); } - if (tok.type !== 'heading') return; - if (tok.depth - depth > 1) { - return cb(new Error('Inappropriate heading level\n' + - JSON.stringify(tok))); + if (token.type !== 'heading') return; + + if (token.depth - depth > 1) { + throw new Error(`Inappropriate heading level:\n${JSON.stringify(token)}`); } - depth = tok.depth; + depth = token.depth; const realFilename = path.basename(realFilenames[0], '.md'); - const apiName = tok.text.trim(); - const id = getId(`${realFilename}_${apiName}`); - toc.push(new Array((depth - 1) * 2 + 1).join(' ') + - `* ` + - `${tok.text}`); - tok.text += getMark(id); - if (realFilename === 'errors' && apiName.startsWith('ERR_')) { - tok.text += getMark(apiName); + const headingText = token.text.trim(); + const id = getId(`${realFilename}_${headingText}`, idCounters); + toc += ' '.repeat((depth - 1) * 2) + + `* ` + + `${token.text}\n`; + token.text += `#`; + if (realFilename === 'errors' && headingText.startsWith('ERR_')) { + token.text += `#`; } }); - toc = marked.parse(toc.join('\n')); - cb(null, toc); + return marked(toc); } -const idCounters = {}; -function getId(text) { - text = text.toLowerCase(); - text = text.replace(/[^a-z0-9]+/g, '_'); - text = text.replace(/^_+|_+$/, ''); - text = text.replace(/^([^a-z])/, '_$1'); - if (idCounters.hasOwnProperty(text)) { - text += `_${++idCounters[text]}`; - } else { - idCounters[text] = 0; +const notAlphaNumerics = /[^a-z0-9]+/g; +const edgeUnderscores = /^_+|_+$/g; +const notAlphaStart = /^[^a-z]/; +function getId(text, idCounters) { + text = text.toLowerCase() + .replace(notAlphaNumerics, '_') + .replace(edgeUnderscores, '') + .replace(notAlphaStart, '_$&'); + if (idCounters[text] !== undefined) { + return `${text}_${++idCounters[text]}`; } + idCounters[text] = 0; return text; } -const numberRe = /^(\d*)/; -function versionSort(a, b) { - a = a.trim(); - b = b.trim(); - let i = 0; // Common prefix length. - while (i < a.length && i < b.length && a[i] === b[i]) i++; - a = a.substr(i); - b = b.substr(i); - return +b.match(numberRe)[1] - +a.match(numberRe)[1]; +function altDocs(filename, docCreated) { + const [, docCreatedMajor, docCreatedMinor] = docCreated.map(Number); + const host = 'https://nodejs.org'; + const versions = [ + { num: '10.x' }, + { num: '9.x' }, + { num: '8.x', lts: true }, + { num: '7.x' }, + { num: '6.x', lts: true }, + { num: '5.x' }, + { num: '4.x', lts: true }, + { num: '0.12.x' }, + { num: '0.10.x' } + ]; + + const getHref = (versionNum) => + `${host}/docs/latest-v${versionNum}/api/${filename}.html`; + + const wrapInListItem = (version) => + `
  • ${version.num}` + + `${version.lts ? ' LTS' : ''}
  • `; + + function isDocInVersion(version) { + const [versionMajor, versionMinor] = version.num.split('.').map(Number); + if (docCreatedMajor > versionMajor) return false; + if (docCreatedMajor < versionMajor) return true; + return docCreatedMinor <= versionMinor; + } + + const list = versions.filter(isDocInVersion).map(wrapInListItem).join('\n'); + + return list ? ` +
  • + View another version +
      ${list}
    +
  • + ` : ''; } From 28b58b56a809ae61ebb89aff9d56dac48194cbbb Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 May 2018 15:46:41 +0200 Subject: [PATCH 081/208] =?UTF-8?q?src:=20replace=20`template<`=20?= =?UTF-8?q?=E2=86=92=20`template=20<`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/20675 Reviewed-By: Colin Ihrig Reviewed-By: Ben Noordhuis Reviewed-By: Gus Caplan Reviewed-By: Tobias Nießen Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Daniel Bevenius Reviewed-By: Ruben Bridgewater --- src/cares_wrap.cc | 2 +- src/inspector_io.cc | 4 ++-- src/inspector_js_api.cc | 2 +- src/inspector_socket_server.cc | 2 +- src/module_wrap.cc | 2 +- src/node_http2.h | 2 +- src/node_url.cc | 2 +- src/util.h | 6 +++--- test/cctest/test_aliased_buffer.cc | 12 ++++++------ 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index f829eb2b01d5c9..24d424f73221e8 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -722,7 +722,7 @@ class QueryWrap : public AsyncWrap { }; -template +template Local AddrTTLToArray(Environment* env, const T* addrttls, size_t naddrttls) { diff --git a/src/inspector_io.cc b/src/inspector_io.cc index 38d88d7ab890c9..e6ceaf5b736d37 100644 --- a/src/inspector_io.cc +++ b/src/inspector_io.cc @@ -23,7 +23,7 @@ using AsyncAndAgent = std::pair; using v8_inspector::StringBuffer; using v8_inspector::StringView; -template +template using TransportAndIo = std::pair; std::string ScriptPath(uv_loop_t* loop, const std::string& script_name) { @@ -284,7 +284,7 @@ void InspectorIo::IoThreadAsyncCb(uv_async_t* async) { } } -template +template void InspectorIo::ThreadMain() { uv_loop_t loop; loop.data = nullptr; diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index ae353defe8079d..802029ac4f2e5a 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -194,7 +194,7 @@ static void* GetAsyncTask(int64_t asyncId) { return reinterpret_cast(asyncId << 1); } -template +template static void InvokeAsyncTaskFnWithId(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK(args[0]->IsNumber()); diff --git a/src/inspector_socket_server.cc b/src/inspector_socket_server.cc index e890f66a38b53f..42082c9c8495ed 100644 --- a/src/inspector_socket_server.cc +++ b/src/inspector_socket_server.cc @@ -261,7 +261,7 @@ class ServerSocket { private: explicit ServerSocket(InspectorSocketServer* server) : tcp_socket_(uv_tcp_t()), server_(server), port_(-1) {} - template + template static ServerSocket* FromTcpSocket(UvHandle* socket) { return node::ContainerOf(&ServerSocket::tcp_socket_, reinterpret_cast(socket)); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 060d0db7c7fffe..eb46225a318dca 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -548,7 +548,7 @@ enum ResolveExtensionsOptions { ONLY_VIA_EXTENSIONS }; -template +template Maybe ResolveExtensions(const URL& search) { if (options == TRY_EXACT_NAME) { std::string filePath = search.ToFilePath(); diff --git a/src/node_http2.h b/src/node_http2.h index f4ac926bb54452..f473e30286bc79 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1222,7 +1222,7 @@ class ExternalHeader : vec.len); } - template + template static MaybeLocal New(Environment* env, nghttp2_rcbuf* buf) { if (nghttp2_rcbuf_is_static(buf)) { auto& static_str_map = env->isolate_data()->http2_static_strs; diff --git a/src/node_url.cc b/src/node_url.cc index 09199afb141e3f..42ecf47f4c958a 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -988,7 +988,7 @@ void URLHost::ParseHost(const char* input, // Locates the longest sequence of 0 segments in an IPv6 address // in order to use the :: compression when serializing -template +template inline T* FindLongestZeroSequence(T* values, size_t len) { T* start = values; T* end = start + len; diff --git a/src/util.h b/src/util.h index 9595ee8f07a1c6..0e6fd5dd067c73 100644 --- a/src/util.h +++ b/src/util.h @@ -420,7 +420,7 @@ struct OnScopeLeave { }; // Simple RAII wrapper for contiguous data that uses malloc()/free(). -template +template struct MallocedBuffer { T* data; size_t size; @@ -448,10 +448,10 @@ struct MallocedBuffer { }; // Test whether some value can be called with (). -template +template struct is_callable : std::is_function { }; -template +template struct is_callable::value >::type> : std::true_type { }; diff --git a/test/cctest/test_aliased_buffer.cc b/test/cctest/test_aliased_buffer.cc index b2f29d3b7cb1ab..bfbf7294db612b 100644 --- a/test/cctest/test_aliased_buffer.cc +++ b/test/cctest/test_aliased_buffer.cc @@ -7,14 +7,14 @@ using node::AliasedBuffer; class AliasBufferTest : public NodeTestFixture {}; -template +template void CreateOracleValues(std::vector* buf) { for (size_t i = 0, j = buf->size(); i < buf->size(); i++, j--) { (*buf)[i] = static_cast(j); } } -template +template void WriteViaOperator(AliasedBuffer* aliasedBuffer, const std::vector& oracle) { // write through the API @@ -23,7 +23,7 @@ void WriteViaOperator(AliasedBuffer* aliasedBuffer, } } -template +template void WriteViaSetValue(AliasedBuffer* aliasedBuffer, const std::vector& oracle) { // write through the API @@ -32,7 +32,7 @@ void WriteViaSetValue(AliasedBuffer* aliasedBuffer, } } -template +template void ReadAndValidate(v8::Isolate* isolate, v8::Local context, AliasedBuffer* aliasedBuffer, @@ -68,7 +68,7 @@ void ReadAndValidate(v8::Isolate* isolate, } } -template +template void ReadWriteTest(v8::Isolate* isolate) { v8::Isolate::Scope isolate_scope(isolate); v8::HandleScope handle_scope(isolate); @@ -92,7 +92,7 @@ void ReadWriteTest(v8::Isolate* isolate) { ReadAndValidate(isolate, context, &ab, oracle); } -template< +template < class NativeT_A, class V8T_A, class NativeT_B, class V8T_B, class NativeT_C, class V8T_C> From 44960a0d5acb64c9aeef9f6d8089adc3b16e46da Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 May 2018 15:43:21 +0200 Subject: [PATCH 082/208] tools: make C++ linter reject `template<` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This one is more or less just for me. :) PR-URL: https://github.com/nodejs/node/pull/20675 Reviewed-By: Colin Ihrig Reviewed-By: Ben Noordhuis Reviewed-By: Gus Caplan Reviewed-By: Tobias Nießen Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Daniel Bevenius Reviewed-By: Ruben Bridgewater --- tools/cpplint.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/cpplint.py b/tools/cpplint.py index 460c1ecbfeb02f..ee103ef7161ba1 100644 --- a/tools/cpplint.py +++ b/tools/cpplint.py @@ -4230,6 +4230,10 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, error(filename, linenum, 'whitespace/tab', 1, 'Tab found; better to use spaces') + if line.find('template<') != -1: + error(filename, linenum, 'whitespace/template', 1, + 'Leave a single space after template, as in `template <...>`') + # One or three blank spaces at the beginning of the line is weird; it's # hard to reconcile that with 2-space indents. # NOTE: here are the conditions rob pike used for his tests. Mine aren't From 9c1c03e5d42d3379bb8cf83898d2561b83cc4fe8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 May 2018 13:53:18 +0200 Subject: [PATCH 083/208] test: better error message in trace events test PR-URL: https://github.com/nodejs/node/pull/20655 Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig Reviewed-By: Rich Trott Reviewed-By: Trivikram Kamat --- test/parallel/test-trace-events-async-hooks.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-trace-events-async-hooks.js b/test/parallel/test-trace-events-async-hooks.js index d3c5a0af5b8e50..e0b5e0625bea6f 100644 --- a/test/parallel/test-trace-events-async-hooks.js +++ b/test/parallel/test-trace-events-async-hooks.js @@ -3,6 +3,7 @@ const common = require('../common'); const assert = require('assert'); const cp = require('child_process'); const fs = require('fs'); +const util = require('util'); const CODE = 'setTimeout(() => { for (var i = 0; i < 100000; i++) { "test" + i } }, 1)'; @@ -58,9 +59,9 @@ proc.once('exit', common.mustCall(() => { const initEvents = traces.filter((trace) => { return (trace.ph === 'b' && !trace.name.includes('_CALLBACK')); }); - assert(initEvents.every((trace) => { + assert.ok(initEvents.every((trace) => { return (trace.args.executionAsyncId > 0 && trace.args.triggerAsyncId > 0); - })); + }), `Unexpected initEvents format: ${util.inspect(initEvents)}`); })); })); From b795953b5f041c5c1b209a7738f01bc4e78569de Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 9 May 2018 19:00:57 +0200 Subject: [PATCH 084/208] tools: hide symbols for builtin JS files in binary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Do not expose symbols like `node::internal_process_next_tick_value`, `node::internal_process_next_tick_key` in the created `node` binary by wrapping them in an anonymous namespace. PR-URL: https://github.com/nodejs/node/pull/20634 Reviewed-By: James M Snell Reviewed-By: Gus Caplan Reviewed-By: Colin Ihrig Reviewed-By: Ben Noordhuis Reviewed-By: Tobias Nießen Reviewed-By: Daniel Bevenius Reviewed-By: Tiancheng "Timothy" Gu --- tools/js2c.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/js2c.py b/tools/js2c.py index bcfc4764a97df5..8685722c13cdc5 100755 --- a/tools/js2c.py +++ b/tools/js2c.py @@ -183,8 +183,12 @@ def ReadMacros(lines): namespace node {{ +namespace {{ + {definitions} +}} // anonymous namespace + v8::Local LoadersBootstrapperSource(Environment* env) {{ return internal_bootstrap_loaders_value.ToStringChecked(env->isolate()); }} From d223e3ca415c0f5b7751cb0fa8f808a318dcd071 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 9 May 2018 18:32:59 +0200 Subject: [PATCH 085/208] src: make `AsyncResource` destructor virtual MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AsyncResource` is intended to be a base class, and since we don’t know what API consumers will do with it in their own code, it’s good practice to make its destructor virtual. This should not be ABI-breaking since all class methods are inline. PR-URL: https://github.com/nodejs/node/pull/20633 Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Colin Ihrig --- src/node.h | 2 +- test/addons/async-resource/binding.cc | 22 ++++++++++++++++++++++ test/addons/async-resource/test.js | 2 ++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/node.h b/src/node.h index 23e2e9995ce209..5d6aa8dadfc8c4 100644 --- a/src/node.h +++ b/src/node.h @@ -712,7 +712,7 @@ class AsyncResource { trigger_async_id); } - ~AsyncResource() { + virtual ~AsyncResource() { EmitAsyncDestroy(isolate_, async_context_); } diff --git a/test/addons/async-resource/binding.cc b/test/addons/async-resource/binding.cc index 857d74c2e62206..ab33858c233dd0 100644 --- a/test/addons/async-resource/binding.cc +++ b/test/addons/async-resource/binding.cc @@ -17,6 +17,17 @@ using v8::Object; using v8::String; using v8::Value; +int custom_async_resource_destructor_calls = 0; + +class CustomAsyncResource : public AsyncResource { + public: + CustomAsyncResource(Isolate* isolate, Local resource) + : AsyncResource(isolate, resource, "CustomAsyncResource") {} + ~CustomAsyncResource() { + custom_async_resource_destructor_calls++; + } +}; + void CreateAsyncResource(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); assert(args[0]->IsObject()); @@ -98,6 +109,16 @@ void GetResource(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(r->get_resource()); } +void RunSubclassTest(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Local obj = Object::New(isolate); + + assert(custom_async_resource_destructor_calls == 0); + CustomAsyncResource* resource = new CustomAsyncResource(isolate, obj); + delete static_cast(resource); + assert(custom_async_resource_destructor_calls == 1); +} + void Initialize(Local exports) { NODE_SET_METHOD(exports, "createAsyncResource", CreateAsyncResource); NODE_SET_METHOD(exports, "destroyAsyncResource", DestroyAsyncResource); @@ -107,6 +128,7 @@ void Initialize(Local exports) { NODE_SET_METHOD(exports, "getAsyncId", GetAsyncId); NODE_SET_METHOD(exports, "getTriggerAsyncId", GetTriggerAsyncId); NODE_SET_METHOD(exports, "getResource", GetResource); + NODE_SET_METHOD(exports, "runSubclassTest", RunSubclassTest); } } // anonymous namespace diff --git a/test/addons/async-resource/test.js b/test/addons/async-resource/test.js index f19ad58637f187..c37d4df83d0103 100644 --- a/test/addons/async-resource/test.js +++ b/test/addons/async-resource/test.js @@ -5,6 +5,8 @@ const assert = require('assert'); const binding = require(`./build/${common.buildType}/binding`); const async_hooks = require('async_hooks'); +binding.runSubclassTest(); + const kObjectTag = Symbol('kObjectTag'); const rootAsyncId = async_hooks.executionAsyncId(); From 2a7c863d3d197ce833351411c74cd912846e04c9 Mon Sep 17 00:00:00 2001 From: Vse Mozhet Byt Date: Fri, 11 May 2018 17:04:12 +0300 Subject: [PATCH 086/208] test: modernize and correct test-doctool-html.js PR-URL: https://github.com/nodejs/node/pull/20676 Reviewed-By: Luigi Pinca Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Ruben Bridgewater --- test/doctool/test-doctool-html.js | 59 +++++++++++++------------------ 1 file changed, 25 insertions(+), 34 deletions(-) diff --git a/test/doctool/test-doctool-html.js b/test/doctool/test-doctool-html.js index 91a18d536a6992..185faa46f71886 100644 --- a/test/doctool/test-doctool-html.js +++ b/test/doctool/test-doctool-html.js @@ -9,16 +9,16 @@ try { } const assert = require('assert'); -const fs = require('fs'); +const { readFile } = require('fs'); const fixtures = require('../common/fixtures'); const processIncludes = require('../../tools/doc/preprocess.js'); -const html = require('../../tools/doc/html.js'); +const toHTML = require('../../tools/doc/html.js'); // Test data is a list of objects with two properties. // The file property is the file path. -// The html property is some html which will be generated by the doctool. -// This html will be stripped of all whitespace because we don't currently -// have an html parser. +// The html property is some HTML which will be generated by the doctool. +// This HTML will be stripped of all whitespace because we don't currently +// have an HTML parser. const testData = [ { file: fixtures.path('sample_document.md'), @@ -32,8 +32,7 @@ const testData = [ 'id="foo_class_method_buffer_from_array"># ' + '' + 'Reference/Global_Objects/Array" class="type"><Array>' }, { file: fixtures.path('doc_with_yaml.md'), @@ -45,42 +44,34 @@ const testData = [ ' ' + '

    Describe Foobar in more detail here.

    ' + '

    Foobar II#

    ' + - ' ' + '

    Describe Foobar II in more detail here.' + 'fg(1)

    ' + '

    Deprecated thingy#' + - '

    ' + - '