From e5f101fe7beba032089ae3b3fb425c0ee15df258 Mon Sep 17 00:00:00 2001 From: Peter Marton Date: Thu, 5 Oct 2017 14:24:12 +0200 Subject: [PATCH 01/23] http2: add req and res options to server creation Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. PR-URL: https://github.com/nodejs/node/pull/15560 Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Anatoli Papirovski Reviewed-By: Anna Henningsen --- doc/api/http2.md | 8 ++++ lib/internal/http2/compat.js | 8 ++-- lib/internal/http2/core.js | 10 ++++- .../test-http2-options-server-request.js | 40 +++++++++++++++++++ .../test-http2-options-server-response.js | 34 ++++++++++++++++ 5 files changed, 95 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-http2-options-server-request.js create mode 100644 test/parallel/test-http2-options-server-response.js diff --git a/doc/api/http2.md b/doc/api/http2.md index 083a491cca3..8b428484c00 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1707,6 +1707,14 @@ changes: * `Http1ServerResponse` {http.ServerResponse} Specifies the ServerResponse class to used for HTTP/1 fallback. Useful for extending the original `http.ServerResponse`. **Default:** `http.ServerResponse` + * `Http2ServerRequest` {http2.Http2ServerRequest} Specifies the + Http2ServerRequest class to use. + Useful for extending the original `Http2ServerRequest`. + **Default:** `Http2ServerRequest` + * `Http2ServerResponse` {htt2.Http2ServerResponse} Specifies the + Http2ServerResponse class to use. + Useful for extending the original `Http2ServerResponse`. + **Default:** `Http2ServerResponse` * `onRequestHandler` {Function} See [Compatibility API][] * Returns: {Http2Server} diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index b5dd81c80f4..5e6c51377e9 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -661,11 +661,11 @@ class Http2ServerResponse extends Stream { } } -function onServerStream(stream, headers, flags, rawHeaders) { +function onServerStream(ServerRequest, ServerResponse, + stream, headers, flags, rawHeaders) { const server = this; - const request = new Http2ServerRequest(stream, headers, undefined, - rawHeaders); - const response = new Http2ServerResponse(stream); + const request = new ServerRequest(stream, headers, undefined, rawHeaders); + const response = new ServerResponse(stream); // Check for the CONNECT method const method = headers[HTTP2_HEADER_METHOD]; diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 5f811ca8af4..307ca98db8d 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2498,6 +2498,10 @@ function initializeOptions(options) { options.Http1ServerResponse = options.Http1ServerResponse || http.ServerResponse; + options.Http2ServerRequest = options.Http2ServerRequest || + Http2ServerRequest; + options.Http2ServerResponse = options.Http2ServerResponse || + Http2ServerResponse; return options; } @@ -2563,7 +2567,11 @@ class Http2Server extends NETServer { function setupCompat(ev) { if (ev === 'request') { this.removeListener('newListener', setupCompat); - this.on('stream', onServerStream); + this.on('stream', onServerStream.bind( + this, + this[kOptions].Http2ServerRequest, + this[kOptions].Http2ServerResponse) + ); } } diff --git a/test/parallel/test-http2-options-server-request.js b/test/parallel/test-http2-options-server-request.js new file mode 100644 index 00000000000..2143d379823 --- /dev/null +++ b/test/parallel/test-http2-options-server-request.js @@ -0,0 +1,40 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const h2 = require('http2'); + +class MyServerRequest extends h2.Http2ServerRequest { + getUserAgent() { + return this.headers['user-agent'] || 'unknown'; + } +} + +const server = h2.createServer({ + Http2ServerRequest: MyServerRequest +}, (req, res) => { + assert.strictEqual(req.getUserAgent(), 'node-test'); + + res.writeHead(200, { 'Content-Type': 'text/plain' }); + res.end(); +}); +server.listen(0); + +server.on('listening', common.mustCall(() => { + + const client = h2.connect(`http://localhost:${server.address().port}`); + const req = client.request({ + ':path': '/', + 'User-Agent': 'node-test' + }); + + req.on('response', common.mustCall()); + + req.resume(); + req.on('end', common.mustCall(() => { + server.close(); + client.destroy(); + })); +})); diff --git a/test/parallel/test-http2-options-server-response.js b/test/parallel/test-http2-options-server-response.js new file mode 100644 index 00000000000..6f1ae1881d2 --- /dev/null +++ b/test/parallel/test-http2-options-server-response.js @@ -0,0 +1,34 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const h2 = require('http2'); + +class MyServerResponse extends h2.Http2ServerResponse { + status(code) { + return this.writeHead(code, { 'Content-Type': 'text/plain' }); + } +} + +const server = h2.createServer({ + Http2ServerResponse: MyServerResponse +}, (req, res) => { + res.status(200); + res.end(); +}); +server.listen(0); + +server.on('listening', common.mustCall(() => { + + const client = h2.connect(`http://localhost:${server.address().port}`); + const req = client.request({ ':path': '/' }); + + req.on('response', common.mustCall()); + + req.resume(); + req.on('end', common.mustCall(() => { + server.close(); + client.destroy(); + })); +})); From 0f9efef05deb11dbbdf5adf96460839f5b332207 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 5 Feb 2018 14:29:32 -0500 Subject: [PATCH 02/23] timers: refactor timer list processing Instead of using kOnTimeout index to track a special list processing function, just pass in a function to C++ at startup that executes all handles and determines which function to call. This change improves the performance of unpooled timeouts by roughly 20%, as well as makes the unref/ref processing easier to follow. PR-URL: https://github.com/nodejs/node/pull/18582 Reviewed-By: Ruben Bridgewater Reviewed-By: Matteo Collina Reviewed-By: Vladimir de Turckheim Reviewed-By: Evan Lucas --- benchmark/timers/timers-timeout-unpooled.js | 36 +++++++++++++++++++ lib/timers.js | 38 ++++++++++----------- src/env.h | 1 + src/timer_wrap.cc | 17 ++++----- test/message/timeout_throw.out | 3 +- 5 files changed, 67 insertions(+), 28 deletions(-) create mode 100644 benchmark/timers/timers-timeout-unpooled.js diff --git a/benchmark/timers/timers-timeout-unpooled.js b/benchmark/timers/timers-timeout-unpooled.js new file mode 100644 index 00000000000..19f0f6a4af4 --- /dev/null +++ b/benchmark/timers/timers-timeout-unpooled.js @@ -0,0 +1,36 @@ +'use strict'; +const common = require('../common.js'); + +// The following benchmark sets up n * 1e6 unpooled timeouts, +// then measures their execution on the next uv tick + +const bench = common.createBenchmark(main, { + n: [1e6], +}); + +function main({ n }) { + let count = 0; + + // Function tracking on the hidden class in V8 can cause misleading + // results in this benchmark if only a single function is used — + // alternate between two functions for a fairer benchmark + + function cb() { + count++; + if (count === n) + bench.end(n); + } + function cb2() { + count++; + if (count === n) + bench.end(n); + } + + for (var i = 0; i < n; i++) { + // unref().ref() will cause each of these timers to + // allocate their own handle + setTimeout(i % 2 ? cb : cb2, 1).unref().ref(); + } + + bench.start(); +} diff --git a/lib/timers.js b/lib/timers.js index 39a22454ac9..8e914d751a9 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -24,7 +24,7 @@ const async_wrap = process.binding('async_wrap'); const { Timer: TimerWrap, - setImmediateCallback, + setupTimers, } = process.binding('timer_wrap'); const L = require('internal/linkedlist'); const timerInternals = require('internal/timers'); @@ -34,7 +34,6 @@ const assert = require('assert'); const util = require('util'); const errors = require('internal/errors'); const debug = util.debuglog('timer'); -const kOnTimeout = TimerWrap.kOnTimeout | 0; // Two arrays that share state between C++ and JS. const { async_hook_fields, async_id_fields } = async_wrap; const { @@ -57,7 +56,7 @@ const kRefCount = 1; const kHasOutstanding = 2; const [immediateInfo, toggleImmediateRef] = - setImmediateCallback(processImmediate); + setupTimers(processImmediate, processTimers); const kRefed = Symbol('refed'); @@ -221,10 +220,14 @@ function TimersList(msecs, unrefed) { timer.start(msecs); } -// adds listOnTimeout to the C++ object prototype, as -// V8 would not inline it otherwise. -TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) { - const list = this._list; +function processTimers(now) { + if (this.owner) + return unrefdHandle(this.owner, now); + return listOnTimeout(this, now); +} + +function listOnTimeout(handle, now) { + const list = handle._list; const msecs = list.msecs; debug('timeout callback %d', msecs); @@ -241,7 +244,7 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) { if (timeRemaining <= 0) { timeRemaining = 1; } - this.start(timeRemaining); + handle.start(timeRemaining); debug('%d list wait because diff is %d', msecs, diff); return true; } @@ -280,11 +283,11 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) { // Do not close the underlying handle if its ownership has changed // (e.g it was unrefed in its callback). - if (!this.owner) - this.close(); + if (!handle.owner) + handle.close(); return true; -}; +} // An optimization so that the try/finally only de-optimizes (since at least v8 @@ -516,18 +519,17 @@ exports.clearInterval = function(timer) { }; -function unrefdHandle(now) { +function unrefdHandle(timer, now) { try { // Don't attempt to call the callback if it is not a function. - if (typeof this.owner._onTimeout === 'function') { - tryOnTimeout(this.owner, now); + if (typeof timer._onTimeout === 'function') { + tryOnTimeout(timer, now); } } finally { // Make sure we clean up if the callback is no longer a function // even if the timer is an interval. - if (!this.owner._repeat || - typeof this.owner._onTimeout !== 'function') { - this.owner.close(); + if (!timer._repeat || typeof timer._onTimeout !== 'function') { + timer.close(); } } @@ -557,7 +559,6 @@ Timeout.prototype.unref = function() { this._handle = handle || new TimerWrap(); this._handle.owner = this; - this._handle[kOnTimeout] = unrefdHandle; this._handle.start(delay); this._handle.unref(); } @@ -581,7 +582,6 @@ Timeout.prototype.close = function() { } this._idleTimeout = -1; - this._handle[kOnTimeout] = null; this._handle.close(); } else { unenroll(this); diff --git a/src/env.h b/src/env.h index b621a54e378..95548c0900e 100644 --- a/src/env.h +++ b/src/env.h @@ -308,6 +308,7 @@ class ModuleWrap; V(secure_context_constructor_template, v8::FunctionTemplate) \ V(tcp_constructor_template, v8::FunctionTemplate) \ V(tick_callback_function, v8::Function) \ + V(timers_callback_function, v8::Function) \ V(tls_wrap_constructor_function, v8::Function) \ V(tty_constructor_template, v8::FunctionTemplate) \ V(udp_constructor_function, v8::Function) \ diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index 441974ae77b..02c0b816698 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -41,8 +41,6 @@ using v8::Object; using v8::String; using v8::Value; -const uint32_t kOnTimeout = 0; - class TimerWrap : public HandleWrap { public: static void Initialize(Local target, @@ -53,8 +51,6 @@ class TimerWrap : public HandleWrap { Local timerString = FIXED_ONE_BYTE_STRING(env->isolate(), "Timer"); constructor->InstanceTemplate()->SetInternalFieldCount(1); constructor->SetClassName(timerString); - constructor->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kOnTimeout"), - Integer::New(env->isolate(), kOnTimeout)); env->SetTemplateMethod(constructor, "now", Now); @@ -71,18 +67,22 @@ class TimerWrap : public HandleWrap { target->Set(timerString, constructor->GetFunction()); target->Set(env->context(), - FIXED_ONE_BYTE_STRING(env->isolate(), "setImmediateCallback"), - env->NewFunctionTemplate(SetImmediateCallback) + FIXED_ONE_BYTE_STRING(env->isolate(), "setupTimers"), + env->NewFunctionTemplate(SetupTimers) ->GetFunction(env->context()).ToLocalChecked()).FromJust(); } size_t self_size() const override { return sizeof(*this); } private: - static void SetImmediateCallback(const FunctionCallbackInfo& args) { + static void SetupTimers(const FunctionCallbackInfo& args) { CHECK(args[0]->IsFunction()); + CHECK(args[1]->IsFunction()); auto env = Environment::GetCurrent(args); + env->set_immediate_callback_function(args[0].As()); + env->set_timers_callback_function(args[1].As()); + auto toggle_ref_cb = [] (const FunctionCallbackInfo& args) { Environment::GetCurrent(args)->ToggleImmediateRef(args[0]->IsTrue()); }; @@ -142,7 +142,8 @@ class TimerWrap : public HandleWrap { Local args[1]; do { args[0] = env->GetNow(); - ret = wrap->MakeCallback(kOnTimeout, 1, args).ToLocalChecked(); + ret = wrap->MakeCallback(env->timers_callback_function(), 1, args) + .ToLocalChecked(); } while (ret->IsUndefined() && !env->tick_info()->has_thrown() && wrap->object()->Get(env->context(), diff --git a/test/message/timeout_throw.out b/test/message/timeout_throw.out index 9ef4f63e3d8..382cf5a0549 100644 --- a/test/message/timeout_throw.out +++ b/test/message/timeout_throw.out @@ -5,4 +5,5 @@ ReferenceError: undefined_reference_error_maker is not defined at Timeout._onTimeout (*test*message*timeout_throw.js:*:*) at ontimeout (timers.js:*:*) at tryOnTimeout (timers.js:*:*) - at Timer.listOnTimeout (timers.js:*:*) + at listOnTimeout (timers.js:*:*) + at Timer.processTimers (timers.js:*:*) From 523a1550a3d07ecc3d13e071cb0f1c732bae3bad Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Thu, 1 Feb 2018 15:25:41 -0800 Subject: [PATCH 03/23] async_hooks: deprecate unsafe emit{Before,After} The emit{Before,After} APIs in AsyncResource are problematic. * emit{Before,After} are named to suggest that the only thing they do is emit the before and after hooks. However, they in fact, mutate the current execution context. * They must be properly nested. Failure to do so by user code leads to catastrophic (unrecoverable) exceptions. It is very easy for the users to forget that they must be using a try/finally block around the code that must be surrounded by these operations. Even the example provided in the official docs makes this mistake. Failing to use a finally can lead to a catastrophic crash if the callback ends up throwing. This change provides a safer `runInAsyncScope` API as an alternative and deprecates emit{Before,After}. PR-URL: https://github.com/nodejs/node/pull/18513 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Andreas Madsen --- doc/api/async_hooks.md | 64 +++++++++++++++---- doc/api/deprecations.md | 13 ++++ lib/async_hooks.js | 24 +++++++ ...dder.api.async-resource.runInAsyncScope.js | 11 ++++ ...c-hooks-recursive-stack-runInAsyncScope.js | 20 ++++++ ...fter-uncaught-exception-runInAsyncScope.js | 40 ++++++++++++ 6 files changed, 160 insertions(+), 12 deletions(-) create mode 100644 test/async-hooks/test-embedder.api.async-resource.runInAsyncScope.js create mode 100644 test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js create mode 100644 test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js diff --git a/doc/api/async_hooks.md b/doc/api/async_hooks.md index d854d737dce..4813169bfd8 100644 --- a/doc/api/async_hooks.md +++ b/doc/api/async_hooks.md @@ -599,10 +599,6 @@ own resources. The `init` hook will trigger when an `AsyncResource` is instantiated. -The `before` and `after` calls must be unwound in the same order that they -are called. Otherwise, an unrecoverable exception will occur and the process -will abort. - The following is an overview of the `AsyncResource` API. ```js @@ -615,11 +611,13 @@ const asyncResource = new AsyncResource( type, { triggerAsyncId: executionAsyncId(), requireManualDestroy: false } ); -// Call AsyncHooks before callbacks. -asyncResource.emitBefore(); - -// Call AsyncHooks after callbacks. -asyncResource.emitAfter(); +// Run a function in the execution context of the resource. This will +// * establish the context of the resource +// * trigger the AsyncHooks before callbacks +// * call the provided function `fn` with the supplied arguments +// * trigger the AsyncHooks after callbacks +// * restore the original execution context +asyncResource.runInAsyncScope(fn, thisArg, ...args); // Call AsyncHooks destroy callbacks. asyncResource.emitDestroy(); @@ -629,6 +627,14 @@ asyncResource.asyncId(); // Return the trigger ID for the AsyncResource instance. asyncResource.triggerAsyncId(); + +// Call AsyncHooks before callbacks. +// Deprecated: Use asyncResource.runInAsyncScope instead. +asyncResource.emitBefore(); + +// Call AsyncHooks after callbacks. +// Deprecated: Use asyncResource.runInAsyncScope instead. +asyncResource.emitAfter(); ``` #### `AsyncResource(type[, options])` @@ -654,9 +660,7 @@ class DBQuery extends AsyncResource { getInfo(query, callback) { this.db.get(query, (err, data) => { - this.emitBefore(); - callback(err, data); - this.emitAfter(); + this.runInAsyncScope(callback, null, err, data); }); } @@ -667,7 +671,26 @@ class DBQuery extends AsyncResource { } ``` +#### `asyncResource.runInAsyncScope(fn[, thisArg, ...args])` + + +* `fn` {Function} The function to call in the execution context of this async + resource. +* `thisArg` {any} The receiver to be used for the function call. +* `...args` {any} Optional arguments to pass to the function. + +Call the provided function with the provided arguments in the execution context +of the async resource. This will establish the context, trigger the AsyncHooks +before callbacks, call the function, trigger the AsyncHooks after callbacks, and +then restore the original execution context. + #### `asyncResource.emitBefore()` + +> Stability: 0 - Deprecated: Use [`asyncResource.runInAsyncScope()`][] instead. * Returns: {undefined} @@ -675,7 +698,17 @@ Call all `before` callbacks to notify that a new asynchronous execution context is being entered. If nested calls to `emitBefore()` are made, the stack of `asyncId`s will be tracked and properly unwound. +`before` and `after` calls must be unwound in the same order that they +are called. Otherwise, an unrecoverable exception will occur and the process +will abort. For this reason, the `emitBefore` and `emitAfter` APIs are +considered deprecated. Please use `runInAsyncScope`, as it provides a much safer +alternative. + #### `asyncResource.emitAfter()` + +> Stability: 0 - Deprecated: Use [`asyncResource.runInAsyncScope()`][] instead. * Returns: {undefined} @@ -686,6 +719,12 @@ If the user's callback throws an exception, `emitAfter()` will automatically be called for all `asyncId`s on the stack if the error is handled by a domain or `'uncaughtException'` handler. +`before` and `after` calls must be unwound in the same order that they +are called. Otherwise, an unrecoverable exception will occur and the process +will abort. For this reason, the `emitBefore` and `emitAfter` APIs are +considered deprecated. Please use `runInAsyncScope`, as it provides a much safer +alternative. + #### `asyncResource.emitDestroy()` * Returns: {undefined} @@ -705,6 +744,7 @@ never be called. constructor. [`after` callback]: #async_hooks_after_asyncid +[`asyncResource.runInAsyncScope()`]: #async_hooks_asyncresource_runinasyncscope_fn_thisarg_args [`before` callback]: #async_hooks_before_asyncid [`destroy` callback]: #async_hooks_destroy_asyncid [`init` callback]: #async_hooks_init_asyncid_type_triggerasyncid_resource diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 1f5809624af..c18c9d58e0b 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -880,6 +880,18 @@ Users of `MakeCallback` that add the `domain` property to carry context, should start using the `async_context` variant of `MakeCallback` or `CallbackScope`, or the the high-level `AsyncResource` class. + +### DEP0098: AsyncHooks Embedder AsyncResource.emit{Before,After} APIs + +Type: Runtime + +The embedded API provided by AsyncHooks exposes emit{Before,After} methods +which are very easy to use incorrectly which can lead to unrecoverable errors. + +Use [`asyncResource.runInAsyncScope()`][] API instead which provides a much +safer, and more convenient, alternative. See +https://github.com/nodejs/node/pull/18513 for more details. + [`--pending-deprecation`]: cli.html#cli_pending_deprecation [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array @@ -892,6 +904,7 @@ should start using the `async_context` variant of `MakeCallback` or [`Server.getConnections()`]: net.html#net_server_getconnections_callback [`Server.listen({fd: })`]: net.html#net_server_listen_handle_backlog_callback [`SlowBuffer`]: buffer.html#buffer_class_slowbuffer +[`asyncResource.runInAsyncScope()`]: async_hooks.html#async_hooks_asyncresource_runinasyncscope_fn_thisarg_args [`child_process`]: child_process.html [`console.error()`]: console.html#console_console_error_data_args [`console.log()`]: console.html#console_console_log_data_args diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 55a16a569e9..e7450f29acc 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -139,6 +139,17 @@ function triggerAsyncId() { const destroyedSymbol = Symbol('destroyed'); +let emitBeforeAfterWarning = true; +function showEmitBeforeAfterWarning() { + if (emitBeforeAfterWarning) { + process.emitWarning( + 'asyncResource.emitBefore and emitAfter are deprecated. Please use ' + + 'asyncResource.runInAsyncScope instead', + 'DeprecationWarning', 'DEP00XX'); + emitBeforeAfterWarning = false; + } +} + class AsyncResource { constructor(type, opts = {}) { if (typeof type !== 'string') @@ -174,15 +185,28 @@ class AsyncResource { } emitBefore() { + showEmitBeforeAfterWarning(); emitBefore(this[async_id_symbol], this[trigger_async_id_symbol]); return this; } emitAfter() { + showEmitBeforeAfterWarning(); emitAfter(this[async_id_symbol]); return this; } + runInAsyncScope(fn, thisArg, ...args) { + emitBefore(this[async_id_symbol], this[trigger_async_id_symbol]); + let ret; + try { + ret = Reflect.apply(fn, thisArg, args); + } finally { + emitAfter(this[async_id_symbol]); + } + return ret; + } + emitDestroy() { this[destroyedSymbol].destroyed = true; emitDestroy(this[async_id_symbol]); diff --git a/test/async-hooks/test-embedder.api.async-resource.runInAsyncScope.js b/test/async-hooks/test-embedder.api.async-resource.runInAsyncScope.js new file mode 100644 index 00000000000..627880b4d98 --- /dev/null +++ b/test/async-hooks/test-embedder.api.async-resource.runInAsyncScope.js @@ -0,0 +1,11 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); + +// Ensure that asyncResource.makeCallback returns the callback return value. +const a = new async_hooks.AsyncResource('foobar'); +const ret = a.runInAsyncScope(() => { + return 1729; +}); +assert.strictEqual(ret, 1729); diff --git a/test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js b/test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js new file mode 100644 index 00000000000..bc4ac86e7f1 --- /dev/null +++ b/test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js @@ -0,0 +1,20 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); + +// This test verifies that the async ID stack can grow indefinitely. + +function recurse(n) { + const a = new async_hooks.AsyncResource('foobar'); + a.runInAsyncScope(() => { + assert.strictEqual(a.asyncId(), async_hooks.executionAsyncId()); + assert.strictEqual(a.triggerAsyncId(), async_hooks.triggerAsyncId()); + if (n >= 0) + recurse(n - 1); + assert.strictEqual(a.asyncId(), async_hooks.executionAsyncId()); + assert.strictEqual(a.triggerAsyncId(), async_hooks.triggerAsyncId()); + }); +} + +recurse(1000); diff --git a/test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js b/test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js new file mode 100644 index 00000000000..5003972e998 --- /dev/null +++ b/test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js @@ -0,0 +1,40 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); + +const id_obj = {}; +let collect = true; + +const hook = async_hooks.createHook({ + before(id) { if (collect) id_obj[id] = true; }, + after(id) { delete id_obj[id]; }, +}).enable(); + +process.once('uncaughtException', common.mustCall((er) => { + assert.strictEqual(er.message, 'bye'); + collect = false; +})); + +setImmediate(common.mustCall(() => { + process.nextTick(common.mustCall(() => { + assert.strictEqual(Object.keys(id_obj).length, 0); + hook.disable(); + })); + + // Create a stack of async ids that will need to be emitted in the case of + // an uncaught exception. + const ar1 = new async_hooks.AsyncResource('Mine'); + ar1.runInAsyncScope(() => { + const ar2 = new async_hooks.AsyncResource('Mine'); + ar2.runInAsyncScope(() => { + throw new Error('bye'); + }); + }); + + // TODO(trevnorris): This test shows that the after() hooks are always called + // correctly, but it doesn't solve where the emitDestroy() is missed because + // of the uncaught exception. Simple solution is to always call emitDestroy() + // before the emitAfter(), but how to codify this? +})); From edffad075e1dac0ebea7e70b05e4e49528fc59d2 Mon Sep 17 00:00:00 2001 From: JiaHerr Tee Date: Thu, 8 Feb 2018 18:38:38 -0500 Subject: [PATCH 04/23] test: add url type check in Module options The code coverage in `root/internal/vm/Module.js` lacked test coverage for the url options paramter. The test adds a check to ensure error is thrown. PR-URL: https://github.com/nodejs/node/pull/18664 Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: Gus Caplan Reviewed-By: Yuta Hiroto Reviewed-By: James M Snell --- test/parallel/test-vm-module-errors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-vm-module-errors.js b/test/parallel/test-vm-module-errors.js index 1c7e01cf30e..424d35e8aaf 100644 --- a/test/parallel/test-vm-module-errors.js +++ b/test/parallel/test-vm-module-errors.js @@ -44,7 +44,7 @@ async function checkArgType() { }); for (const invalidOptions of [ - 0, 1, null, true, 'str', () => {}, Symbol.iterator + 0, 1, null, true, 'str', () => {}, { url: 0 }, Symbol.iterator ]) { common.expectsError(() => { new Module('', invalidOptions); From 37c88f080de987d6adf9e2b4f7fe65cca1f5ecc2 Mon Sep 17 00:00:00 2001 From: Justin Lee Date: Fri, 9 Feb 2018 09:02:50 -0800 Subject: [PATCH 05/23] doc: fix links to Style Guide and CPP Style Guide PR-URL: https://github.com/nodejs/node/pull/18683 Reviewed-By: Vse Mozhet Byt Reviewed-By: Joyee Cheung Reviewed-By: Ruben Bridgewater Reviewed-By: Richard Lau Reviewed-By: Colin Ihrig --- doc/guides/contributing/pull-requests.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/guides/contributing/pull-requests.md b/doc/guides/contributing/pull-requests.md index 5812c8c5464..3d7c548bbd3 100644 --- a/doc/guides/contributing/pull-requests.md +++ b/doc/guides/contributing/pull-requests.md @@ -109,12 +109,12 @@ If you are modifying code, please be sure to run `make lint` from time to time to ensure that the changes follow the Node.js code style guide. Any documentation you write (including code comments and API documentation) -should follow the [Style Guide](doc/STYLE_GUIDE.md). Code samples included +should follow the [Style Guide](../../STYLE_GUIDE.md). Code samples included in the API docs will also be checked when running `make lint` (or `vcbuild.bat lint` on Windows). For contributing C++ code, you may want to look at the -[C++ Style Guide](CPP_STYLE_GUIDE.md). +[C++ Style Guide](../../../CPP_STYLE_GUIDE.md). ### Step 4: Commit From 9b4aa78f720284b5ad43850bbef90400ca010845 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 7 Feb 2018 00:07:08 +0100 Subject: [PATCH 06/23] test: refactor assert test This adds puctiations to the comments, uses a capital letters for the first character, removes a few obsolete comments and switches to assert.ok when suitable. It also moves all `assert.deepEqual()` and `assert.deepStrictEqual()` tests to the appropriate file. PR-URL: https://github.com/nodejs/node/pull/18610 Reviewed-By: Joyee Cheung --- test/parallel/test-assert-deep.js | 299 +++++++++++++++++++- test/parallel/test-assert.js | 436 ++++-------------------------- 2 files changed, 349 insertions(+), 386 deletions(-) diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index 8f08d571383..1fad9f40517 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -1,7 +1,9 @@ 'use strict'; + const common = require('../common'); const assert = require('assert'); const util = require('util'); +const { AssertionError } = assert; // Template tag function turning an error message into a RegExp // for assert.throws() @@ -25,7 +27,7 @@ function re(literals, ...values) { // That is why we discourage using deepEqual in our own tests. // Turn off no-restricted-properties because we are testing deepEqual! -/* eslint-disable no-restricted-properties */ +/* eslint-disable no-restricted-properties, prefer-common-expectserror */ const arr = new Uint8Array([120, 121, 122, 10]); const buf = Buffer.from(arr); @@ -545,4 +547,299 @@ assertDeepAndStrictEqual(-0, -0); assertDeepAndStrictEqual(a, b); } +assert.doesNotThrow( + () => assert.deepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14)), + 'deepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))'); + +assert.throws(() => assert.deepEqual(new Date(), new Date(2000, 3, 14)), + AssertionError, + 'deepEqual(new Date(), new Date(2000, 3, 14))'); + +assert.throws( + () => assert.notDeepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14)), + AssertionError, + 'notDeepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))' +); + +assert.doesNotThrow( + () => assert.notDeepEqual(new Date(), new Date(2000, 3, 14)), + 'notDeepEqual(new Date(), new Date(2000, 3, 14))' +); + +assert.doesNotThrow(() => assert.deepEqual(/a/, /a/)); +assert.doesNotThrow(() => assert.deepEqual(/a/g, /a/g)); +assert.doesNotThrow(() => assert.deepEqual(/a/i, /a/i)); +assert.doesNotThrow(() => assert.deepEqual(/a/m, /a/m)); +assert.doesNotThrow(() => assert.deepEqual(/a/igm, /a/igm)); +assert.throws(() => assert.deepEqual(/ab/, /a/), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '/ab/ deepEqual /a/' + }); +assert.throws(() => assert.deepEqual(/a/g, /a/), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '/a/g deepEqual /a/' + }); +assert.throws(() => assert.deepEqual(/a/i, /a/), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '/a/i deepEqual /a/' + }); +assert.throws(() => assert.deepEqual(/a/m, /a/), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '/a/m deepEqual /a/' + }); +assert.throws(() => assert.deepEqual(/a/igm, /a/im), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '/a/gim deepEqual /a/im' + }); + +{ + const re1 = /a/g; + re1.lastIndex = 3; + assert.doesNotThrow(() => assert.deepEqual(re1, /a/g)); +} + +assert.doesNotThrow(() => assert.deepEqual(4, '4'), 'deepEqual(4, \'4\')'); +assert.doesNotThrow(() => assert.deepEqual(true, 1), 'deepEqual(true, 1)'); +assert.throws(() => assert.deepEqual(4, '5'), + AssertionError, + 'deepEqual( 4, \'5\')'); + +// Having the same number of owned properties && the same set of keys. +assert.doesNotThrow(() => assert.deepEqual({ a: 4 }, { a: 4 })); +assert.doesNotThrow(() => assert.deepEqual({ a: 4, b: '2' }, { a: 4, b: '2' })); +assert.doesNotThrow(() => assert.deepEqual([4], ['4'])); +assert.throws( + () => assert.deepEqual({ a: 4 }, { a: 4, b: true }), AssertionError); +assert.doesNotThrow(() => assert.deepEqual(['a'], { 0: 'a' })); +assert.doesNotThrow(() => assert.deepEqual({ a: 4, b: '1' }, { b: '1', a: 4 })); +const a1 = [1, 2, 3]; +const a2 = [1, 2, 3]; +a1.a = 'test'; +a1.b = true; +a2.b = true; +a2.a = 'test'; +assert.throws(() => assert.deepEqual(Object.keys(a1), Object.keys(a2)), + AssertionError); +assert.doesNotThrow(() => assert.deepEqual(a1, a2)); + +// Having an identical prototype property. +const nbRoot = { + toString() { return `${this.first} ${this.last}`; } +}; + +function nameBuilder(first, last) { + this.first = first; + this.last = last; + return this; +} +nameBuilder.prototype = nbRoot; + +function nameBuilder2(first, last) { + this.first = first; + this.last = last; + return this; +} +nameBuilder2.prototype = nbRoot; + +const nb1 = new nameBuilder('Ryan', 'Dahl'); +let nb2 = new nameBuilder2('Ryan', 'Dahl'); + +assert.doesNotThrow(() => assert.deepEqual(nb1, nb2)); + +nameBuilder2.prototype = Object; +nb2 = new nameBuilder2('Ryan', 'Dahl'); +assert.doesNotThrow(() => assert.deepEqual(nb1, nb2)); + +// Primitives and object. +assert.throws(() => assert.deepEqual(null, {}), AssertionError); +assert.throws(() => assert.deepEqual(undefined, {}), AssertionError); +assert.throws(() => assert.deepEqual('a', ['a']), AssertionError); +assert.throws(() => assert.deepEqual('a', { 0: 'a' }), AssertionError); +assert.throws(() => assert.deepEqual(1, {}), AssertionError); +assert.throws(() => assert.deepEqual(true, {}), AssertionError); +assert.throws(() => assert.deepEqual(Symbol(), {}), AssertionError); + +// Primitive wrappers and object. +assert.doesNotThrow(() => assert.deepEqual(new String('a'), ['a']), + AssertionError); +assert.doesNotThrow(() => assert.deepEqual(new String('a'), { 0: 'a' }), + AssertionError); +assert.doesNotThrow(() => assert.deepEqual(new Number(1), {}), AssertionError); +assert.doesNotThrow(() => assert.deepEqual(new Boolean(true), {}), + AssertionError); + +// Same number of keys but different key names. +assert.throws(() => assert.deepEqual({ a: 1 }, { b: 1 }), AssertionError); + +assert.doesNotThrow( + () => assert.deepStrictEqual(new Date(2000, 3, 14), new Date(2000, 3, 14)), + 'deepStrictEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))' +); + +assert.throws( + () => assert.deepStrictEqual(new Date(), new Date(2000, 3, 14)), + AssertionError, + 'deepStrictEqual(new Date(), new Date(2000, 3, 14))' +); + +assert.throws( + () => assert.notDeepStrictEqual(new Date(2000, 3, 14), new Date(2000, 3, 14)), + AssertionError, + 'notDeepStrictEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))' +); + +assert.doesNotThrow( + () => assert.notDeepStrictEqual(new Date(), new Date(2000, 3, 14)), + 'notDeepStrictEqual(new Date(), new Date(2000, 3, 14))' +); + +assert.doesNotThrow(() => assert.deepStrictEqual(/a/, /a/)); +assert.doesNotThrow(() => assert.deepStrictEqual(/a/g, /a/g)); +assert.doesNotThrow(() => assert.deepStrictEqual(/a/i, /a/i)); +assert.doesNotThrow(() => assert.deepStrictEqual(/a/m, /a/m)); +assert.doesNotThrow(() => assert.deepStrictEqual(/a/igm, /a/igm)); +assert.throws( + () => assert.deepStrictEqual(/ab/, /a/), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '/ab/ deepStrictEqual /a/' + }); +assert.throws( + () => assert.deepStrictEqual(/a/g, /a/), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '/a/g deepStrictEqual /a/' + }); +assert.throws( + () => assert.deepStrictEqual(/a/i, /a/), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '/a/i deepStrictEqual /a/' + }); +assert.throws( + () => assert.deepStrictEqual(/a/m, /a/), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '/a/m deepStrictEqual /a/' + }); +assert.throws( + () => assert.deepStrictEqual(/a/igm, /a/im), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '/a/gim deepStrictEqual /a/im' + }); + +{ + const re1 = /a/; + re1.lastIndex = 3; + assert.doesNotThrow(() => assert.deepStrictEqual(re1, /a/)); +} + +assert.throws(() => assert.deepStrictEqual(4, '4'), + AssertionError, + 'deepStrictEqual(4, \'4\')'); + +assert.throws(() => assert.deepStrictEqual(true, 1), + AssertionError, + 'deepStrictEqual(true, 1)'); + +assert.throws(() => assert.deepStrictEqual(4, '5'), + AssertionError, + 'deepStrictEqual(4, \'5\')'); + +// Having the same number of owned properties && the same set of keys. +assert.doesNotThrow(() => assert.deepStrictEqual({ a: 4 }, { a: 4 })); +assert.doesNotThrow( + () => assert.deepStrictEqual({ a: 4, b: '2' }, { a: 4, b: '2' })); +assert.throws(() => assert.deepStrictEqual([4], ['4']), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: "[ 4 ] deepStrictEqual [ '4' ]" + }); +assert.throws(() => assert.deepStrictEqual({ a: 4 }, { a: 4, b: true }), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: '{ a: 4 } deepStrictEqual { a: 4, b: true }' + }); +assert.throws(() => assert.deepStrictEqual(['a'], { 0: 'a' }), + { + code: 'ERR_ASSERTION', + name: 'AssertionError [ERR_ASSERTION]', + message: "[ 'a' ] deepStrictEqual { '0': 'a' }" + }); + /* eslint-enable */ + +assert.doesNotThrow( + () => assert.deepStrictEqual({ a: 4, b: '1' }, { b: '1', a: 4 })); + +assert.throws( + () => assert.deepStrictEqual([0, 1, 2, 'a', 'b'], [0, 1, 2, 'b', 'a']), + AssertionError); + +assert.doesNotThrow(() => assert.deepStrictEqual(a1, a2)); + +// Prototype check. +function Constructor1(first, last) { + this.first = first; + this.last = last; +} + +function Constructor2(first, last) { + this.first = first; + this.last = last; +} + +const obj1 = new Constructor1('Ryan', 'Dahl'); +let obj2 = new Constructor2('Ryan', 'Dahl'); + +assert.throws(() => assert.deepStrictEqual(obj1, obj2), AssertionError); + +Constructor2.prototype = Constructor1.prototype; +obj2 = new Constructor2('Ryan', 'Dahl'); + +assert.doesNotThrow(() => assert.deepStrictEqual(obj1, obj2)); + +// primitives +assert.throws(() => assert.deepStrictEqual(4, '4'), AssertionError); +assert.throws(() => assert.deepStrictEqual(true, 1), AssertionError); +assert.throws(() => assert.deepStrictEqual(Symbol(), Symbol()), + AssertionError); + +const s = Symbol(); +assert.doesNotThrow(() => assert.deepStrictEqual(s, s)); + +// Primitives and object. +assert.throws(() => assert.deepStrictEqual(null, {}), AssertionError); +assert.throws(() => assert.deepStrictEqual(undefined, {}), AssertionError); +assert.throws(() => assert.deepStrictEqual('a', ['a']), AssertionError); +assert.throws(() => assert.deepStrictEqual('a', { 0: 'a' }), AssertionError); +assert.throws(() => assert.deepStrictEqual(1, {}), AssertionError); +assert.throws(() => assert.deepStrictEqual(true, {}), AssertionError); +assert.throws(() => assert.deepStrictEqual(Symbol(), {}), AssertionError); + +// Primitive wrappers and object. +assert.throws(() => assert.deepStrictEqual(new String('a'), ['a']), + AssertionError); +assert.throws(() => assert.deepStrictEqual(new String('a'), { 0: 'a' }), + AssertionError); +assert.throws(() => assert.deepStrictEqual(new Number(1), {}), AssertionError); +assert.throws(() => assert.deepStrictEqual(new Boolean(true), {}), + AssertionError); diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index e422acbbfbc..869011908c3 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -33,432 +33,104 @@ const { errorCache } = require('internal/errors'); const { writeFileSync, unlinkSync } = require('fs'); const a = assert; -function makeBlock(f) { - const args = Array.prototype.slice.call(arguments, 1); - return () => { - return f.apply(null, args); - }; -} - assert.ok(a.AssertionError.prototype instanceof Error, 'a.AssertionError instanceof Error'); -assert.throws(makeBlock(a, false), a.AssertionError, 'ok(false)'); +assert.throws(() => a(false), a.AssertionError, 'ok(false)'); -assert.doesNotThrow(makeBlock(a, true), a.AssertionError, 'ok(true)'); +assert.doesNotThrow(() => a(true), a.AssertionError, 'ok(true)'); -assert.doesNotThrow(makeBlock(a, 'test', 'ok(\'test\')')); +assert.doesNotThrow(() => a('test', 'ok(\'test\')')); -assert.throws(makeBlock(a.ok, false), - a.AssertionError, 'ok(false)'); +assert.throws(() => a.ok(false), a.AssertionError, 'ok(false)'); -assert.doesNotThrow(makeBlock(a.ok, true), - a.AssertionError, 'ok(true)'); +assert.doesNotThrow(() => a.ok(true), a.AssertionError, 'ok(true)'); -assert.doesNotThrow(makeBlock(a.ok, 'test'), 'ok(\'test\')'); +assert.doesNotThrow(() => a.ok('test'), 'ok(\'test\')'); -assert.throws(makeBlock(a.equal, true, false), +assert.throws(() => a.equal(true, false), a.AssertionError, 'equal(true, false)'); -assert.doesNotThrow(makeBlock(a.equal, null, null), - 'equal(null, null)'); +assert.doesNotThrow(() => a.equal(null, null), 'equal(null, null)'); -assert.doesNotThrow(makeBlock(a.equal, undefined, undefined), +assert.doesNotThrow(() => a.equal(undefined, undefined), 'equal(undefined, undefined)'); -assert.doesNotThrow(makeBlock(a.equal, null, undefined), - 'equal(null, undefined)'); +assert.doesNotThrow(() => a.equal(null, undefined), 'equal(null, undefined)'); -assert.doesNotThrow(makeBlock(a.equal, true, true), 'equal(true, true)'); +assert.doesNotThrow(() => a.equal(true, true), 'equal(true, true)'); -assert.doesNotThrow(makeBlock(a.equal, 2, '2'), 'equal(2, \'2\')'); +assert.doesNotThrow(() => a.equal(2, '2'), 'equal(2, \'2\')'); -assert.doesNotThrow(makeBlock(a.notEqual, true, false), - 'notEqual(true, false)'); +assert.doesNotThrow(() => a.notEqual(true, false), 'notEqual(true, false)'); -assert.throws(makeBlock(a.notEqual, true, true), +assert.throws(() => a.notEqual(true, true), a.AssertionError, 'notEqual(true, true)'); -assert.throws(makeBlock(a.strictEqual, 2, '2'), +assert.throws(() => a.strictEqual(2, '2'), a.AssertionError, 'strictEqual(2, \'2\')'); -assert.throws(makeBlock(a.strictEqual, null, undefined), +assert.throws(() => a.strictEqual(null, undefined), a.AssertionError, 'strictEqual(null, undefined)'); -assert.throws(makeBlock(a.notStrictEqual, 2, 2), +assert.throws(() => a.notStrictEqual(2, 2), a.AssertionError, 'notStrictEqual(2, 2)'); -assert.doesNotThrow(makeBlock(a.notStrictEqual, 2, '2'), - 'notStrictEqual(2, \'2\')'); - -// deepEqual joy! -assert.doesNotThrow(makeBlock(a.deepEqual, new Date(2000, 3, 14), - new Date(2000, 3, 14)), - 'deepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))'); - -assert.throws(makeBlock(a.deepEqual, new Date(), new Date(2000, 3, 14)), - a.AssertionError, - 'deepEqual(new Date(), new Date(2000, 3, 14))'); - -assert.throws( - makeBlock(a.notDeepEqual, new Date(2000, 3, 14), new Date(2000, 3, 14)), - a.AssertionError, - 'notDeepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))' -); - -assert.doesNotThrow(makeBlock( - a.notDeepEqual, - new Date(), - new Date(2000, 3, 14)), - 'notDeepEqual(new Date(), new Date(2000, 3, 14))' -); - -assert.doesNotThrow(makeBlock(a.deepEqual, /a/, /a/)); -assert.doesNotThrow(makeBlock(a.deepEqual, /a/g, /a/g)); -assert.doesNotThrow(makeBlock(a.deepEqual, /a/i, /a/i)); -assert.doesNotThrow(makeBlock(a.deepEqual, /a/m, /a/m)); -assert.doesNotThrow(makeBlock(a.deepEqual, /a/igm, /a/igm)); -assert.throws(makeBlock(a.deepEqual, /ab/, /a/), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\/ab\/ deepEqual \/a\/$/ - })); -assert.throws(makeBlock(a.deepEqual, /a/g, /a/), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\/a\/g deepEqual \/a\/$/ - })); -assert.throws(makeBlock(a.deepEqual, /a/i, /a/), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\/a\/i deepEqual \/a\/$/ - })); -assert.throws(makeBlock(a.deepEqual, /a/m, /a/), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\/a\/m deepEqual \/a\/$/ - })); -assert.throws(makeBlock(a.deepEqual, /a/igm, /a/im), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\/a\/gim deepEqual \/a\/im$/ - })); - -{ - const re1 = /a/g; - re1.lastIndex = 3; - assert.doesNotThrow(makeBlock(a.deepEqual, re1, /a/g)); -} - -assert.doesNotThrow(makeBlock(a.deepEqual, 4, '4'), 'deepEqual(4, \'4\')'); -assert.doesNotThrow(makeBlock(a.deepEqual, true, 1), 'deepEqual(true, 1)'); -assert.throws(makeBlock(a.deepEqual, 4, '5'), - a.AssertionError, - 'deepEqual( 4, \'5\')'); - -// having the same number of owned properties && the same set of keys -assert.doesNotThrow(makeBlock(a.deepEqual, { a: 4 }, { a: 4 })); -assert.doesNotThrow(makeBlock(a.deepEqual, { a: 4, b: '2' }, { a: 4, b: '2' })); -assert.doesNotThrow(makeBlock(a.deepEqual, [4], ['4'])); -assert.throws(makeBlock(a.deepEqual, { a: 4 }, { a: 4, b: true }), - a.AssertionError); -assert.doesNotThrow(makeBlock(a.deepEqual, ['a'], { 0: 'a' })); -//(although not necessarily the same order), -assert.doesNotThrow(makeBlock(a.deepEqual, { a: 4, b: '1' }, { b: '1', a: 4 })); -const a1 = [1, 2, 3]; -const a2 = [1, 2, 3]; -a1.a = 'test'; -a1.b = true; -a2.b = true; -a2.a = 'test'; -assert.throws(makeBlock(a.deepEqual, Object.keys(a1), Object.keys(a2)), - a.AssertionError); -assert.doesNotThrow(makeBlock(a.deepEqual, a1, a2)); - -// having an identical prototype property -const nbRoot = { - toString() { return `${this.first} ${this.last}`; } -}; - -function nameBuilder(first, last) { - this.first = first; - this.last = last; - return this; -} -nameBuilder.prototype = nbRoot; - -function nameBuilder2(first, last) { - this.first = first; - this.last = last; - return this; -} -nameBuilder2.prototype = nbRoot; - -const nb1 = new nameBuilder('Ryan', 'Dahl'); -let nb2 = new nameBuilder2('Ryan', 'Dahl'); - -assert.doesNotThrow(makeBlock(a.deepEqual, nb1, nb2)); - -nameBuilder2.prototype = Object; -nb2 = new nameBuilder2('Ryan', 'Dahl'); -assert.doesNotThrow(makeBlock(a.deepEqual, nb1, nb2)); - -// primitives and object -assert.throws(makeBlock(a.deepEqual, null, {}), a.AssertionError); -assert.throws(makeBlock(a.deepEqual, undefined, {}), a.AssertionError); -assert.throws(makeBlock(a.deepEqual, 'a', ['a']), a.AssertionError); -assert.throws(makeBlock(a.deepEqual, 'a', { 0: 'a' }), a.AssertionError); -assert.throws(makeBlock(a.deepEqual, 1, {}), a.AssertionError); -assert.throws(makeBlock(a.deepEqual, true, {}), a.AssertionError); -assert.throws(makeBlock(a.deepEqual, Symbol(), {}), a.AssertionError); - -// primitive wrappers and object -assert.doesNotThrow(makeBlock(a.deepEqual, new String('a'), ['a']), - a.AssertionError); -assert.doesNotThrow(makeBlock(a.deepEqual, new String('a'), { 0: 'a' }), - a.AssertionError); -assert.doesNotThrow(makeBlock(a.deepEqual, new Number(1), {}), - a.AssertionError); -assert.doesNotThrow(makeBlock(a.deepEqual, new Boolean(true), {}), - a.AssertionError); - -// same number of keys but different key names -assert.throws(makeBlock(a.deepEqual, { a: 1 }, { b: 1 }), a.AssertionError); - -//deepStrictEqual -assert.doesNotThrow( - makeBlock(a.deepStrictEqual, new Date(2000, 3, 14), new Date(2000, 3, 14)), - 'deepStrictEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))' -); - -assert.throws( - makeBlock(a.deepStrictEqual, new Date(), new Date(2000, 3, 14)), - a.AssertionError, - 'deepStrictEqual(new Date(), new Date(2000, 3, 14))' -); - -assert.throws( - makeBlock(a.notDeepStrictEqual, new Date(2000, 3, 14), new Date(2000, 3, 14)), - a.AssertionError, - 'notDeepStrictEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))' -); - -assert.doesNotThrow( - makeBlock(a.notDeepStrictEqual, new Date(), new Date(2000, 3, 14)), - 'notDeepStrictEqual(new Date(), new Date(2000, 3, 14))' -); - -assert.doesNotThrow(makeBlock(a.deepStrictEqual, /a/, /a/)); -assert.doesNotThrow(makeBlock(a.deepStrictEqual, /a/g, /a/g)); -assert.doesNotThrow(makeBlock(a.deepStrictEqual, /a/i, /a/i)); -assert.doesNotThrow(makeBlock(a.deepStrictEqual, /a/m, /a/m)); -assert.doesNotThrow(makeBlock(a.deepStrictEqual, /a/igm, /a/igm)); -assert.throws( - makeBlock(a.deepStrictEqual, /ab/, /a/), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\/ab\/ deepStrictEqual \/a\/$/ - })); -assert.throws( - makeBlock(a.deepStrictEqual, /a/g, /a/), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\/a\/g deepStrictEqual \/a\/$/ - })); -assert.throws( - makeBlock(a.deepStrictEqual, /a/i, /a/), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\/a\/i deepStrictEqual \/a\/$/ - })); -assert.throws( - makeBlock(a.deepStrictEqual, /a/m, /a/), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\/a\/m deepStrictEqual \/a\/$/ - })); -assert.throws( - makeBlock(a.deepStrictEqual, /a/igm, /a/im), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\/a\/gim deepStrictEqual \/a\/im$/ - })); - -{ - const re1 = /a/; - re1.lastIndex = 3; - assert.doesNotThrow(makeBlock(a.deepStrictEqual, re1, /a/)); -} - -assert.throws(makeBlock(a.deepStrictEqual, 4, '4'), - a.AssertionError, - 'deepStrictEqual(4, \'4\')'); - -assert.throws(makeBlock(a.deepStrictEqual, true, 1), - a.AssertionError, - 'deepStrictEqual(true, 1)'); - -assert.throws(makeBlock(a.deepStrictEqual, 4, '5'), - a.AssertionError, - 'deepStrictEqual(4, \'5\')'); - -// having the same number of owned properties && the same set of keys -assert.doesNotThrow(makeBlock(a.deepStrictEqual, { a: 4 }, { a: 4 })); -assert.doesNotThrow(makeBlock(a.deepStrictEqual, - { a: 4, b: '2' }, - { a: 4, b: '2' })); -assert.throws(makeBlock(a.deepStrictEqual, [4], ['4']), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\[ 4 ] deepStrictEqual \[ '4' ]$/ - })); -assert.throws(makeBlock(a.deepStrictEqual, { a: 4 }, { a: 4, b: true }), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^{ a: 4 } deepStrictEqual { a: 4, b: true }$/ - })); -assert.throws(makeBlock(a.deepStrictEqual, ['a'], { 0: 'a' }), - common.expectsError({ - code: 'ERR_ASSERTION', - type: a.AssertionError, - message: /^\[ 'a' ] deepStrictEqual { '0': 'a' }$/ - })); -//(although not necessarily the same order), -assert.doesNotThrow(makeBlock(a.deepStrictEqual, - { a: 4, b: '1' }, - { b: '1', a: 4 })); - -assert.throws(makeBlock(a.deepStrictEqual, - [0, 1, 2, 'a', 'b'], - [0, 1, 2, 'b', 'a']), - a.AssertionError); - -assert.doesNotThrow(makeBlock(a.deepStrictEqual, a1, a2)); - -// Prototype check -function Constructor1(first, last) { - this.first = first; - this.last = last; -} - -function Constructor2(first, last) { - this.first = first; - this.last = last; -} - -const obj1 = new Constructor1('Ryan', 'Dahl'); -let obj2 = new Constructor2('Ryan', 'Dahl'); - -assert.throws(makeBlock(a.deepStrictEqual, obj1, obj2), a.AssertionError); - -Constructor2.prototype = Constructor1.prototype; -obj2 = new Constructor2('Ryan', 'Dahl'); - -assert.doesNotThrow(makeBlock(a.deepStrictEqual, obj1, obj2)); - -// primitives -assert.throws(makeBlock(assert.deepStrictEqual, 4, '4'), - a.AssertionError); -assert.throws(makeBlock(assert.deepStrictEqual, true, 1), - a.AssertionError); -assert.throws(makeBlock(assert.deepStrictEqual, Symbol(), Symbol()), - a.AssertionError); - -const s = Symbol(); -assert.doesNotThrow(makeBlock(assert.deepStrictEqual, s, s)); - +assert.doesNotThrow(() => a.notStrictEqual(2, '2'), 'notStrictEqual(2, \'2\')'); -// primitives and object -assert.throws(makeBlock(a.deepStrictEqual, null, {}), a.AssertionError); -assert.throws(makeBlock(a.deepStrictEqual, undefined, {}), a.AssertionError); -assert.throws(makeBlock(a.deepStrictEqual, 'a', ['a']), a.AssertionError); -assert.throws(makeBlock(a.deepStrictEqual, 'a', { 0: 'a' }), a.AssertionError); -assert.throws(makeBlock(a.deepStrictEqual, 1, {}), a.AssertionError); -assert.throws(makeBlock(a.deepStrictEqual, true, {}), a.AssertionError); -assert.throws(makeBlock(assert.deepStrictEqual, Symbol(), {}), - a.AssertionError); - - -// primitive wrappers and object -assert.throws(makeBlock(a.deepStrictEqual, new String('a'), ['a']), - a.AssertionError); -assert.throws(makeBlock(a.deepStrictEqual, new String('a'), { 0: 'a' }), - a.AssertionError); -assert.throws(makeBlock(a.deepStrictEqual, new Number(1), {}), - a.AssertionError); -assert.throws(makeBlock(a.deepStrictEqual, new Boolean(true), {}), - a.AssertionError); - - -// Testing the throwing +// Testing the throwing. function thrower(errorConstructor) { throw new errorConstructor({}); } -// the basic calls work -assert.throws(makeBlock(thrower, a.AssertionError), - a.AssertionError, 'message'); -assert.throws(makeBlock(thrower, a.AssertionError), a.AssertionError); +// The basic calls work. +assert.throws(() => thrower(a.AssertionError), a.AssertionError, 'message'); +assert.throws(() => thrower(a.AssertionError), a.AssertionError); // eslint-disable-next-line no-restricted-syntax -assert.throws(makeBlock(thrower, a.AssertionError)); +assert.throws(() => thrower(a.AssertionError)); -// if not passing an error, catch all. +// If not passing an error, catch all. // eslint-disable-next-line no-restricted-syntax -assert.throws(makeBlock(thrower, TypeError)); +assert.throws(() => thrower(TypeError)); -// when passing a type, only catch errors of the appropriate type +// When passing a type, only catch errors of the appropriate type. { let threw = false; try { - a.throws(makeBlock(thrower, TypeError), a.AssertionError); + a.throws(() => thrower(TypeError), a.AssertionError); } catch (e) { threw = true; assert.ok(e instanceof TypeError, 'type'); } - assert.strictEqual(true, threw, - 'a.throws with an explicit error is eating extra errors'); + assert.ok(threw, 'a.throws with an explicit error is eating extra errors'); } -// doesNotThrow should pass through all errors +// doesNotThrow should pass through all errors. { let threw = false; try { - a.doesNotThrow(makeBlock(thrower, TypeError), a.AssertionError); + a.doesNotThrow(() => thrower(TypeError), a.AssertionError); } catch (e) { threw = true; assert.ok(e instanceof TypeError); } - assert.strictEqual(true, threw, 'a.doesNotThrow with an explicit error is ' + - 'eating extra errors'); + assert(threw, 'a.doesNotThrow with an explicit error is eating extra errors'); } -// key difference is that throwing our correct error makes an assertion error +// Key difference is that throwing our correct error makes an assertion error. { let threw = false; try { - a.doesNotThrow(makeBlock(thrower, TypeError), TypeError); + a.doesNotThrow(() => thrower(TypeError), TypeError); } catch (e) { threw = true; assert.ok(e instanceof a.AssertionError); } - assert.strictEqual(true, threw, - 'a.doesNotThrow is not catching type matching errors'); + assert.ok(threw, 'a.doesNotThrow is not catching type matching errors'); } common.expectsError( - () => assert.doesNotThrow(makeBlock(thrower, Error), 'user message'), + () => assert.doesNotThrow(() => thrower(Error), 'user message'), { type: a.AssertionError, code: 'ERR_ASSERTION', @@ -468,7 +140,7 @@ common.expectsError( ); common.expectsError( - () => assert.doesNotThrow(makeBlock(thrower, Error), 'user message'), + () => assert.doesNotThrow(() => thrower(Error), 'user message'), { code: 'ERR_ASSERTION', message: /Got unwanted exception: user message\n\[object Object\]/ @@ -476,14 +148,14 @@ common.expectsError( ); common.expectsError( - () => assert.doesNotThrow(makeBlock(thrower, Error)), + () => assert.doesNotThrow(() => thrower(Error)), { code: 'ERR_ASSERTION', message: /Got unwanted exception\.\n\[object Object\]/ } ); -// make sure that validating using constructor really works +// Make sure that validating using constructor really works. { let threw = false; try { @@ -499,11 +171,11 @@ common.expectsError( assert.ok(threw, 'wrong constructor validation'); } -// use a RegExp to validate error message -a.throws(makeBlock(thrower, TypeError), /\[object Object\]/); +// Use a RegExp to validate the error message. +a.throws(() => thrower(TypeError), /\[object Object\]/); -// use a fn to validate error object -a.throws(makeBlock(thrower, TypeError), (err) => { +// Use a fn to validate the error object. +a.throws(() => thrower(TypeError), (err) => { if ((err instanceof TypeError) && /\[object Object\]/.test(err)) { return true; } @@ -512,18 +184,12 @@ a.throws(makeBlock(thrower, TypeError), (err) => { // https://github.com/nodejs/node/issues/3188 { let threw = false; - let AnotherErrorType; try { const ES6Error = class extends Error {}; - AnotherErrorType = class extends Error {}; - const functionThatThrows = () => { - throw new AnotherErrorType('foo'); - }; - - assert.throws(functionThatThrows, ES6Error); + assert.throws(() => { throw new AnotherErrorType('foo'); }, ES6Error); } catch (e) { threw = true; assert(e instanceof AnotherErrorType, @@ -533,7 +199,7 @@ a.throws(makeBlock(thrower, TypeError), (err) => { assert.ok(threw); } -// check messages from assert.throws() +// Check messages from assert.throws(). { const noop = () => {}; assert.throws( @@ -622,7 +288,7 @@ try { let threw = false; const rangeError = new RangeError('my range'); - // verify custom errors + // Verify custom errors. try { assert.strictEqual(1, 2, rangeError); } catch (e) { @@ -633,7 +299,7 @@ try { assert.ok(threw); threw = false; - // verify AssertionError is the result from doesNotThrow with custom Error + // Verify AssertionError is the result from doesNotThrow with custom Error. try { assert.doesNotThrow(() => { throw new TypeError('wrong type'); @@ -648,7 +314,7 @@ try { } { - // Verify that throws() and doesNotThrow() throw on non-function block + // Verify that throws() and doesNotThrow() throw on non-function block. function typeName(value) { return value === null ? 'null' : typeof value; } @@ -699,7 +365,7 @@ assert.throws(() => { })); { - // bad args to AssertionError constructor should throw TypeError + // Bad args to AssertionError constructor should throw TypeError. const args = [1, true, false, '', null, Infinity, Symbol('test'), undefined]; const re = /^The "options" argument must be of type Object$/; args.forEach((input) => { @@ -722,7 +388,7 @@ common.expectsError( } ); -// Test strict assert +// Test strict assert. { const a = require('assert'); const assert = require('assert').strict; @@ -771,7 +437,7 @@ common.expectsError( ); Error.stackTraceLimit = tmpLimit; - // Test error diffs + // Test error diffs. const colors = process.stdout.isTTY && process.stdout.getColorDepth() > 1; const start = 'Input A expected to deepStrictEqual input B:'; const actExp = colors ? @@ -936,7 +602,7 @@ common.expectsError( ); { - // Test caching + // Test caching. const fs = process.binding('fs'); const tmp = fs.close; fs.close = common.mustCall(tmp, 1); From b8f47b27571f8d763f811f017be3fb37d466c4fc Mon Sep 17 00:00:00 2001 From: "Steven R. Loomis" Date: Wed, 7 Feb 2018 16:42:21 -0800 Subject: [PATCH 07/23] src: add "icu::" prefix before ICU symbols In ICU 61.x, icu4c will no longer put its declarations in the global namespace. Everything will be in the "icu::" namespace (or icu_60:: in the linker). Prepare for this. https://ssl.icu-project.org/trac/ticket/13460 --- src/inspector_io.cc | 9 +++++---- src/node_i18n.cc | 2 +- tools/icu/iculslocs.cc | 14 +++++++------- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/inspector_io.cc b/src/inspector_io.cc index 9af4458c6b2..01ddc296b08 100644 --- a/src/inspector_io.cc +++ b/src/inspector_io.cc @@ -74,11 +74,11 @@ std::string StringViewToUtf8(const StringView& view) { size_t result_length = view.length() * sizeof(*source); std::string result(result_length, '\0'); - UnicodeString utf16(unicodeSource, view.length()); + icu::UnicodeString utf16(unicodeSource, view.length()); // ICU components for std::string compatibility are not enabled in build... bool done = false; while (!done) { - CheckedArrayByteSink sink(&result[0], result_length); + icu::CheckedArrayByteSink sink(&result[0], result_length); utf16.toUTF8(sink); result_length = sink.NumberOfBytesAppended(); result.resize(result_length); @@ -111,8 +111,9 @@ void ReleasePairOnAsyncClose(uv_handle_t* async) { } // namespace std::unique_ptr Utf8ToStringView(const std::string& message) { - UnicodeString utf16 = - UnicodeString::fromUTF8(StringPiece(message.data(), message.length())); + icu::UnicodeString utf16 = + icu::UnicodeString::fromUTF8(icu::StringPiece(message.data(), + message.length())); StringView view(reinterpret_cast(utf16.getBuffer()), utf16.length()); return StringBuffer::create(view); diff --git a/src/node_i18n.cc b/src/node_i18n.cc index 71ae6a00033..d65fc55ed1f 100644 --- a/src/node_i18n.cc +++ b/src/node_i18n.cc @@ -523,7 +523,7 @@ const char* GetVersion(const char* type, } else if (!strcmp(type, TYPE_UNICODE)) { return U_UNICODE_VERSION; } else if (!strcmp(type, TYPE_TZ)) { - return TimeZone::getTZDataVersion(*status); + return icu::TimeZone::getTZDataVersion(*status); } else if (!strcmp(type, TYPE_CLDR)) { UVersionInfo versionArray; ulocdata_getCLDRVersion(versionArray, status); diff --git a/tools/icu/iculslocs.cc b/tools/icu/iculslocs.cc index ca312b78356..3ceb8d2a4d8 100644 --- a/tools/icu/iculslocs.cc +++ b/tools/icu/iculslocs.cc @@ -64,7 +64,7 @@ int VERBOSE = 0; #define RES_INDEX "res_index" #define INSTALLEDLOCALES "InstalledLocales" -CharString packageName; +icu::CharString packageName; const char* locale = RES_INDEX; // locale referring to our index void usage() { @@ -147,7 +147,7 @@ int localeExists(const char* loc, UBool* exists) { if (VERBOSE > 1) { printf("Trying to open %s:%s\n", packageName.data(), loc); } - LocalUResourceBundlePointer aResource( + icu::LocalUResourceBundlePointer aResource( ures_openDirect(packageName.data(), loc, &status)); *exists = FALSE; if (U_SUCCESS(status)) { @@ -189,11 +189,11 @@ void printIndent(FILE* bf, int indent) { * @return 0 for OK, 1 for err */ int dumpAllButInstalledLocales(int lev, - LocalUResourceBundlePointer* bund, + icu::LocalUResourceBundlePointer* bund, FILE* bf, UErrorCode* status) { ures_resetIterator(bund->getAlias()); - LocalUResourceBundlePointer t; + icu::LocalUResourceBundlePointer t; while (U_SUCCESS(*status) && ures_hasNext(bund->getAlias())) { t.adoptInstead(ures_getNextResource(bund->getAlias(), t.orphan(), status)); ASSERT_SUCCESS(status, "while processing table"); @@ -254,10 +254,10 @@ int list(const char* toBundle) { printf("\"locale\": %s\n", locale); } - LocalUResourceBundlePointer bund( + icu::LocalUResourceBundlePointer bund( ures_openDirect(packageName.data(), locale, &status)); ASSERT_SUCCESS(&status, "while opening the bundle"); - LocalUResourceBundlePointer installedLocales( + icu::LocalUResourceBundlePointer installedLocales( // NOLINTNEXTLINE (readability/null_usage) ures_getByKey(bund.getAlias(), INSTALLEDLOCALES, NULL, &status)); ASSERT_SUCCESS(&status, "while fetching installed locales"); @@ -295,7 +295,7 @@ int list(const char* toBundle) { } // OK, now list them. - LocalUResourceBundlePointer subkey; + icu::LocalUResourceBundlePointer subkey; int validCount = 0; for (int32_t i = 0; i < count; i++) { From 6007a9cc0e361d428123e4c0f74024c6cd7815f4 Mon Sep 17 00:00:00 2001 From: Jeff Principe Date: Mon, 16 Oct 2017 21:23:29 -0700 Subject: [PATCH 08/23] https: add extra options to Agent#getName() Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. PR-URL: https://github.com/nodejs/node/pull/16402 Reviewed-By: Luigi Pinca Reviewed-By: Ruben Bridgewater Reviewed-By: Matteo Collina Reviewed-By: James M Snell --- doc/api/https.md | 11 +-- lib/https.js | 24 +++++ .../test-https-agent-additional-options.js | 87 +++++++++++++++++++ test/parallel/test-https-agent-getname.js | 12 ++- .../test-https-agent-secure-protocol.js | 57 ------------ 5 files changed, 127 insertions(+), 64 deletions(-) create mode 100644 test/parallel/test-https-agent-additional-options.js delete mode 100644 test/parallel/test-https-agent-secure-protocol.js diff --git a/doc/api/https.md b/doc/api/https.md index 2cc7502012d..58e62ccced4 100644 --- a/doc/api/https.md +++ b/doc/api/https.md @@ -12,7 +12,7 @@ separate module. added: v0.4.5 --> -An Agent object for HTTPS similar to [`http.Agent`][]. See [`https.request()`][] +An [`Agent`][] object for HTTPS similar to [`http.Agent`][]. See [`https.request()`][] for more information. ## Class: https.Server @@ -168,9 +168,10 @@ changes: Makes a request to a secure web server. -The following additional `options` from [`tls.connect()`][] are also accepted -when using a custom [`Agent`][]: `ca`, `cert`, `ciphers`, `clientCertEngine`, -`key`, `passphrase`, `pfx`, `rejectUnauthorized`, `secureProtocol`, `servername` +The following additional `options` from [`tls.connect()`][] are also accepted: +`ca`, `cert`, `ciphers`, `clientCertEngine`, `crl`, `dhparam`, `ecdhCurve`, +`honorCipherOrder`, `key`, `passphrase`, `pfx`, `rejectUnauthorized`, +`secureOptions`, `secureProtocol`, `servername`, `sessionIdContext` `options` can be an object, a string, or a [`URL`][] object. If `options` is a string, it is automatically parsed with [`url.parse()`][]. If it is a [`URL`][] @@ -220,7 +221,7 @@ const req = https.request(options, (res) => { }); ``` -Alternatively, opt out of connection pooling by not using an `Agent`. +Alternatively, opt out of connection pooling by not using an [`Agent`][]. Example: diff --git a/lib/https.js b/lib/https.js index 741ce84d2f8..84ddeb5036a 100644 --- a/lib/https.js +++ b/lib/https.js @@ -194,6 +194,30 @@ Agent.prototype.getName = function getName(options) { if (options.secureProtocol) name += options.secureProtocol; + name += ':'; + if (options.crl) + name += options.crl; + + name += ':'; + if (options.honorCipherOrder !== undefined) + name += options.honorCipherOrder; + + name += ':'; + if (options.ecdhCurve) + name += options.ecdhCurve; + + name += ':'; + if (options.dhparam) + name += options.dhparam; + + name += ':'; + if (options.secureOptions !== undefined) + name += options.secureOptions; + + name += ':'; + if (options.sessionIdContext) + name += options.sessionIdContext; + return name; }; diff --git a/test/parallel/test-https-agent-additional-options.js b/test/parallel/test-https-agent-additional-options.js new file mode 100644 index 00000000000..8d10524d902 --- /dev/null +++ b/test/parallel/test-https-agent-additional-options.js @@ -0,0 +1,87 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const crypto = require('crypto'); +const https = require('https'); +const fixtures = require('../common/fixtures'); + +const options = { + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), + ca: fixtures.readKey('ca1-cert.pem') +}; + +const server = https.Server(options, function(req, res) { + res.writeHead(200); + res.end('hello world\n'); +}); + +function getBaseOptions(port) { + return { + path: '/', + port: port, + ca: options.ca, + rejectUnautorized: true, + servername: 'agent1', + }; +} + +const updatedValues = new Map([ + ['dhparam', fixtures.readKey('dh2048.pem')], + ['ecdhCurve', 'secp384r1'], + ['honorCipherOrder', true], + ['secureOptions', crypto.constants.SSL_OP_CIPHER_SERVER_PREFERENCE], + ['secureProtocol', 'TLSv1_method'], + ['sessionIdContext', 'sessionIdContext'], +]); + +function variations(iter, port, cb) { + const { done, value } = iter.next(); + if (done) { + return common.mustCall(cb); + } else { + const [key, val] = value; + return common.mustCall(function(res) { + res.resume(); + https.globalAgent.once('free', common.mustCall(function() { + https.get( + Object.assign({}, getBaseOptions(port), { [key]: val }), + variations(iter, port, cb) + ); + })); + }); + } +} + +server.listen(0, common.mustCall(function() { + const port = this.address().port; + const globalAgent = https.globalAgent; + globalAgent.keepAlive = true; + https.get(getBaseOptions(port), variations( + updatedValues.entries(), + port, + common.mustCall(function(res) { + res.resume(); + globalAgent.once('free', common.mustCall(function() { + // Verify that different keep-alived connections are created + // for the base call and each variation + const keys = Object.keys(globalAgent.freeSockets); + assert.strictEqual(keys.length, 1 + updatedValues.size); + let i = 1; + for (const [, value] of updatedValues) { + assert.ok( + keys[i].startsWith(value.toString() + ':') || + keys[i].endsWith(':' + value.toString()) || + keys[i].includes(':' + value.toString() + ':') + ); + i++; + } + globalAgent.destroy(); + server.close(); + })); + }) + )); +})); diff --git a/test/parallel/test-https-agent-getname.js b/test/parallel/test-https-agent-getname.js index 0cdc9568d84..c29e09731df 100644 --- a/test/parallel/test-https-agent-getname.js +++ b/test/parallel/test-https-agent-getname.js @@ -12,7 +12,7 @@ const agent = new https.Agent(); // empty options assert.strictEqual( agent.getName({}), - 'localhost:::::::::::' + 'localhost:::::::::::::::::' ); // pass all options arguments @@ -23,13 +23,21 @@ const options = { ca: 'ca', cert: 'cert', ciphers: 'ciphers', + crl: [Buffer.from('c'), Buffer.from('r'), Buffer.from('l')], + dhparam: 'dhparam', + ecdhCurve: 'ecdhCurve', + honorCipherOrder: false, key: 'key', pfx: 'pfx', rejectUnauthorized: false, + secureOptions: 0, + secureProtocol: 'secureProtocol', servername: 'localhost', + sessionIdContext: 'sessionIdContext' }; assert.strictEqual( agent.getName(options), - '0.0.0.0:443:192.168.1.1:ca:cert::ciphers:key:pfx:false:localhost:' + '0.0.0.0:443:192.168.1.1:ca:cert::ciphers:key:pfx:false:localhost:' + + 'secureProtocol:c,r,l:false:ecdhCurve:dhparam:0:sessionIdContext' ); diff --git a/test/parallel/test-https-agent-secure-protocol.js b/test/parallel/test-https-agent-secure-protocol.js deleted file mode 100644 index 82554952e84..00000000000 --- a/test/parallel/test-https-agent-secure-protocol.js +++ /dev/null @@ -1,57 +0,0 @@ -'use strict'; -const common = require('../common'); -if (!common.hasCrypto) - common.skip('missing crypto'); - -const assert = require('assert'); -const https = require('https'); -const fixtures = require('../common/fixtures'); - -const options = { - key: fixtures.readKey('agent1-key.pem'), - cert: fixtures.readKey('agent1-cert.pem'), - ca: fixtures.readKey('ca1-cert.pem') -}; - -const server = https.Server(options, function(req, res) { - res.writeHead(200); - res.end('hello world\n'); -}); - -server.listen(0, common.mustCall(function() { - const port = this.address().port; - const globalAgent = https.globalAgent; - globalAgent.keepAlive = true; - https.get({ - path: '/', - port: port, - ca: options.ca, - rejectUnauthorized: true, - servername: 'agent1', - secureProtocol: 'SSLv23_method' - }, common.mustCall(function(res) { - res.resume(); - globalAgent.once('free', common.mustCall(function() { - https.get({ - path: '/', - port: port, - ca: options.ca, - rejectUnauthorized: true, - servername: 'agent1', - secureProtocol: 'TLSv1_method' - }, common.mustCall(function(res) { - res.resume(); - globalAgent.once('free', common.mustCall(function() { - // Verify that two keep-alive connections are created - // due to the different secureProtocol settings: - const keys = Object.keys(globalAgent.freeSockets); - assert.strictEqual(keys.length, 2); - assert.ok(keys[0].includes(':SSLv23_method')); - assert.ok(keys[1].includes(':TLSv1_method')); - globalAgent.destroy(); - server.close(); - })); - })); - })); - })); -})); From de848ac1e0483327a2ce8716c3f8567eaeacb660 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 22 Dec 2017 11:19:50 -0600 Subject: [PATCH 09/23] repl: refactor tests to not rely on timing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tests relying on synchronous timing have been migrated to use events. PR-URL: https://github.com/nodejs/node/pull/17828 Reviewed-By: James M Snell Reviewed-By: Michaël Zasso --- lib/internal/repl.js | 4 +- lib/repl.js | 79 +++++------ test/parallel/test-repl-autolibs.js | 27 +++- test/parallel/test-repl-context.js | 67 ++++++---- test/parallel/test-repl-end-emits-exit.js | 18 +-- test/parallel/test-repl-eval-scope.js | 38 +++--- test/parallel/test-repl-eval.js | 50 +++---- ...test-repl-function-definition-edge-case.js | 71 ++++++---- test/parallel/test-repl-load-multiline.js | 4 +- test/parallel/test-repl-mode.js | 53 ++++++-- test/parallel/test-repl-null-thrown.js | 7 +- test/parallel/test-repl-null.js | 21 ++- .../parallel/test-repl-pretty-custom-stack.js | 4 +- test/parallel/test-repl-pretty-stack.js | 4 +- test/parallel/test-repl-recoverable.js | 8 +- .../test-repl-throw-null-or-undefined.js | 20 +-- test/parallel/test-repl-underscore.js | 124 ++++++++++-------- test/parallel/test-repl-use-global.js | 65 ++++----- 18 files changed, 391 insertions(+), 273 deletions(-) diff --git a/lib/internal/repl.js b/lib/internal/repl.js index 812af56ed2e..76412c3b118 100644 --- a/lib/internal/repl.js +++ b/lib/internal/repl.js @@ -8,7 +8,7 @@ const os = require('os'); const util = require('util'); const debug = util.debuglog('repl'); module.exports = Object.create(REPL); -module.exports.createInternalRepl = createRepl; +module.exports.createInternalRepl = createInternalRepl; // XXX(chrisdickinson): The 15ms debounce value is somewhat arbitrary. // The debounce is to guard against code pasted into the REPL. @@ -19,7 +19,7 @@ function _writeToOutput(repl, message) { repl._refreshLine(); } -function createRepl(env, opts, cb) { +function createInternalRepl(env, opts, cb) { if (typeof opts === 'function') { cb = opts; opts = null; diff --git a/lib/repl.js b/lib/repl.js index 84682b1b63e..2227953fa86 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -109,6 +109,11 @@ writer.options = Object.assign({}, exports._builtinLibs = internalModule.builtinLibs; +const sep = '\u0000\u0000\u0000'; +const regExMatcher = new RegExp(`^${sep}(.*)${sep}(.*)${sep}(.*)${sep}(.*)` + + `${sep}(.*)${sep}(.*)${sep}(.*)${sep}(.*)` + + `${sep}(.*)$`); + function REPLServer(prompt, stream, eval_, @@ -149,6 +154,7 @@ function REPLServer(prompt, } var self = this; + replMap.set(self, self); self._domain = dom || domain.create(); self.useGlobal = !!useGlobal; @@ -165,41 +171,6 @@ function REPLServer(prompt, self.rli = this; const savedRegExMatches = ['', '', '', '', '', '', '', '', '', '']; - const sep = '\u0000\u0000\u0000'; - const regExMatcher = new RegExp(`^${sep}(.*)${sep}(.*)${sep}(.*)${sep}(.*)` + - `${sep}(.*)${sep}(.*)${sep}(.*)${sep}(.*)` + - `${sep}(.*)$`); - - eval_ = eval_ || defaultEval; - - // Pause taking in new input, and store the keys in a buffer. - const pausedBuffer = []; - let paused = false; - function pause() { - paused = true; - } - function unpause() { - if (!paused) return; - paused = false; - let entry; - while (entry = pausedBuffer.shift()) { - const [type, payload] = entry; - switch (type) { - case 'key': { - const [d, key] = payload; - self._ttyWrite(d, key); - break; - } - case 'close': - self.emit('exit'); - break; - } - if (paused) { - break; - } - } - } - function defaultEval(code, context, file, cb) { var err, result, script, wrappedErr; var wrappedCmd = false; @@ -331,7 +302,6 @@ function REPLServer(prompt, if (awaitPromise && !err) { let sigintListener; - pause(); let promise = result; if (self.breakEvalOnSigint) { const interrupt = new Promise((resolve, reject) => { @@ -350,7 +320,6 @@ function REPLServer(prompt, prioritizedSigintQueue.delete(sigintListener); finishExecution(undefined, result); - unpause(); }, (err) => { // Remove prioritized SIGINT listener if it was not called. prioritizedSigintQueue.delete(sigintListener); @@ -360,7 +329,6 @@ function REPLServer(prompt, Object.defineProperty(err, 'stack', { value: '' }); } - unpause(); if (err && process.domain) { debug('not recoverable, send to domain'); process.domain.emit('error', err); @@ -377,6 +345,36 @@ function REPLServer(prompt, } } + eval_ = eval_ || defaultEval; + + // Pause taking in new input, and store the keys in a buffer. + const pausedBuffer = []; + let paused = false; + function pause() { + paused = true; + } + function unpause() { + if (!paused) return; + paused = false; + let entry; + while (entry = pausedBuffer.shift()) { + const [type, payload] = entry; + switch (type) { + case 'key': { + const [d, key] = payload; + self._ttyWrite(d, key); + break; + } + case 'close': + self.emit('exit'); + break; + } + if (paused) { + break; + } + } + } + self.eval = self._domain.bind(eval_); self._domain.on('error', function debugDomainError(e) { @@ -405,6 +403,7 @@ function REPLServer(prompt, top.clearBufferedCommand(); top.lines.level = []; top.displayPrompt(); + unpause(); }); if (!input && !output) { @@ -593,6 +592,7 @@ function REPLServer(prompt, const evalCmd = self[kBufferedCommandSymbol] + cmd + '\n'; debug('eval %j', evalCmd); + pause(); self.eval(evalCmd, self.context, 'repl', finish); function finish(e, ret) { @@ -605,6 +605,7 @@ function REPLServer(prompt, '(Press Control-D to exit.)\n'); self.clearBufferedCommand(); self.displayPrompt(); + unpause(); return; } @@ -642,6 +643,7 @@ function REPLServer(prompt, // Display prompt again self.displayPrompt(); + unpause(); } }); @@ -724,7 +726,6 @@ exports.start = function(prompt, ignoreUndefined, replMode); if (!exports.repl) exports.repl = repl; - replMap.set(repl, repl); return repl; }; diff --git a/test/parallel/test-repl-autolibs.js b/test/parallel/test-repl-autolibs.js index 52234deb5e7..68f8402ddbf 100644 --- a/test/parallel/test-repl-autolibs.js +++ b/test/parallel/test-repl-autolibs.js @@ -29,7 +29,22 @@ const repl = require('repl'); common.globalCheck = false; const putIn = new common.ArrayStream(); -repl.start('', putIn, null, true); + + +const replserver = repl.start('', putIn, null, true); +const callbacks = []; +const $eval = replserver.eval; +replserver.eval = function(code, context, file, cb) { + const expected = callbacks.shift(); + return $eval.call(this, code, context, file, (...args) => { + try { + expected(cb, ...args); + } catch (e) { + console.error(e); + process.exit(1); + } + }); +}; test1(); @@ -48,6 +63,11 @@ function test1() { } }; assert(!gotWrite); + callbacks.push(common.mustCall((cb, err, result) => { + assert.ifError(err); + assert.strictEqual(result, require('fs')); + cb(err, result); + })); putIn.run(['fs']); assert(gotWrite); } @@ -66,6 +86,11 @@ function test2() { const val = {}; global.url = val; assert(!gotWrite); + callbacks.push(common.mustCall((cb, err, result) => { + assert.ifError(err); + assert.strictEqual(result, val); + cb(err, result); + })); putIn.run(['url']); assert(gotWrite); } diff --git a/test/parallel/test-repl-context.js b/test/parallel/test-repl-context.js index 9d18067bc2a..90364cee694 100644 --- a/test/parallel/test-repl-context.js +++ b/test/parallel/test-repl-context.js @@ -27,40 +27,57 @@ function testContext(repl) { repl.close(); } -testContextSideEffects(repl.start({ input: stream, output: stream })); +const replserver = repl.start({ input: stream, output: stream }); +const callbacks = []; +const $eval = replserver.eval; +replserver.eval = function(code, context, file, cb) { + const expected = callbacks.shift(); + return $eval.call(this, code, context, file, (...args) => { + try { + expected(cb, ...args); + } catch (e) { + console.error(e); + process.exit(1); + } + }); +}; +testContextSideEffects(replserver); function testContextSideEffects(server) { assert.ok(!server.underscoreAssigned); assert.strictEqual(server.lines.length, 0); // an assignment to '_' in the repl server - server.write('_ = 500;\n'); - assert.ok(server.underscoreAssigned); - assert.strictEqual(server.lines.length, 1); - assert.strictEqual(server.lines[0], '_ = 500;'); - assert.strictEqual(server.last, 500); + callbacks.push(common.mustCall((cb, ...args) => { + assert.ok(server.underscoreAssigned); + assert.strictEqual(server.last, 500); + cb(...args); + assert.strictEqual(server.lines.length, 1); + assert.strictEqual(server.lines[0], '_ = 500;'); - // use the server to create a new context - const context = server.createContext(); + // use the server to create a new context + const context = server.createContext(); - // ensure that creating a new context does not - // have side effects on the server - assert.ok(server.underscoreAssigned); - assert.strictEqual(server.lines.length, 1); - assert.strictEqual(server.lines[0], '_ = 500;'); - assert.strictEqual(server.last, 500); + // ensure that creating a new context does not + // have side effects on the server + assert.ok(server.underscoreAssigned); + assert.strictEqual(server.lines.length, 1); + assert.strictEqual(server.lines[0], '_ = 500;'); + assert.strictEqual(server.last, 500); - // reset the server context - server.resetContext(); - assert.ok(!server.underscoreAssigned); - assert.strictEqual(server.lines.length, 0); + // reset the server context + server.resetContext(); + assert.ok(!server.underscoreAssigned); + assert.strictEqual(server.lines.length, 0); - // ensure that assigning to '_' in the new context - // does not change the value in our server. - assert.ok(!server.underscoreAssigned); - vm.runInContext('_ = 1000;\n', context); + // ensure that assigning to '_' in the new context + // does not change the value in our server. + assert.ok(!server.underscoreAssigned); + vm.runInContext('_ = 1000;\n', context); - assert.ok(!server.underscoreAssigned); - assert.strictEqual(server.lines.length, 0); - server.close(); + assert.ok(!server.underscoreAssigned); + assert.strictEqual(server.lines.length, 0); + server.close(); + })); + server.write('_ = 500;\n'); } diff --git a/test/parallel/test-repl-end-emits-exit.js b/test/parallel/test-repl-end-emits-exit.js index 67f667eeb3d..dd9ad5c1eb0 100644 --- a/test/parallel/test-repl-end-emits-exit.js +++ b/test/parallel/test-repl-end-emits-exit.js @@ -21,10 +21,7 @@ 'use strict'; const common = require('../common'); -const assert = require('assert'); const repl = require('repl'); -let terminalExit = 0; -let regularExit = 0; // Create a dummy stream that does nothing const stream = new common.ArrayStream(); @@ -41,11 +38,10 @@ function testTerminalMode() { stream.emit('data', '\u0004'); }); - r1.on('exit', function() { + r1.on('exit', common.mustCall(function() { // should be fired from the simulated ^D keypress - terminalExit++; testRegularMode(); - }); + })); } function testRegularMode() { @@ -59,17 +55,11 @@ function testRegularMode() { stream.emit('end'); }); - r2.on('exit', function() { + r2.on('exit', common.mustCall(function() { // should be fired from the simulated 'end' event - regularExit++; - }); + })); } -process.on('exit', function() { - assert.strictEqual(terminalExit, 1); - assert.strictEqual(regularExit, 1); -}); - // start testTerminalMode(); diff --git a/test/parallel/test-repl-eval-scope.js b/test/parallel/test-repl-eval-scope.js index 00b577cba73..83311fd92cd 100644 --- a/test/parallel/test-repl-eval-scope.js +++ b/test/parallel/test-repl-eval-scope.js @@ -3,21 +3,27 @@ const common = require('../common'); const assert = require('assert'); const repl = require('repl'); -{ - const stream = new common.ArrayStream(); - const options = { - eval: common.mustCall((cmd, context) => { - assert.strictEqual(cmd, '.scope\n'); - assert.deepStrictEqual(context, { animal: 'Sterrance' }); - }), - input: stream, - output: stream, - terminal: true - }; +const exitTests = []; +process.on('exit', () => { + for (const test of exitTests) test(); +}); +const CONTEXT = { animal: 'Sterrance' }; +const stream = new common.ArrayStream(); +const options = { + eval: common.mustCall((cmd, context) => { + // need to escape the domain + exitTests.push(common.mustCall(() => { + assert.strictEqual(cmd, '.scope'); + assert.ok(context === CONTEXT); + })); + }), + input: stream, + output: stream, + terminal: true +}; - const r = repl.start(options); - r.context = { animal: 'Sterrance' }; +const r = repl.start(options); +r.context = CONTEXT; - stream.emit('data', '\t'); - stream.emit('.exit\n'); -} +stream.emit('data', '\t'); +stream.emit('.exit\n'); diff --git a/test/parallel/test-repl-eval.js b/test/parallel/test-repl-eval.js index d775423fb74..d7290a1583d 100644 --- a/test/parallel/test-repl-eval.js +++ b/test/parallel/test-repl-eval.js @@ -3,31 +3,31 @@ const common = require('../common'); const assert = require('assert'); const repl = require('repl'); -{ - let evalCalledWithExpectedArgs = false; +const exitTests = []; +process.on('exit', () => { + for (const test of exitTests) test(); +}); +const options = { + eval: common.mustCall((cmd, context) => { + // Assertions here will not cause the test to exit with an error code + // so set a boolean that is checked later instead. + exitTests.push(common.mustCall(() => { + assert.strictEqual(cmd, 'function f() {}\n'); + assert.strictEqual(context.foo, 'bar'); + })); + }) +}; - const options = { - eval: common.mustCall((cmd, context) => { - // Assertions here will not cause the test to exit with an error code - // so set a boolean that is checked later instead. - evalCalledWithExpectedArgs = (cmd === 'function f() {}\n' && - context.foo === 'bar'); - }) - }; +const r = repl.start(options); +r.context = { foo: 'bar' }; - const r = repl.start(options); - r.context = { foo: 'bar' }; - - try { - // Default preprocessor transforms - // function f() {} to - // var f = function f() {} - // Test to ensure that original input is preserved. - // Reference: https://github.com/nodejs/node/issues/9743 - r.write('function f() {}\n'); - } finally { - r.write('.exit\n'); - } - - assert(evalCalledWithExpectedArgs); +try { + // Default preprocessor transforms + // function f() {} to + // var f = function f() {} + // Test to ensure that original input is preserved. + // Reference: https://github.com/nodejs/node/issues/9743 + r.write('function f() {}\n'); +} finally { + r.write('.exit\n'); } diff --git a/test/parallel/test-repl-function-definition-edge-case.js b/test/parallel/test-repl-function-definition-edge-case.js index 1e3063e3db5..bda40594a88 100644 --- a/test/parallel/test-repl-function-definition-edge-case.js +++ b/test/parallel/test-repl-function-definition-edge-case.js @@ -7,32 +7,47 @@ const stream = require('stream'); common.globalCheck = false; -const r = initRepl(); - -r.input.emit('data', 'function a() { return 42; } (1)\n'); -r.input.emit('data', 'a\n'); -r.input.emit('data', '.exit'); - -const expected = '1\n[Function: a]\n'; -const got = r.output.accumulator.join(''); -assert.strictEqual(got, expected); - -function initRepl() { - const input = new stream(); - input.write = input.pause = input.resume = () => {}; - input.readable = true; - - const output = new stream(); - output.writable = true; - output.accumulator = []; - - output.write = (data) => output.accumulator.push(data); - - return repl.start({ - input, - output, - useColors: false, - terminal: false, - prompt: '' +const input = new stream(); +input.write = input.pause = input.resume = () => {}; +input.readable = true; + +const output = new stream(); +output.writable = true; +output.accumulator = []; + +output.write = (data) => output.accumulator.push(data); + +const replserver = repl.start({ + input, + output, + useColors: false, + terminal: false, + prompt: '' +}); +const callbacks = []; +const $eval = replserver.eval; +replserver.eval = function(code, context, file, cb) { + const expected = callbacks.shift(); + return $eval.call(this, code, context, file, (...args) => { + try { + expected(...args); + } catch (e) { + console.error(e); + process.exit(1); + } + cb(...args); }); -} +}; + +callbacks.push(common.mustCall((err, result) => { + assert.ifError(err); + assert.strictEqual(result, 1); +})); +replserver.input.emit('data', 'function a() { return 42; } (1)\n'); +callbacks.push(common.mustCall((err, result) => { + assert.ifError(err); + assert.strictEqual(typeof result, 'function'); + assert.strictEqual(result.toString(), 'function a() { return 42; }'); +})); +replserver.input.emit('data', 'a\n'); +replserver.input.emit('data', '.exit'); diff --git a/test/parallel/test-repl-load-multiline.js b/test/parallel/test-repl-load-multiline.js index 8ab878ae768..922aef3d493 100644 --- a/test/parallel/test-repl-load-multiline.js +++ b/test/parallel/test-repl-load-multiline.js @@ -36,5 +36,7 @@ const r = repl.start({ }); r.write(`${command}\n`); -assert.strictEqual(accum.replace(terminalCodeRegex, ''), expected); +r.on('exit', common.mustCall(() => { + assert.strictEqual(accum.replace(terminalCodeRegex, ''), expected); +})); r.close(); diff --git a/test/parallel/test-repl-mode.js b/test/parallel/test-repl-mode.js index 60b430d8c7e..39d7ff88cce 100644 --- a/test/parallel/test-repl-mode.js +++ b/test/parallel/test-repl-mode.js @@ -19,35 +19,49 @@ tests.forEach(function(test) { function testSloppyMode() { const cli = initRepl(repl.REPL_MODE_SLOPPY); + cli.callbacks.push(common.mustCall((err, result) => { + assert.ifError(err); + assert.strictEqual(result, 3); + })); cli.input.emit('data', 'x = 3\n'); - assert.strictEqual(cli.output.accumulator.join(''), '> 3\n> '); - cli.output.accumulator.length = 0; + cli.callbacks.push(common.mustCall((err, result) => { + assert.ifError(err); + assert.strictEqual(result, undefined); + })); cli.input.emit('data', 'let y = 3\n'); - assert.strictEqual(cli.output.accumulator.join(''), 'undefined\n> '); } function testStrictMode() { const cli = initRepl(repl.REPL_MODE_STRICT); + cli._domain.once('error', common.mustCall((err) => { + assert.ok(err); + assert.ok(/ReferenceError: x is not defined/.test(err.message)); + })); cli.input.emit('data', 'x = 3\n'); - assert.ok(/ReferenceError: x is not defined/.test( - cli.output.accumulator.join(''))); - cli.output.accumulator.length = 0; + cli.callbacks.push(common.mustCall((err, result) => { + assert.ifError(err); + assert.strictEqual(result, undefined); + })); cli.input.emit('data', 'let y = 3\n'); - assert.strictEqual(cli.output.accumulator.join(''), 'undefined\n> '); } function testAutoMode() { const cli = initRepl(repl.REPL_MODE_MAGIC); + cli.callbacks.push(common.mustCall((err, result) => { + assert.ifError(err); + assert.strictEqual(result, 3); + })); cli.input.emit('data', 'x = 3\n'); - assert.strictEqual(cli.output.accumulator.join(''), '> 3\n> '); - cli.output.accumulator.length = 0; + cli.callbacks.push(common.mustCall((err, result) => { + assert.ifError(err); + assert.strictEqual(result, undefined); + })); cli.input.emit('data', 'let y = 3\n'); - assert.strictEqual(cli.output.accumulator.join(''), 'undefined\n> '); } function initRepl(mode) { @@ -62,11 +76,28 @@ function initRepl(mode) { output.accumulator = []; output.writable = true; - return repl.start({ + const replserver = repl.start({ input: input, output: output, useColors: false, terminal: false, replMode: mode }); + const callbacks = []; + const $eval = replserver.eval; + replserver.eval = function(code, context, file, cb) { + const expected = callbacks.shift(); + return $eval.call(this, code, context, file, (...args) => { + console.log('EVAL RET', args); + try { + expected(...args); + } catch (e) { + console.error(e); + process.exit(1); + } + cb(...args); + }); + }; + replserver.callbacks = callbacks; + return replserver; } diff --git a/test/parallel/test-repl-null-thrown.js b/test/parallel/test-repl-null-thrown.js index 1fe5d30396d..9b78b0b1dc4 100644 --- a/test/parallel/test-repl-null-thrown.js +++ b/test/parallel/test-repl-null-thrown.js @@ -1,5 +1,5 @@ 'use strict'; -require('../common'); +const common = require('../common'); const repl = require('repl'); const assert = require('assert'); const Stream = require('stream'); @@ -18,7 +18,6 @@ const replserver = repl.start({ replserver.emit('line', 'process.nextTick(() => { throw null; })'); replserver.emit('line', '.exit'); -setTimeout(() => { - console.log(text); +replserver.on('exit', common.mustCall(() => { assert(text.includes('Thrown: null')); -}, 0); +})); diff --git a/test/parallel/test-repl-null.js b/test/parallel/test-repl-null.js index 66d09b28f28..2748cfa780e 100644 --- a/test/parallel/test-repl-null.js +++ b/test/parallel/test-repl-null.js @@ -1,9 +1,28 @@ 'use strict'; -require('../common'); +const common = require('../common'); const repl = require('repl'); const assert = require('assert'); +const callbacks = [ + common.mustCall((err, value) => { + assert.ifError(err); + assert.strictEqual(value, undefined); + }) +]; const replserver = new repl.REPLServer(); +const $eval = replserver.eval; +replserver.eval = function(code, context, file, cb) { + const expected = callbacks.shift(); + return $eval.call(this, code, context, file, (...args) => { + try { + expected(...args); + } catch (e) { + console.error(e); + process.exit(1); + } + cb(...args); + }); +}; replserver._inTemplateLiteral = true; diff --git a/test/parallel/test-repl-pretty-custom-stack.js b/test/parallel/test-repl-pretty-custom-stack.js index be102c1d677..0ca4039a5f8 100644 --- a/test/parallel/test-repl-pretty-custom-stack.js +++ b/test/parallel/test-repl-pretty-custom-stack.js @@ -22,7 +22,9 @@ function run({ command, expected }) { }); r.write(`${command}\n`); - assert.strictEqual(accum, expected); + r.on('exit', common.mustCall(() => { + assert.strictEqual(accum, expected); + })); r.close(); } diff --git a/test/parallel/test-repl-pretty-stack.js b/test/parallel/test-repl-pretty-stack.js index 0fc6b3ada04..55f37e1703e 100644 --- a/test/parallel/test-repl-pretty-stack.js +++ b/test/parallel/test-repl-pretty-stack.js @@ -22,7 +22,9 @@ function run({ command, expected }) { }); r.write(`${command}\n`); - assert.strictEqual(accum, expected); + r.on('exit', common.mustCall(() => { + assert.strictEqual(accum, expected); + })); r.close(); } diff --git a/test/parallel/test-repl-recoverable.js b/test/parallel/test-repl-recoverable.js index 6788d845950..c1767bbc4db 100644 --- a/test/parallel/test-repl-recoverable.js +++ b/test/parallel/test-repl-recoverable.js @@ -4,14 +4,11 @@ const common = require('../common'); const assert = require('assert'); const repl = require('repl'); -let evalCount = 0; let recovered = false; let rendered = false; function customEval(code, context, file, cb) { - evalCount++; - - return cb(evalCount === 1 ? new repl.Recoverable() : null, true); + return cb(!recovered ? new repl.Recoverable() : null, true); } const putIn = new common.ArrayStream(); @@ -26,7 +23,7 @@ putIn.write = function(msg) { } }; -repl.start('', putIn, customEval); +repl.start('', putIn, common.mustCall(customEval, 2)); // https://github.com/nodejs/node/issues/2939 // Expose recoverable errors to the consumer. @@ -36,5 +33,4 @@ putIn.emit('data', '2\n'); process.on('exit', function() { assert(recovered, 'REPL never recovered'); assert(rendered, 'REPL never rendered the result'); - assert.strictEqual(evalCount, 2); }); diff --git a/test/parallel/test-repl-throw-null-or-undefined.js b/test/parallel/test-repl-throw-null-or-undefined.js index fd2fd202b5b..ef25dbe015d 100644 --- a/test/parallel/test-repl-throw-null-or-undefined.js +++ b/test/parallel/test-repl-throw-null-or-undefined.js @@ -1,18 +1,20 @@ 'use strict'; -require('../common'); +const common = require('../common'); // This test ensures that the repl does not // crash or emit error when throwing `null|undefined` // ie `throw null` or `throw undefined` -const assert = require('assert'); const repl = require('repl'); -const r = repl.start(); +const replserver = repl.start(); +const $eval = replserver.eval; +replserver.eval = function(code, context, file, cb) { + return $eval.call(this, code, context, file, + common.mustNotCall( + 'repl crashes/throw error on `throw null|undefined`')); +}; +replserver.write('throw null\n'); +replserver.write('throw undefined\n'); -assert.doesNotThrow(() => { - r.write('throw null\n'); - r.write('throw undefined\n'); -}, TypeError, 'repl crashes/throw error on `throw null|undefined`'); - -r.write('.exit\n'); +replserver.write('.exit\n'); diff --git a/test/parallel/test-repl-underscore.js b/test/parallel/test-repl-underscore.js index 91f32223e18..1b22dcc7221 100644 --- a/test/parallel/test-repl-underscore.js +++ b/test/parallel/test-repl-underscore.js @@ -28,20 +28,22 @@ function testSloppyMode() { _; // remains 30 from user input `); - assertOutput(r.output, [ - 'undefined', - 'undefined', - 'undefined', - '10', - '10', - 'Expression assignment to _ now disabled.', - '20', - '20', - '30', - '30', - '40', - '30' - ]); + r.on('exit', () => { + assertOutput(r.output, [ + 'undefined', + 'undefined', + 'undefined', + '10', + '10', + 'Expression assignment to _ now disabled.', + '20', + '20', + '30', + '30', + '40', + '30' + ]); + }); } function testStrictMode() { @@ -61,20 +63,22 @@ function testStrictMode() { _; // remains 30 from user input `); - assertOutput(r.output, [ - 'undefined', - 'undefined', - 'undefined', - 'undefined', - '20', - '30', - '30', - 'undefined', - '30', - 'undefined', - 'undefined', - '30' - ]); + r.on('exit', () => { + assertOutput(r.output, [ + 'undefined', + 'undefined', + 'undefined', + 'undefined', + '20', + '30', + '30', + 'undefined', + '30', + 'undefined', + 'undefined', + '30' + ]); + }); } function testMagicMode() { @@ -94,20 +98,22 @@ function testMagicMode() { _; // remains 30 from user input `); - assertOutput(r.output, [ - 'undefined', - '10', - '10', - 'undefined', - '20', - '30', - '30', - 'undefined', - '30', - 'undefined', - '50', - '30' - ]); + r.on('exit', () => { + assertOutput(r.output, [ + 'undefined', + '10', + '10', + 'undefined', + '20', + '30', + '30', + 'undefined', + '30', + 'undefined', + '50', + '30' + ]); + }); } function testResetContext() { @@ -121,15 +127,17 @@ function testResetContext() { _; // expect 20 `); - assertOutput(r.output, [ - 'Expression assignment to _ now disabled.', - '10', - '10', - 'Clearing context...', - '10', - '20', - '20' - ]); + r.on('exit', () => { + assertOutput(r.output, [ + 'Expression assignment to _ now disabled.', + '10', + '10', + 'Clearing context...', + '10', + '20', + '20' + ]); + }); } function testResetContextGlobal() { @@ -141,12 +149,14 @@ function testResetContextGlobal() { _; // remains 10 `); - assertOutput(r.output, [ - 'Expression assignment to _ now disabled.', - '10', - '10', - '10', - ]); + r.on('exit', () => { + assertOutput(r.output, [ + 'Expression assignment to _ now disabled.', + '10', + '10', + '10', + ]); + }); // delete globals leaked by REPL when `useGlobal` is `true` delete global.module; diff --git a/test/parallel/test-repl-use-global.js b/test/parallel/test-repl-use-global.js index c76505272b2..75646a9c00a 100644 --- a/test/parallel/test-repl-use-global.js +++ b/test/parallel/test-repl-use-global.js @@ -7,13 +7,6 @@ const stream = require('stream'); const repl = require('internal/repl'); const assert = require('assert'); -// Array of [useGlobal, expectedResult] pairs -const globalTestCases = [ - [false, 'undefined'], - [true, '\'tacos\''], - [undefined, 'undefined'] -]; - const globalTest = (useGlobal, cb, output) => (err, repl) => { if (err) return cb(err); @@ -26,26 +19,12 @@ const globalTest = (useGlobal, cb, output) => (err, repl) => { global.lunch = 'tacos'; repl.write('global.lunch;\n'); repl.close(); - delete global.lunch; - cb(null, str.trim()); -}; - -// Test how the global object behaves in each state for useGlobal -for (const [option, expected] of globalTestCases) { - runRepl(option, globalTest, common.mustCall((err, output) => { - assert.ifError(err); - assert.strictEqual(output, expected); + repl.on('exit', common.mustCall(() => { + delete global.lunch; + cb(null, str.trim()); })); -} +}; -// Test how shadowing the process object via `let` -// behaves in each useGlobal state. Note: we can't -// actually test the state when useGlobal is true, -// because the exception that's generated is caught -// (see below), but errors are printed, and the test -// suite is aware of it, causing a failure to be flagged. -// -const processTestCases = [false, undefined]; const processTest = (useGlobal, cb, output) => (err, repl) => { if (err) return cb(err); @@ -57,15 +36,37 @@ const processTest = (useGlobal, cb, output) => (err, repl) => { repl.write('let process;\n'); repl.write('21 * 2;\n'); repl.close(); - cb(null, str.trim()); -}; - -for (const option of processTestCases) { - runRepl(option, processTest, common.mustCall((err, output) => { + repl.on('exit', common.mustCall((err) => { assert.ifError(err); - assert.strictEqual(output, 'undefined\n42'); + cb(null, str.trim()); })); -} +}; + +// Array of [useGlobal, expectedResult, fn] pairs +const testCases = [ + // Test how the global object behaves in each state for useGlobal + [false, 'undefined', globalTest], + [true, '\'tacos\'', globalTest], + [undefined, 'undefined', globalTest], + // Test how shadowing the process object via `let` + // behaves in each useGlobal state. Note: we can't + // actually test the state when useGlobal is true, + // because the exception that's generated is caught + // (see below), but errors are printed, and the test + // suite is aware of it, causing a failure to be flagged. + [false, 'undefined\n42', processTest] +]; + +const next = common.mustCall(() => { + if (testCases.length) { + const [option, expected, runner] = testCases.shift(); + runRepl(option, runner, common.mustCall((err, output) => { + assert.strictEqual(output, expected); + next(); + })); + } +}, testCases.length + 1); +next(); function runRepl(useGlobal, testFunc, cb) { const inputStream = new stream.PassThrough(); From 180af17b522f531eb15b917f4fde9570b6aa95ae Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 1 Feb 2018 02:28:39 +0100 Subject: [PATCH 10/23] string_decoder: reimplement in C++ Implement string decoder in C++. The perks are a decent speed boost (for decoding, whereas creation show some performance degradation), that this can now be used more easily to add native decoding support to C++ streams and (arguably) more readable variable names. PR-URL: https://github.com/nodejs/node/pull/18537 Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis --- lib/string_decoder.js | 285 ++++------------------- node.gyp | 4 + src/node_internals.h | 1 + src/string_decoder-inl.h | 38 +++ src/string_decoder.cc | 334 +++++++++++++++++++++++++++ src/string_decoder.h | 50 ++++ test/parallel/test-string-decoder.js | 4 + 7 files changed, 478 insertions(+), 238 deletions(-) create mode 100644 src/string_decoder-inl.h create mode 100644 src/string_decoder.cc create mode 100644 src/string_decoder.h diff --git a/lib/string_decoder.js b/lib/string_decoder.js index 1e569ba6b26..d955a663307 100644 --- a/lib/string_decoder.js +++ b/lib/string_decoder.js @@ -22,10 +22,23 @@ 'use strict'; const { Buffer } = require('buffer'); +const { + kIncompleteCharactersStart, + kIncompleteCharactersEnd, + kMissingBytes, + kBufferedBytes, + kEncodingField, + kSize, + decode, + flush, + encodings +} = internalBinding('string_decoder'); const internalUtil = require('internal/util'); const errors = require('internal/errors'); const isEncoding = Buffer[internalUtil.kIsEncodingSymbol]; +const kNativeDecoder = Symbol('kNativeDecoder'); + // Do not cache `Buffer.isEncoding` when checking encoding names as some // modules monkey-patch it to support additional encodings function normalizeEncoding(enc) { @@ -36,258 +49,54 @@ function normalizeEncoding(enc) { return nenc || enc; } +const encodingsMap = {}; +for (var i = 0; i < encodings.length; ++i) + encodingsMap[encodings[i]] = i; + // StringDecoder provides an interface for efficiently splitting a series of // buffers into a series of JS strings without breaking apart multi-byte // characters. -exports.StringDecoder = StringDecoder; -function StringDecoder(encoding) { - this.encoding = normalizeEncoding(encoding); - var nb; - switch (this.encoding) { - case 'utf16le': - this.text = utf16Text; - this.end = utf16End; - nb = 4; - break; - case 'utf8': - this.fillLast = utf8FillLast; - nb = 4; - break; - case 'base64': - this.text = base64Text; - this.end = base64End; - nb = 3; - break; - default: - this.write = simpleWrite; - this.end = simpleEnd; - return; - } - this.lastNeed = 0; - this.lastTotal = 0; - this.lastChar = Buffer.allocUnsafe(nb); -} - -StringDecoder.prototype.write = function(buf) { - if (buf.length === 0) - return ''; - var r; - var i; - if (this.lastNeed) { - r = this.fillLast(buf); - if (r === undefined) - return ''; - i = this.lastNeed; - this.lastNeed = 0; - } else { - i = 0; - } - if (i < buf.length) - return (r ? r + this.text(buf, i) : this.text(buf, i)); - return r || ''; -}; - -StringDecoder.prototype.end = utf8End; - -// Returns only complete characters in a Buffer -StringDecoder.prototype.text = utf8Text; - -// Attempts to complete a partial non-UTF-8 character using bytes from a Buffer -StringDecoder.prototype.fillLast = function(buf) { - if (this.lastNeed <= buf.length) { - buf.copy(this.lastChar, this.lastTotal - this.lastNeed, 0, this.lastNeed); - return this.lastChar.toString(this.encoding, 0, this.lastTotal); - } - buf.copy(this.lastChar, this.lastTotal - this.lastNeed, 0, buf.length); - this.lastNeed -= buf.length; -}; - -// Checks the type of a UTF-8 byte, whether it's ASCII, a leading byte, or a -// continuation byte. If an invalid byte is detected, -2 is returned. -function utf8CheckByte(byte) { - if (byte <= 0x7F) - return 0; - else if (byte >> 5 === 0x06) - return 2; - else if (byte >> 4 === 0x0E) - return 3; - else if (byte >> 3 === 0x1E) - return 4; - return (byte >> 6 === 0x02 ? -1 : -2); -} - -// Checks at most 3 bytes at the end of a Buffer in order to detect an -// incomplete multi-byte UTF-8 character. The total number of bytes (2, 3, or 4) -// needed to complete the UTF-8 character (if applicable) are returned. -function utf8CheckIncomplete(self, buf, i) { - var j = buf.length - 1; - if (j < i) - return 0; - var nb = utf8CheckByte(buf[j]); - if (nb >= 0) { - if (nb > 0) - self.lastNeed = nb - 1; - return nb; - } - if (--j < i || nb === -2) - return 0; - nb = utf8CheckByte(buf[j]); - if (nb >= 0) { - if (nb > 0) - self.lastNeed = nb - 2; - return nb; - } - if (--j < i || nb === -2) - return 0; - nb = utf8CheckByte(buf[j]); - if (nb >= 0) { - if (nb > 0) { - if (nb === 2) - nb = 0; - else - self.lastNeed = nb - 3; - } - return nb; - } - return 0; -} - -// Validates as many continuation bytes for a multi-byte UTF-8 character as -// needed or are available. If we see a non-continuation byte where we expect -// one, we "replace" the validated continuation bytes we've seen so far with -// a single UTF-8 replacement character ('\ufffd'), to match v8's UTF-8 decoding -// behavior. The continuation byte check is included three times in the case -// where all of the continuation bytes for a character exist in the same buffer. -// It is also done this way as a slight performance increase instead of using a -// loop. -function utf8CheckExtraBytes(self, buf, p) { - if ((buf[0] & 0xC0) !== 0x80) { - self.lastNeed = 0; - return '\ufffd'; - } - if (self.lastNeed > 1 && buf.length > 1) { - if ((buf[1] & 0xC0) !== 0x80) { - self.lastNeed = 1; - return '\ufffd'; - } - if (self.lastNeed > 2 && buf.length > 2) { - if ((buf[2] & 0xC0) !== 0x80) { - self.lastNeed = 2; - return '\ufffd'; - } - } +class StringDecoder { + constructor(encoding) { + this.encoding = normalizeEncoding(encoding); + this[kNativeDecoder] = Buffer.alloc(kSize); + this[kNativeDecoder][kEncodingField] = encodingsMap[this.encoding]; } -} -// Attempts to complete a multi-byte UTF-8 character using bytes from a Buffer. -function utf8FillLast(buf) { - const p = this.lastTotal - this.lastNeed; - var r = utf8CheckExtraBytes(this, buf, p); - if (r !== undefined) - return r; - if (this.lastNeed <= buf.length) { - buf.copy(this.lastChar, p, 0, this.lastNeed); - return this.lastChar.toString(this.encoding, 0, this.lastTotal); + write(buf) { + if (typeof buf === 'string') + return buf; + if (!ArrayBuffer.isView(buf)) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'buf', + ['Buffer', 'Uint8Array', 'ArrayBufferView']); + return decode(this[kNativeDecoder], buf); } - buf.copy(this.lastChar, p, 0, buf.length); - this.lastNeed -= buf.length; -} -// Returns all complete UTF-8 characters in a Buffer. If the Buffer ended on a -// partial character, the character's bytes are buffered until the required -// number of bytes are available. -function utf8Text(buf, i) { - const total = utf8CheckIncomplete(this, buf, i); - if (!this.lastNeed) - return buf.toString('utf8', i); - this.lastTotal = total; - const end = buf.length - (total - this.lastNeed); - buf.copy(this.lastChar, 0, end); - return buf.toString('utf8', i, end); -} - -// For UTF-8, a replacement character is added when ending on a partial -// character. -function utf8End(buf) { - const r = (buf && buf.length ? this.write(buf) : ''); - if (this.lastNeed) { - this.lastNeed = 0; - this.lastTotal = 0; - return r + '\ufffd'; + end(buf) { + let ret = ''; + if (buf !== undefined) + ret = this.write(buf); + if (this[kNativeDecoder][kBufferedBytes] > 0) + ret += flush(this[kNativeDecoder]); + return ret; } - return r; -} -// UTF-16LE typically needs two bytes per character, but even if we have an even -// number of bytes available, we need to check if we end on a leading/high -// surrogate. In that case, we need to wait for the next two bytes in order to -// decode the last character properly. -function utf16Text(buf, i) { - if ((buf.length - i) % 2 === 0) { - const r = buf.toString('utf16le', i); - if (r) { - const c = r.charCodeAt(r.length - 1); - if (c >= 0xD800 && c <= 0xDBFF) { - this.lastNeed = 2; - this.lastTotal = 4; - this.lastChar[0] = buf[buf.length - 2]; - this.lastChar[1] = buf[buf.length - 1]; - return r.slice(0, -1); - } - } - return r; - } - this.lastNeed = 1; - this.lastTotal = 2; - this.lastChar[0] = buf[buf.length - 1]; - return buf.toString('utf16le', i, buf.length - 1); -} + /* Everything below this line is undocumented legacy stuff. */ -// For UTF-16LE we do not explicitly append special replacement characters if we -// end on a partial character, we simply let v8 handle that. -function utf16End(buf) { - const r = (buf && buf.length ? this.write(buf) : ''); - if (this.lastNeed) { - const end = this.lastTotal - this.lastNeed; - this.lastNeed = 0; - this.lastTotal = 0; - return r + this.lastChar.toString('utf16le', 0, end); + text(buf, offset) { + this[kNativeDecoder][kMissingBytes] = 0; + this[kNativeDecoder][kBufferedBytes] = 0; + return this.write(buf.slice(offset)); } - return r; -} -function base64Text(buf, i) { - const n = (buf.length - i) % 3; - if (n === 0) - return buf.toString('base64', i); - this.lastNeed = 3 - n; - this.lastTotal = 3; - if (n === 1) { - this.lastChar[0] = buf[buf.length - 1]; - } else { - this.lastChar[0] = buf[buf.length - 2]; - this.lastChar[1] = buf[buf.length - 1]; + get lastTotal() { + return this[kNativeDecoder][kBufferedBytes] + this.lastNeed; } - return buf.toString('base64', i, buf.length - n); -} - -function base64End(buf) { - const r = (buf && buf.length ? this.write(buf) : ''); - if (this.lastNeed) { - const end = 3 - this.lastNeed; - this.lastNeed = 0; - this.lastTotal = 0; - return r + this.lastChar.toString('base64', 0, end); + get lastChar() { + return this[kNativeDecoder].subarray(kIncompleteCharactersStart, + kIncompleteCharactersEnd); } - return r; } -// Pass bytes on through for single-byte encodings (e.g. ascii, latin1, hex) -function simpleWrite(buf) { - return buf.toString(this.encoding); -} - -function simpleEnd(buf) { - return (buf && buf.length ? this.write(buf) : ''); -} +exports.StringDecoder = StringDecoder; diff --git a/node.gyp b/node.gyp index 9c398284939..e2b17cd2b5f 100644 --- a/node.gyp +++ b/node.gyp @@ -326,6 +326,7 @@ 'src/signal_wrap.cc', 'src/spawn_sync.cc', 'src/string_bytes.cc', + 'src/string_decoder.cc', 'src/string_search.cc', 'src/stream_base.cc', 'src/stream_wrap.cc', @@ -379,6 +380,8 @@ 'src/req_wrap.h', 'src/req_wrap-inl.h', 'src/string_bytes.h', + 'src/string_decoder.h', + 'src/string_decoder-inl.h', 'src/stream_base.h', 'src/stream_base-inl.h', 'src/stream_wrap.h', @@ -989,6 +992,7 @@ '<(obj_path)<(obj_separator)node_url.<(obj_suffix)', '<(obj_path)<(obj_separator)util.<(obj_suffix)', '<(obj_path)<(obj_separator)string_bytes.<(obj_suffix)', + '<(obj_path)<(obj_separator)string_decoder.<(obj_suffix)', '<(obj_path)<(obj_separator)string_search.<(obj_suffix)', '<(obj_path)<(obj_separator)stream_base.<(obj_suffix)', '<(obj_path)<(obj_separator)node_constants.<(obj_suffix)', diff --git a/src/node_internals.h b/src/node_internals.h index b3e1f5cd9f2..094fcc2d839 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -120,6 +120,7 @@ struct sockaddr; V(signal_wrap) \ V(spawn_sync) \ V(stream_wrap) \ + V(string_decoder) \ V(tcp_wrap) \ V(timer_wrap) \ V(trace_events) \ diff --git a/src/string_decoder-inl.h b/src/string_decoder-inl.h new file mode 100644 index 00000000000..8a04211906f --- /dev/null +++ b/src/string_decoder-inl.h @@ -0,0 +1,38 @@ +#ifndef SRC_STRING_DECODER_INL_H_ +#define SRC_STRING_DECODER_INL_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "string_decoder.h" +#include "util.h" + +namespace node { + +void StringDecoder::SetEncoding(enum encoding encoding) { + state_[kBufferedBytes] = 0; + state_[kMissingBytes] = 0; + state_[kEncodingField] = encoding; +} + +enum encoding StringDecoder::Encoding() const { + return static_cast(state_[kEncodingField]); +} + +unsigned StringDecoder::BufferedBytes() const { + return state_[kBufferedBytes]; +} + +unsigned StringDecoder::MissingBytes() const { + return state_[kMissingBytes]; +} + +char* StringDecoder::IncompleteCharacterBuffer() { + return reinterpret_cast(state_ + kIncompleteCharactersStart); +} + + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_STRING_DECODER_INL_H_ diff --git a/src/string_decoder.cc b/src/string_decoder.cc new file mode 100644 index 00000000000..ad1bace918c --- /dev/null +++ b/src/string_decoder.cc @@ -0,0 +1,334 @@ +#include "string_decoder-inl.h" +#include "string_bytes.h" +#include "node_internals.h" +#include "node_buffer.h" + +using v8::Array; +using v8::Context; +using v8::FunctionCallbackInfo; +using v8::Integer; +using v8::Isolate; +using v8::Local; +using v8::MaybeLocal; +using v8::Object; +using v8::String; +using v8::Value; + +namespace node { + +namespace { + +MaybeLocal MakeString(Isolate* isolate, + const char* data, + size_t length, + enum encoding encoding) { + Local error; + MaybeLocal ret; + if (encoding == UTF8) { + return String::NewFromUtf8( + isolate, + data, + v8::NewStringType::kNormal, + length); + } else if (encoding == UCS2) { +#ifdef DEBUG + CHECK_EQ(reinterpret_cast(data) % 2, 0); + CHECK_EQ(length % 2, 0); +#endif + ret = StringBytes::Encode( + isolate, + reinterpret_cast(data), + length / 2, + &error); + } else { + ret = StringBytes::Encode( + isolate, + data, + length, + encoding, + &error); + } + + if (ret.IsEmpty()) { + CHECK(!error.IsEmpty()); + isolate->ThrowException(error); + } + +#ifdef DEBUG + CHECK(ret.IsEmpty() || ret.ToLocalChecked()->IsString()); +#endif + return ret.FromMaybe(Local()).As(); +} + +} // anonymous namespace + + +MaybeLocal StringDecoder::DecodeData(Isolate* isolate, + const char* data, + size_t* nread_ptr) { + Local prepend, body; + + size_t nread = *nread_ptr; + + if (Encoding() == UTF8 || Encoding() == UCS2 || Encoding() == BASE64) { + // See if we want bytes to finish a character from the previous + // chunk; if so, copy the new bytes to the missing bytes buffer + // and create a small string from it that is to be prepended to the + // main body. + if (MissingBytes() > 0) { + // There are never more bytes missing than the pre-calculated maximum. + CHECK_LE(MissingBytes() + BufferedBytes(), + kIncompleteCharactersEnd); + if (Encoding() == UTF8) { + // For UTF-8, we need special treatment to align with the V8 decoder: + // If an incomplete character is found at a chunk boundary, we turn + // that character into a single invalid one. + for (size_t i = 0; i < nread && i < MissingBytes(); ++i) { + if ((data[i] & 0xC0) != 0x80) { + // This byte is not a continuation byte even though it should have + // been one. + // Act as if there was a 1-byte incomplete character, which does + // not make sense but works here because we know it's invalid. + state_[kMissingBytes] = 0; + state_[kBufferedBytes] = 1; + data += i; + nread -= i; + break; + } + } + } + + size_t found_bytes = + std::min(nread, static_cast(MissingBytes())); + memcpy(IncompleteCharacterBuffer() + BufferedBytes(), + data, + found_bytes); + // Adjust the two buffers. + data += found_bytes; + nread -= found_bytes; + + state_[kMissingBytes] -= found_bytes; + state_[kBufferedBytes] += found_bytes; + + if (LIKELY(MissingBytes() == 0)) { + // If no more bytes are missing, create a small string that we + // will later prepend. + if (!MakeString(isolate, + IncompleteCharacterBuffer(), + BufferedBytes(), + Encoding()).ToLocal(&prepend)) { + return MaybeLocal(); + } + + *nread_ptr += BufferedBytes(); + // No more buffered bytes. + state_[kBufferedBytes] = 0; + } + } + + // It could be that trying to finish the previous chunk already + // consumed all data that we received in this chunk. + if (UNLIKELY(nread == 0)) { + body = !prepend.IsEmpty() ? prepend : String::Empty(isolate); + prepend = Local(); + } else { +#ifdef DEBUG + // If not, that means is no character left to finish at this point. + CHECK_EQ(MissingBytes(), 0); + CHECK_EQ(BufferedBytes(), 0); +#endif + + // See whether there is a character that we may have to cut off and + // finish when receiving the next chunk. + if (Encoding() == UTF8 && data[nread - 1] & 0x80) { + // This is UTF-8 encoded data and we ended on a non-ASCII UTF-8 byte. + // This means we'll need to figure out where the character to which + // the byte belongs begins. + for (size_t i = nread - 1; ; --i) { +#ifdef DEBUG + CHECK_LT(i, nread); +#endif + state_[kBufferedBytes]++; + if ((data[i] & 0xC0) == 0x80) { + // This byte does not start a character (a "trailing" byte). + if (state_[kBufferedBytes] >= 4 || i == 0) { + // We either have more then 4 trailing bytes (which means + // the current character would not be inside the range for + // valid Unicode, and in particular cannot be represented + // through JavaScript's UTF-16-based approach to strings), or the + // current buffer does not contain the start of an UTF-8 character + // at all. Either way, this is invalid UTF8 and we can just + // let the engine's decoder handle it. + state_[kBufferedBytes] = 0; + break; + } + } else { + // Found the first byte of a UTF-8 character. By looking at the + // upper bits we can tell how long the character *should* be. + if ((data[i] & 0xE0) == 0xC0) { + state_[kMissingBytes] = 2; + } else if ((data[i] & 0xF0) == 0xE0) { + state_[kMissingBytes] = 3; + } else if ((data[i] & 0xF8) == 0xF0) { + state_[kMissingBytes] = 4; + } else { + // This lead byte would indicate a character outside of the + // representable range. + state_[kBufferedBytes] = 0; + break; + } + + if (BufferedBytes() >= MissingBytes()) { + // Received more or exactly as many trailing bytes than the lead + // character would indicate. In the "==" case, we have valid + // data and don't need to slice anything off; + // in the ">" case, this is invalid UTF-8 anyway. + state_[kMissingBytes] = 0; + state_[kBufferedBytes] = 0; + } + + state_[kMissingBytes] -= state_[kBufferedBytes]; + break; + } + } + } else if (Encoding() == UCS2) { + if ((nread % 2) == 1) { + // We got half a codepoint, and need the second byte of it. + state_[kBufferedBytes] = 1; + state_[kMissingBytes] = 1; + } else if ((data[nread - 1] & 0xFC) == 0xD8) { + // Half a split UTF-16 character. + state_[kBufferedBytes] = 2; + state_[kMissingBytes] = 2; + } + } else if (Encoding() == BASE64) { + state_[kBufferedBytes] = nread % 3; + if (state_[kBufferedBytes] > 0) + state_[kMissingBytes] = 3 - BufferedBytes(); + } + + if (BufferedBytes() > 0) { + // Copy the requested number of buffered bytes from the end of the + // input into the incomplete character buffer. + nread -= BufferedBytes(); + *nread_ptr -= BufferedBytes(); + memcpy(IncompleteCharacterBuffer(), data + nread, BufferedBytes()); + } + + if (nread > 0) { + if (!MakeString(isolate, data, nread, Encoding()).ToLocal(&body)) + return MaybeLocal(); + } else { + body = String::Empty(isolate); + } + } + + if (prepend.IsEmpty()) { + return body; + } else { + return String::Concat(prepend, body); + } + } else { + CHECK(Encoding() == ASCII || Encoding() == HEX || Encoding() == LATIN1); + return MakeString(isolate, data, nread, Encoding()); + } +} + +MaybeLocal StringDecoder::FlushData(Isolate* isolate) { + if (Encoding() == ASCII || Encoding() == HEX || Encoding() == LATIN1) { + CHECK_EQ(MissingBytes(), 0); + CHECK_EQ(BufferedBytes(), 0); + } + + if (Encoding() == UCS2 && BufferedBytes() % 2 == 1) { + // Ignore a single trailing byte, like the JS decoder does. + state_[kMissingBytes]--; + state_[kBufferedBytes]--; + } + + if (BufferedBytes() == 0) + return String::Empty(isolate); + + MaybeLocal ret = + MakeString(isolate, + IncompleteCharacterBuffer(), + BufferedBytes(), + Encoding()); + + state_[kMissingBytes] = 0; + state_[kBufferedBytes] = 0; + + return ret; +} + +namespace { + +void DecodeData(const FunctionCallbackInfo& args) { + StringDecoder* decoder = + reinterpret_cast(Buffer::Data(args[0])); + CHECK_NE(decoder, nullptr); + size_t nread = Buffer::Length(args[1]); + MaybeLocal ret = + decoder->DecodeData(args.GetIsolate(), Buffer::Data(args[1]), &nread); + if (!ret.IsEmpty()) + args.GetReturnValue().Set(ret.ToLocalChecked()); +} + +void FlushData(const FunctionCallbackInfo& args) { + StringDecoder* decoder = + reinterpret_cast(Buffer::Data(args[0])); + CHECK_NE(decoder, nullptr); + MaybeLocal ret = decoder->FlushData(args.GetIsolate()); + if (!ret.IsEmpty()) + args.GetReturnValue().Set(ret.ToLocalChecked()); +} + +void InitializeStringDecoder(Local target, + Local unused, + Local context) { + Environment* env = Environment::GetCurrent(context); + Isolate* isolate = env->isolate(); + +#define SET_DECODER_CONSTANT(name) \ + target->Set(context, \ + FIXED_ONE_BYTE_STRING(isolate, #name), \ + Integer::New(isolate, StringDecoder::name)).FromJust() + + SET_DECODER_CONSTANT(kIncompleteCharactersStart); + SET_DECODER_CONSTANT(kIncompleteCharactersEnd); + SET_DECODER_CONSTANT(kMissingBytes); + SET_DECODER_CONSTANT(kBufferedBytes); + SET_DECODER_CONSTANT(kEncodingField); + SET_DECODER_CONSTANT(kNumFields); + + Local encodings = Array::New(isolate); +#define ADD_TO_ENCODINGS_ARRAY(cname, jsname) \ + encodings->Set(context, \ + static_cast(cname), \ + FIXED_ONE_BYTE_STRING(isolate, jsname)).FromJust() + ADD_TO_ENCODINGS_ARRAY(ASCII, "ascii"); + ADD_TO_ENCODINGS_ARRAY(UTF8, "utf8"); + ADD_TO_ENCODINGS_ARRAY(BASE64, "base64"); + ADD_TO_ENCODINGS_ARRAY(UCS2, "utf16le"); + ADD_TO_ENCODINGS_ARRAY(HEX, "hex"); + ADD_TO_ENCODINGS_ARRAY(BUFFER, "buffer"); + ADD_TO_ENCODINGS_ARRAY(LATIN1, "latin1"); + + target->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "encodings"), + encodings).FromJust(); + + target->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "kSize"), + Integer::New(isolate, sizeof(StringDecoder))).FromJust(); + + env->SetMethod(target, "decode", DecodeData); + env->SetMethod(target, "flush", FlushData); +} + +} // anonymous namespace + +} // namespace node + +NODE_MODULE_CONTEXT_AWARE_INTERNAL(string_decoder, + node::InitializeStringDecoder) diff --git a/src/string_decoder.h b/src/string_decoder.h new file mode 100644 index 00000000000..9059eeaa9d2 --- /dev/null +++ b/src/string_decoder.h @@ -0,0 +1,50 @@ +#ifndef SRC_STRING_DECODER_H_ +#define SRC_STRING_DECODER_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "node.h" + +namespace node { + +class StringDecoder { + public: + StringDecoder() { state_[kEncodingField] = BUFFER; } + inline void SetEncoding(enum encoding encoding); + inline enum encoding Encoding() const; + + inline char* IncompleteCharacterBuffer(); + inline unsigned MissingBytes() const; + inline unsigned BufferedBytes() const; + + // Decode a string from the specified encoding. + // The value pointed to by `nread` will be modified to reflect that + // less data may have been read because it ended on an incomplete character + // and more data may have been read because a previously incomplete character + // was finished. + v8::MaybeLocal DecodeData(v8::Isolate* isolate, + const char* data, + size_t* nread); + // Flush an incomplete character. For character encodings like UTF8 this + // means printing replacement characters, buf for e.g. Base64 the returned + // string contains more data. + v8::MaybeLocal FlushData(v8::Isolate* isolate); + + enum Fields { + kIncompleteCharactersStart = 0, + kIncompleteCharactersEnd = 4, + kMissingBytes = 4, + kBufferedBytes = 5, + kEncodingField = 6, + kNumFields = 7 + }; + + private: + uint8_t state_[kNumFields] = {}; +}; + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_STRING_DECODER_H_ diff --git a/test/parallel/test-string-decoder.js b/test/parallel/test-string-decoder.js index 9d1fe69a25d..21a0b6c3e38 100644 --- a/test/parallel/test-string-decoder.js +++ b/test/parallel/test-string-decoder.js @@ -128,6 +128,10 @@ assert.strictEqual(decoder.write(Buffer.from('3DD8', 'hex')), ''); assert.strictEqual(decoder.write(Buffer.from('4D', 'hex')), ''); assert.strictEqual(decoder.end(), '\ud83d'); +decoder = new StringDecoder('utf16le'); +assert.strictEqual(decoder.write(Buffer.from('3DD84D', 'hex')), '\ud83d'); +assert.strictEqual(decoder.end(), ''); + common.expectsError( () => new StringDecoder(1), { From 93bbe4e3ee034d22657e263a2fc6972589e6723f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 4 Feb 2018 19:32:26 +0100 Subject: [PATCH 11/23] deps,src: align ssize_t ABI between Node & nghttp2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, we performed casts that are considered undefined behavior. Instead, just define `ssize_t` for nghttp2 the same way we define it for the rest of Node. Also, remove a TODO comment that would probably also be *technically* correct but shouldn’t matter as long as nobody is complaining. PR-URL: https://github.com/nodejs/node/pull/18565 Reviewed-By: Matteo Collina Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- deps/nghttp2/lib/includes/config.h | 14 ++++++++++++-- src/node.h | 1 - src/node_http2.cc | 13 ++----------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/deps/nghttp2/lib/includes/config.h b/deps/nghttp2/lib/includes/config.h index 0346e0614fd..242bbcfb62f 100644 --- a/deps/nghttp2/lib/includes/config.h +++ b/deps/nghttp2/lib/includes/config.h @@ -1,8 +1,18 @@ /* Hint to the compiler that a function never returns */ #define NGHTTP2_NORETURN -/* Define to `int' if does not define. */ -#define ssize_t int +/* Edited to match src/node.h. */ +#include + +#ifdef _WIN32 +#if !defined(_SSIZE_T_) && !defined(_SSIZE_T_DEFINED) +typedef intptr_t ssize_t; +# define _SSIZE_T_ +# define _SSIZE_T_DEFINED +#endif +#else // !_WIN32 +# include // size_t, ssize_t +#endif // _WIN32 /* Define to 1 if you have the `std::map::emplace`. */ #define HAVE_STD_MAP_EMPLACE 1 diff --git a/src/node.h b/src/node.h index e6f47aa3007..89dbdfc727b 100644 --- a/src/node.h +++ b/src/node.h @@ -184,7 +184,6 @@ NODE_EXTERN v8::Local MakeCallback( #endif #ifdef _WIN32 -// TODO(tjfontaine) consider changing the usage of ssize_t to ptrdiff_t #if !defined(_SSIZE_T_) && !defined(_SSIZE_T_DEFINED) typedef intptr_t ssize_t; # define _SSIZE_T_ diff --git a/src/node_http2.cc b/src/node_http2.cc index bd2e93a13c2..2f688a4b352 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -764,13 +764,7 @@ inline ssize_t Http2Session::Write(const uv_buf_t* bufs, size_t nbufs) { bufs[n].len); CHECK_NE(ret, NGHTTP2_ERR_NOMEM); - // If there is an error calling any of the callbacks, ret will be a - // negative number identifying the error code. This can happen, for - // instance, if the session is destroyed during any of the JS callbacks - // Note: if ssize_t is not defined (e.g. on Win32), nghttp2 will typedef - // ssize_t to int. Cast here so that the < 0 check actually works on - // Windows. - if (static_cast(ret) < 0) + if (ret < 0) return ret; total += ret; @@ -1709,10 +1703,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { statistics_.data_received += nread; ssize_t ret = Write(&stream_buf_, 1); - // Note: if ssize_t is not defined (e.g. on Win32), nghttp2 will typedef - // ssize_t to int. Cast here so that the < 0 check actually works on - // Windows. - if (static_cast(ret) < 0) { + if (ret < 0) { DEBUG_HTTP2SESSION2(this, "fatal error receiving data: %d", ret); Local argv[] = { From 1729af2ce907bd1c97b501fb2dce3d94ccc2174c Mon Sep 17 00:00:00 2001 From: Jack Horton Date: Mon, 5 Feb 2018 11:00:33 -0800 Subject: [PATCH 12/23] test: convert new tests to use error types PR-URL: https://github.com/nodejs/node/pull/18581 Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca --- test/addons-napi/test_typedarray/test.js | 4 ++-- test/parallel/test-console-assign-undefined.js | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/test/addons-napi/test_typedarray/test.js b/test/addons-napi/test_typedarray/test.js index ed37f3c5d4d..41915b380be 100644 --- a/test/addons-napi/test_typedarray/test.js +++ b/test/addons-napi/test_typedarray/test.js @@ -60,7 +60,7 @@ arrayTypes.forEach((currentType) => { const template = Reflect.construct(currentType, buffer); assert.throws(() => { test_typedarray.CreateTypedArray(template, buffer, 0, 136); - }, /Invalid typed array length/); + }, RangeError); }); const nonByteArrayTypes = [ Int16Array, Uint16Array, Int32Array, Uint32Array, @@ -71,5 +71,5 @@ nonByteArrayTypes.forEach((currentType) => { test_typedarray.CreateTypedArray(template, buffer, currentType.BYTES_PER_ELEMENT + 1, 1); console.log(`start of offset ${currentType}`); - }, /start offset of/); + }, RangeError); }); diff --git a/test/parallel/test-console-assign-undefined.js b/test/parallel/test-console-assign-undefined.js index 76007ce41e2..1c47b3bda78 100644 --- a/test/parallel/test-console-assign-undefined.js +++ b/test/parallel/test-console-assign-undefined.js @@ -17,8 +17,7 @@ assert.strictEqual(global.console, 42); common.expectsError( () => console.log('foo'), { - type: TypeError, - message: 'console.log is not a function' + type: TypeError } ); From 28708677d941f51ebdcee2640b8b95f36110ad39 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Wed, 7 Feb 2018 14:51:10 -0500 Subject: [PATCH 13/23] src: resolve issues reported by coverity The specific issues raised by Coverity are: ** CID 182716: Control flow issues (DEADCODE) /src/node_file.cc: 1192 >>> CID 182716: Control flow issues (DEADCODE) >>> Execution cannot reach this statement: "args->GetReturnValue().Set(...". ** CID 182715: Uninitialized members (UNINIT_CTOR) /src/node_file.h: 29 >>> CID 182715: Uninitialized members (UNINIT_CTOR) >>> Non-static class member "syscall_" is not initialized in this constructor nor in any functions that it calls. PR-URL: https://github.com/nodejs/node/pull/18629 Reviewed-By: Ben Noordhuis Reviewed-By: Anna Henningsen Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- src/node_file.cc | 10 +++------- src/node_file.h | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 62dd5fe80cf..7991cdd3504 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1192,13 +1192,9 @@ static void OpenFileHandle(const FunctionCallbackInfo& args) { req_wrap->SetReturnValue(args); } else { SYNC_CALL(open, *path, *path, flags, mode) - if (SYNC_RESULT < 0) { - args.GetReturnValue().Set(SYNC_RESULT); - } else { - HandleScope scope(env->isolate()); - FileHandle* fd = new FileHandle(env, SYNC_RESULT); - args.GetReturnValue().Set(fd->object()); - } + HandleScope scope(env->isolate()); + FileHandle* fd = new FileHandle(env, SYNC_RESULT); + args.GetReturnValue().Set(fd->object()); } } diff --git a/src/node_file.h b/src/node_file.h index d49807f5294..b76caa1467b 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -62,7 +62,7 @@ class FSReqBase : public ReqWrap { private: enum encoding encoding_ = UTF8; - const char* syscall_; + const char* syscall_ = nullptr; const char* data_ = nullptr; MaybeStackBuffer buffer_; From df2f4ad22b27bc4cf26e1c5552d037327850bef4 Mon Sep 17 00:00:00 2001 From: alejandro estrada Date: Thu, 8 Feb 2018 13:01:33 -0500 Subject: [PATCH 14/23] src: replace var for let / const. Replace var for let or const. PR-URL: https://github.com/nodejs/node/pull/18649 Reviewed-By: Ruben Bridgewater Reviewed-By: Julian Duque Reviewed-By: James M Snell --- lib/internal/async_hooks.js | 4 ++-- lib/internal/bootstrap_node.js | 8 ++++---- lib/internal/readline.js | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index 92cd9ba1e42..d1310ad2cac 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -262,7 +262,7 @@ function getOrSetAsyncId(object) { // the user to safeguard this call and make sure it's zero'd out when the // constructor is complete. function getDefaultTriggerAsyncId() { - var defaultTriggerAsyncId = async_id_fields[kDefaultTriggerAsyncId]; + let defaultTriggerAsyncId = async_id_fields[kDefaultTriggerAsyncId]; // If defaultTriggerAsyncId isn't set, use the executionAsyncId if (defaultTriggerAsyncId < 0) defaultTriggerAsyncId = async_id_fields[kExecutionAsyncId]; @@ -276,7 +276,7 @@ function defaultTriggerAsyncIdScope(triggerAsyncId, block, ...args) { const oldDefaultTriggerAsyncId = async_id_fields[kDefaultTriggerAsyncId]; async_id_fields[kDefaultTriggerAsyncId] = triggerAsyncId; - var ret; + let ret; try { ret = Reflect.apply(block, null, args); } finally { diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 33de5ae4be2..0c2109fab36 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -176,7 +176,7 @@ const fs = NativeModule.require('fs'); // read the source const filename = Module._resolveFilename(process.argv[1]); - var source = fs.readFileSync(filename, 'utf-8'); + const source = fs.readFileSync(filename, 'utf-8'); checkScriptSyntax(source, filename); process.exit(0); } @@ -222,7 +222,7 @@ // Read all of stdin - execute it. process.stdin.setEncoding('utf8'); - var code = ''; + let code = ''; process.stdin.on('data', function(d) { code += d; }); @@ -495,7 +495,7 @@ const versionTypes = icu.getVersion().split(','); for (var n = 0; n < versionTypes.length; n++) { - var name = versionTypes[n]; + const name = versionTypes[n]; const version = icu.getVersion(name); Object.defineProperty(process.versions, name, { writable: false, @@ -670,7 +670,7 @@ ]; NativeModule.prototype.compile = function() { - var source = NativeModule.getSource(this.id); + let source = NativeModule.getSource(this.id); source = NativeModule.wrap(source); this.loading = true; diff --git a/lib/internal/readline.js b/lib/internal/readline.js index b15ed4972ef..e3d3007a75c 100644 --- a/lib/internal/readline.js +++ b/lib/internal/readline.js @@ -9,8 +9,8 @@ const ansi = const kEscape = '\x1b'; -var getStringWidth; -var isFullWidthCodePoint; +let getStringWidth; +let isFullWidthCodePoint; function CSI(strings, ...args) { let ret = `${kEscape}[`; From c5c9515c1b8abea2fd98cdd1319176ea1c367764 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 9 Feb 2018 01:22:37 +0800 Subject: [PATCH 15/23] fs: fix stack overflow in fs.readdirSync Previously, fs.readdirSync calls the function returned by env->push_values_to_array_function() in batch and check the returned Maybe right away in C++, which can lead to assertions if the call stack already reaches the maximum size. This patch fixes that by returning early the call fails so the stack overflow error will be properly thrown into JS land. PR-URL: https://github.com/nodejs/node/pull/18647 Fixes: https://github.com/nodejs/node/issues/18645 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- src/node_file.cc | 12 +++++++++--- .../parallel/test-fs-readdir-stack-overflow.js | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-fs-readdir-stack-overflow.js diff --git a/src/node_file.cc b/src/node_file.cc index 7991cdd3504..9f9c7044f91 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1132,14 +1132,20 @@ static void ReadDir(const FunctionCallbackInfo& args) { name_v[name_idx++] = filename.ToLocalChecked(); if (name_idx >= arraysize(name_v)) { - fn->Call(env->context(), names, name_idx, name_v) - .ToLocalChecked(); + MaybeLocal ret = fn->Call(env->context(), names, name_idx, + name_v); + if (ret.IsEmpty()) { + return; + } name_idx = 0; } } if (name_idx > 0) { - fn->Call(env->context(), names, name_idx, name_v).ToLocalChecked(); + MaybeLocal ret = fn->Call(env->context(), names, name_idx, name_v); + if (ret.IsEmpty()) { + return; + } } args.GetReturnValue().Set(names); diff --git a/test/parallel/test-fs-readdir-stack-overflow.js b/test/parallel/test-fs-readdir-stack-overflow.js new file mode 100644 index 00000000000..b7dea52cc37 --- /dev/null +++ b/test/parallel/test-fs-readdir-stack-overflow.js @@ -0,0 +1,18 @@ +'use strict'; + +const common = require('../common'); + +const fs = require('fs'); + +function recurse() { + fs.readdirSync('.'); + recurse(); +} + +common.expectsError( + () => recurse(), + { + type: RangeError, + message: 'Maximum call stack size exceeded' + } +); From cf52ab19dc9632e59b38744ebd0614b4d8ac151b Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Thu, 8 Feb 2018 10:07:57 +0100 Subject: [PATCH 16/23] test: remove unused using declarations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/18637 Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Jon Moss Reviewed-By: Tobias Nießen --- test/cctest/test_environment.cc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 4575d3b65ae..c559a21fda1 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -5,12 +5,6 @@ #include "gtest/gtest.h" #include "node_test_fixture.h" -using node::Environment; -using node::IsolateData; -using node::CreateIsolateData; -using node::FreeIsolateData; -using node::CreateEnvironment; -using node::FreeEnvironment; using node::AtExit; using node::RunAtExit; From 38bac4266a4f7adcfdd9832934aa57c564da1179 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Wed, 7 Feb 2018 16:20:21 +0100 Subject: [PATCH 17/23] crypto: allow passing null as IV unless required PR-URL: https://github.com/nodejs/node/pull/18644 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- doc/api/crypto.md | 16 +++++++-- lib/internal/crypto/cipher.js | 4 +-- src/node_crypto.cc | 34 ++++++++++++++----- .../test-crypto-cipheriv-decipheriv.js | 12 +++++-- 4 files changed, 51 insertions(+), 15 deletions(-) diff --git a/doc/api/crypto.md b/doc/api/crypto.md index 3eb4519d9f9..9adc9082fc2 100644 --- a/doc/api/crypto.md +++ b/doc/api/crypto.md @@ -1286,6 +1286,11 @@ Adversaries][] for details. ### crypto.createCipheriv(algorithm, key, iv[, options]) - `algorithm` {string} - `key` {string | Buffer | TypedArray | DataView} @@ -1301,7 +1306,8 @@ available cipher algorithms. The `key` is the raw key used by the `algorithm` and `iv` is an [initialization vector][]. Both arguments must be `'utf8'` encoded strings, -[Buffers][`Buffer`], `TypedArray`, or `DataView`s. +[Buffers][`Buffer`], `TypedArray`, or `DataView`s. If the cipher does not need +an initialization vector, `iv` may be `null`. ### crypto.createCredentials(details) - `algorithm` {string} - `key` {string | Buffer | TypedArray | DataView} @@ -1363,7 +1374,8 @@ available cipher algorithms. The `key` is the raw key used by the `algorithm` and `iv` is an [initialization vector][]. Both arguments must be `'utf8'` encoded strings, -[Buffers][`Buffer`], `TypedArray`, or `DataView`s. +[Buffers][`Buffer`], `TypedArray`, or `DataView`s. If the cipher does not need +an initialization vector, `iv` may be `null`. ### crypto.createDiffieHellman(prime[, primeEncoding][, generator][, generatorEncoding]) * `urlString` {string} The URL string to parse. From 2bead4ba9e1f92f1cf07fdaaa32197fff7600172 Mon Sep 17 00:00:00 2001 From: Aonghus O Nia Date: Thu, 8 Feb 2018 17:01:23 -0500 Subject: [PATCH 19/23] doc: fix exporting a function example MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Missing the length argument in napi_create_function. PR-URL: https://github.com/nodejs/node/pull/18661 Reviewed-By: Michael Dawson Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Tobias Nießen --- doc/api/n-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 3a954bd0d58..050f8a18925 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -918,7 +918,7 @@ For example, to set a function to be returned by the `require()` for the addon: napi_value Init(napi_env env, napi_value exports) { napi_value method; napi_status status; - status = napi_create_function(env, "exports", Method, NULL, &method); + status = napi_create_function(env, "exports", NAPI_AUTO_LENGTH, Method, NULL, &method); if (status != napi_ok) return NULL; return method; } From d1e80e7cf1aa70fc2a22aea887063f4fa87fc4a9 Mon Sep 17 00:00:00 2001 From: Mihail Bodrov Date: Fri, 9 Feb 2018 02:42:23 +0300 Subject: [PATCH 20/23] buffer: simplify check size in assertSize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/18665 Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: Michaël Zasso Reviewed-By: Joyee Cheung Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell --- lib/buffer.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index 4b800039fe6..edebf901aa8 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -238,9 +238,7 @@ function assertSize(size) { if (typeof size !== 'number') { err = new errors.TypeError('ERR_INVALID_ARG_TYPE', 'size', 'number', size); - } else if (size < 0) { - err = new errors.RangeError('ERR_INVALID_OPT_VALUE', 'size', size); - } else if (size > kMaxLength) { + } else if (size < 0 || size > kMaxLength) { err = new errors.RangeError('ERR_INVALID_OPT_VALUE', 'size', size); } From 4a1a4bfc7edf4ce209b5ed29ce7ff440e1457ed1 Mon Sep 17 00:00:00 2001 From: Yang Guo Date: Fri, 9 Feb 2018 14:23:42 +0100 Subject: [PATCH 21/23] build: no longer have v8-debug.h as dependency. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ref: https://github.com/nodejs/node/issues/18643 PR-URL: https://github.com/nodejs/node/pull/18677 Refs: https://github.com/nodejs/node/issues/18643 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell Reviewed-By: Tobias Nießen --- node.gyp | 1 - 1 file changed, 1 deletion(-) diff --git a/node.gyp b/node.gyp index e2b17cd2b5f..26811945441 100644 --- a/node.gyp +++ b/node.gyp @@ -393,7 +393,6 @@ 'src/util-inl.h', 'deps/http_parser/http_parser.h', 'deps/v8/include/v8.h', - 'deps/v8/include/v8-debug.h', # javascript files to make for an even more pleasant IDE experience '<@(library_files)', # node.gyp is added to the project by default. From 540cbf84afddfbdd2e88ecbb92c28b7dcc582498 Mon Sep 17 00:00:00 2001 From: Evan Lucas Date: Fri, 9 Feb 2018 10:20:20 -0600 Subject: [PATCH 22/23] doc: add error check to fs example MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the err passed to the callback of fs.open() was not checked. PR-URL: https://github.com/nodejs/node/pull/18681 Reviewed-By: Anatoli Papirovski Reviewed-By: Vse Mozhet Byt Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell Reviewed-By: Tobias Nießen --- doc/api/fs.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/api/fs.md b/doc/api/fs.md index a83d4031e7a..2a0ce3f8a22 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -1397,6 +1397,7 @@ fs.open('myfile', 'wx', (err, fd) => { fs.exists('myfile', (exists) => { if (exists) { fs.open('myfile', 'r', (err, fd) => { + if (err) throw err; readMyData(fd); }); } else { From 01d049165c7d3ef9d102c85f0218044cb45f4787 Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Sat, 10 Feb 2018 09:32:49 +0100 Subject: [PATCH 23/23] test: fix flaky repl-timeout-throw Don't disconnect the child until all exceptions are thrown. Fixes: https://github.com/nodejs/node/issues/18659 PR-URL: https://github.com/nodejs/node/pull/18692 Fixes: https://github.com/nodejs/node/issues/18659 Reviewed-By: Ben Noordhuis Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig --- test/sequential/test-repl-timeout-throw.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/sequential/test-repl-timeout-throw.js b/test/sequential/test-repl-timeout-throw.js index aa933394b42..3636b93ddfc 100644 --- a/test/sequential/test-repl-timeout-throw.js +++ b/test/sequential/test-repl-timeout-throw.js @@ -1,5 +1,5 @@ 'use strict'; -const common = require('../common'); +require('../common'); const assert = require('assert'); const spawn = require('child_process').spawn; @@ -13,6 +13,8 @@ child.stdout.setEncoding('utf8'); child.stdout.on('data', function(c) { process.stdout.write(c); stdout += c; + if (stdout.includes('> THROW 2')) + child.stdin.end(); }); child.stdin.write = function(original) { @@ -46,8 +48,6 @@ child.stdout.once('data', function() { ' });\n' + ' });\n' + '});"";\n'); - - setTimeout(child.stdin.end.bind(child.stdin), common.platformTimeout(200)); } });