From 9dd633a2e5b4b363df76f3556b70f92732f941cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Szewczak?= Date: Tue, 25 Apr 2017 20:03:58 +0200 Subject: [PATCH 01/12] test: change URIError constructor to regular expression in assert.throws --- test/parallel/test-querystring.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-querystring.js b/test/parallel/test-querystring.js index 6d8dd2a7f262ef..4715f40d77fb1b 100644 --- a/test/parallel/test-querystring.js +++ b/test/parallel/test-querystring.js @@ -254,7 +254,7 @@ qsWeirdObjects.forEach(function(testCase) { // invalid surrogate pair throws URIError assert.throws(function() { qs.stringify({ foo: '\udc00' }); -}, URIError); +}, /^URIError: URI malformed$/); // coerce numbers to string assert.strictEqual('foo=0', qs.stringify({ foo: 0 })); From b8393d047d495a760d5174d99eddb95f7f7e655d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Szewczak?= Date: Tue, 25 Apr 2017 22:43:37 +0200 Subject: [PATCH 02/12] test: use block-scope for tests that spans multiple statements --- test/parallel/test-querystring.js | 122 +++++++++++++++++------------- 1 file changed, 69 insertions(+), 53 deletions(-) diff --git a/test/parallel/test-querystring.js b/test/parallel/test-querystring.js index 4715f40d77fb1b..82f253fa8b43b6 100644 --- a/test/parallel/test-querystring.js +++ b/test/parallel/test-querystring.js @@ -318,43 +318,47 @@ assert.strictEqual( 0); // Test removing limit -function testUnlimitedKeys() { - const query = {}; +{ + const testUnlimitedKeys = function() { + const query = {}; - for (let i = 0; i < 2000; i++) query[i] = i; + for (let i = 0; i < 2000; i++) query[i] = i; - const url = qs.stringify(query); + const url = qs.stringify(query); - assert.strictEqual( - Object.keys(qs.parse(url, null, null, { maxKeys: 0 })).length, + assert.strictEqual( + Object.keys(qs.parse(url, null, null, {maxKeys: 0})).length, 2000); -} -testUnlimitedKeys(); + }; + testUnlimitedKeys(); +} -const b = qs.unescapeBuffer('%d3%f2Ug%1f6v%24%5e%98%cb' + - '%0d%ac%a2%2f%9d%eb%d8%a2%e6'); +{ + const b = qs.unescapeBuffer('%d3%f2Ug%1f6v%24%5e%98%cb' + + '%0d%ac%a2%2f%9d%eb%d8%a2%e6'); // -assert.strictEqual(0xd3, b[0]); -assert.strictEqual(0xf2, b[1]); -assert.strictEqual(0x55, b[2]); -assert.strictEqual(0x67, b[3]); -assert.strictEqual(0x1f, b[4]); -assert.strictEqual(0x36, b[5]); -assert.strictEqual(0x76, b[6]); -assert.strictEqual(0x24, b[7]); -assert.strictEqual(0x5e, b[8]); -assert.strictEqual(0x98, b[9]); -assert.strictEqual(0xcb, b[10]); -assert.strictEqual(0x0d, b[11]); -assert.strictEqual(0xac, b[12]); -assert.strictEqual(0xa2, b[13]); -assert.strictEqual(0x2f, b[14]); -assert.strictEqual(0x9d, b[15]); -assert.strictEqual(0xeb, b[16]); -assert.strictEqual(0xd8, b[17]); -assert.strictEqual(0xa2, b[18]); -assert.strictEqual(0xe6, b[19]); + assert.strictEqual(0xd3, b[0]); + assert.strictEqual(0xf2, b[1]); + assert.strictEqual(0x55, b[2]); + assert.strictEqual(0x67, b[3]); + assert.strictEqual(0x1f, b[4]); + assert.strictEqual(0x36, b[5]); + assert.strictEqual(0x76, b[6]); + assert.strictEqual(0x24, b[7]); + assert.strictEqual(0x5e, b[8]); + assert.strictEqual(0x98, b[9]); + assert.strictEqual(0xcb, b[10]); + assert.strictEqual(0x0d, b[11]); + assert.strictEqual(0xac, b[12]); + assert.strictEqual(0xa2, b[13]); + assert.strictEqual(0x2f, b[14]); + assert.strictEqual(0x9d, b[15]); + assert.strictEqual(0xeb, b[16]); + assert.strictEqual(0xd8, b[17]); + assert.strictEqual(0xa2, b[18]); + assert.strictEqual(0xe6, b[19]); +} assert.strictEqual(qs.unescapeBuffer('a+b', true).toString(), 'a b'); assert.strictEqual(qs.unescapeBuffer('a+b').toString(), 'a+b'); @@ -368,29 +372,38 @@ assert.strictEqual(qs.unescapeBuffer('a%%').toString(), 'a%%'); check(qs.parse('%\u0100=%\u0101'), { '%Ā': '%ā' }); // Test custom decode -function demoDecode(str) { - return str + str; +{ + const demoDecode = function(str) { + return str + str; + }; + + check(qs.parse('a=a&b=b&c=c', null, null, {decodeURIComponent: demoDecode}), + {aa: 'aa', bb: 'bb', cc: 'cc'}); + check(qs.parse('a=a&b=b&c=c', null, '==', {decodeURIComponent: (str) => str}), + {'a=a': '', 'b=b': '', 'c=c': ''}); } -check(qs.parse('a=a&b=b&c=c', null, null, { decodeURIComponent: demoDecode }), - { aa: 'aa', bb: 'bb', cc: 'cc' }); -check(qs.parse('a=a&b=b&c=c', null, '==', { decodeURIComponent: (str) => str }), - { 'a=a': '', 'b=b': '', 'c=c': '' }); // Test QueryString.unescape -function errDecode(str) { - throw new Error('To jump to the catch scope'); +{ + const errDecode = function(str) { + throw new Error('To jump to the catch scope'); + }; + + check(qs.parse('a=a', null, null, {decodeURIComponent: errDecode}), + {a: 'a'}); } -check(qs.parse('a=a', null, null, { decodeURIComponent: errDecode }), - { a: 'a' }); // Test custom encode -function demoEncode(str) { - return str[0]; +{ + const demoEncode = function(str) { + return str[0]; + }; + + const obj = {aa: 'aa', bb: 'bb', cc: 'cc'}; + assert.strictEqual( + qs.stringify(obj, null, null, {encodeURIComponent: demoEncode}), + 'a=a&b=b&c=c'); } -const obj = { aa: 'aa', bb: 'bb', cc: 'cc' }; -assert.strictEqual( - qs.stringify(obj, null, null, { encodeURIComponent: demoEncode }), - 'a=a&b=b&c=c'); // Test QueryString.unescapeBuffer qsUnescapeTestCases.forEach(function(testCase) { @@ -399,12 +412,15 @@ qsUnescapeTestCases.forEach(function(testCase) { }); // test overriding .unescape -const prevUnescape = qs.unescape; -qs.unescape = function(str) { - return str.replace(/o/g, '_'); -}; -check(qs.parse('foo=bor'), createWithNoPrototype([{key: 'f__', value: 'b_r'}])); -qs.unescape = prevUnescape; - +{ + const prevUnescape = qs.unescape; + qs.unescape = function(str) { + return str.replace(/o/g, '_'); + }; + check( + qs.parse('foo=bor'), + createWithNoPrototype([{key: 'f__', value: 'b_r'}])); + qs.unescape = prevUnescape; +} // test separator and "equals" parsing order check(qs.parse('foo&bar', '&', '&'), { foo: '', bar: '' }); From 494d53fa3cec82e4bca7c79a9f1d4d8368edd5d8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 15 Sep 2017 15:03:48 +0200 Subject: [PATCH 03/12] src: prepare platform for upstream V8 changes V8 platform tasks may schedule other tasks (both background and foreground), and may perform asynchronous operations like resolving Promises. To address that: - Run the task queue drain call inside a callback scope. This makes sure asynchronous operations inside it, like resolving promises, lead to the microtask queue and any subsequent operations not being silently forgotten. - Move the task queue drain call before `EmitBeforeExit()` and only run `EmitBeforeExit()` if there is no new event loop work. - Account for possible new foreground tasks scheduled by background tasks in `DrainBackgroundTasks()`. PR-URL: https://github.com/nodejs/node/pull/15428 Reviewed-By: Franziska Hinkelmann Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis Reviewed-By: Matthew Loring --- src/node.cc | 45 ++++++++++++++++---------------------------- src/node_internals.h | 28 +++++++++++++++++++++++++++ src/node_platform.cc | 30 ++++++++++++++++++++++------- src/node_platform.h | 3 ++- 4 files changed, 69 insertions(+), 37 deletions(-) diff --git a/src/node.cc b/src/node.cc index a234569e21df55..d6678dd967ea71 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1339,30 +1339,6 @@ void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) { env->AddPromiseHook(fn, arg); } -class InternalCallbackScope { - public: - InternalCallbackScope(Environment* env, - Local object, - const async_context& asyncContext); - ~InternalCallbackScope(); - void Close(); - - inline bool Failed() const { return failed_; } - inline void MarkAsFailed() { failed_ = true; } - inline bool IsInnerMakeCallback() const { - return callback_scope_.in_makecallback(); - } - - private: - Environment* env_; - async_context async_context_; - v8::Local object_; - Environment::AsyncCallbackScope callback_scope_; - bool failed_ = false; - bool pushed_ids_ = false; - bool closed_ = false; -}; - CallbackScope::CallbackScope(Isolate* isolate, Local object, async_context asyncContext) @@ -1381,17 +1357,21 @@ CallbackScope::~CallbackScope() { InternalCallbackScope::InternalCallbackScope(Environment* env, Local object, - const async_context& asyncContext) + const async_context& asyncContext, + ResourceExpectation expect) : env_(env), async_context_(asyncContext), object_(object), callback_scope_(env) { - CHECK(!object.IsEmpty()); + if (expect == kRequireResource) { + CHECK(!object.IsEmpty()); + } + HandleScope handle_scope(env->isolate()); // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); - if (env->using_domains()) { + if (env->using_domains() && !object_.IsEmpty()) { DomainEnter(env, object_); } @@ -1413,6 +1393,7 @@ InternalCallbackScope::~InternalCallbackScope() { void InternalCallbackScope::Close() { if (closed_) return; closed_ = true; + HandleScope handle_scope(env_->isolate()); if (pushed_ids_) env_->async_hooks()->pop_ids(async_context_.async_id); @@ -1423,7 +1404,7 @@ void InternalCallbackScope::Close() { AsyncWrap::EmitAfter(env_, async_context_.async_id); } - if (env_->using_domains()) { + if (env_->using_domains() && !object_.IsEmpty()) { DomainExit(env_, object_); } @@ -1463,6 +1444,7 @@ MaybeLocal InternalMakeCallback(Environment* env, int argc, Local argv[], async_context asyncContext) { + CHECK(!recv.IsEmpty()); InternalCallbackScope scope(env, recv, asyncContext); if (scope.Failed()) { return Undefined(env->isolate()); @@ -4726,9 +4708,14 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, do { uv_run(env.event_loop(), UV_RUN_DEFAULT); + v8_platform.DrainVMTasks(); + + more = uv_loop_alive(env.event_loop()); + if (more) + continue; + EmitBeforeExit(&env); - v8_platform.DrainVMTasks(); // Emit `beforeExit` if the loop became alive either after emitting // event, or after running some callbacks. more = uv_loop_alive(env.event_loop()); diff --git a/src/node_internals.h b/src/node_internals.h index 9371d442ad0ea5..fd8cc26a2881ca 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -294,8 +294,36 @@ v8::MaybeLocal InternalMakeCallback( v8::Local argv[], async_context asyncContext); +class InternalCallbackScope { + public: + // Tell the constructor whether its `object` parameter may be empty or not. + enum ResourceExpectation { kRequireResource, kAllowEmptyResource }; + InternalCallbackScope(Environment* env, + v8::Local object, + const async_context& asyncContext, + ResourceExpectation expect = kRequireResource); + ~InternalCallbackScope(); + void Close(); + + inline bool Failed() const { return failed_; } + inline void MarkAsFailed() { failed_ = true; } + inline bool IsInnerMakeCallback() const { + return callback_scope_.in_makecallback(); + } + + private: + Environment* env_; + async_context async_context_; + v8::Local object_; + Environment::AsyncCallbackScope callback_scope_; + bool failed_ = false; + bool pushed_ids_ = false; + bool closed_ = false; +}; + } // namespace node + #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #endif // SRC_NODE_INTERNALS_H_ diff --git a/src/node_platform.cc b/src/node_platform.cc index 3d023114ad2691..320136c3f72159 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -1,10 +1,14 @@ #include "node_platform.h" +#include "node_internals.h" #include "util.h" namespace node { +using v8::HandleScope; using v8::Isolate; +using v8::Local; +using v8::Object; using v8::Platform; using v8::Task; using v8::TracingController; @@ -63,22 +67,33 @@ size_t NodePlatform::NumberOfAvailableBackgroundThreads() { return threads_.size(); } -static void RunForegroundTask(uv_timer_t* handle) { - Task* task = static_cast(handle->data); +static void RunForegroundTask(Task* task) { + Isolate* isolate = Isolate::GetCurrent(); + HandleScope scope(isolate); + Environment* env = Environment::GetCurrent(isolate); + InternalCallbackScope cb_scope(env, Local(), { 0, 0 }, + InternalCallbackScope::kAllowEmptyResource); task->Run(); delete task; +} + +static void RunForegroundTask(uv_timer_t* handle) { + Task* task = static_cast(handle->data); + RunForegroundTask(task); uv_close(reinterpret_cast(handle), [](uv_handle_t* handle) { delete reinterpret_cast(handle); }); } void NodePlatform::DrainBackgroundTasks() { - FlushForegroundTasksInternal(); - background_tasks_.BlockingDrain(); + while (FlushForegroundTasksInternal()) + background_tasks_.BlockingDrain(); } -void NodePlatform::FlushForegroundTasksInternal() { +bool NodePlatform::FlushForegroundTasksInternal() { + bool did_work = false; while (auto delayed = foreground_delayed_tasks_.Pop()) { + did_work = true; uint64_t delay_millis = static_cast(delayed->second + 0.5) * 1000; uv_timer_t* handle = new uv_timer_t(); @@ -91,9 +106,10 @@ void NodePlatform::FlushForegroundTasksInternal() { delete delayed; } while (Task* task = foreground_tasks_.Pop()) { - task->Run(); - delete task; + did_work = true; + RunForegroundTask(task); } + return did_work; } void NodePlatform::CallOnBackgroundThread(Task* task, diff --git a/src/node_platform.h b/src/node_platform.h index 668fcf28e40233..04927fccc3df66 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -39,7 +39,8 @@ class NodePlatform : public v8::Platform { virtual ~NodePlatform() {} void DrainBackgroundTasks(); - void FlushForegroundTasksInternal(); + // Returns true iff work was dispatched or executed. + bool FlushForegroundTasksInternal(); void Shutdown(); // v8::Platform implementation. From 90f7d9f14d8ee26f93189638c22faef6a83c7fe9 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 14 Sep 2017 13:46:02 +0200 Subject: [PATCH 04/12] fs: refactor close to use destroy Refactor close() to use destroy() and not vice versa in ReadStream. Avoid races between WriteStream.close and WriteStream.write, by aliasing close to end(). Fixes: https://github.com/nodejs/node/issues/2006 PR-URL: https://github.com/nodejs/node/pull/15407 Reviewed-By: James M Snell Reviewed-By: Refael Ackermann Reviewed-By: Colin Ihrig --- lib/fs.js | 60 +++++++++++-------- .../test-fs-read-stream-double-close.js | 16 ++++- 2 files changed, 47 insertions(+), 29 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index fb1001c8c09d6f..24af0e58a3dc99 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2090,44 +2090,38 @@ ReadStream.prototype._read = function(n) { } }; - ReadStream.prototype._destroy = function(err, cb) { - this.close(function(err2) { - cb(err || err2); - }); -}; - - -ReadStream.prototype.close = function(cb) { - if (cb) - this.once('close', cb); - if (this.closed || typeof this.fd !== 'number') { if (typeof this.fd !== 'number') { - this.once('open', closeOnOpen); + this.once('open', closeFsStream.bind(null, this, cb, err)); return; } - return process.nextTick(() => this.emit('close')); + + return process.nextTick(() => { + cb(err); + this.emit('close'); + }); } this.closed = true; - fs.close(this.fd, (er) => { - if (er) - this.emit('error', er); - else - this.emit('close'); - }); - + closeFsStream(this, cb); this.fd = null; }; -// needed because as it will be called with arguments -// that does not match this.close() signature -function closeOnOpen(fd) { - this.close(); +function closeFsStream(stream, cb, err) { + fs.close(stream.fd, (er) => { + er = er || err; + cb(er); + if (!er) + stream.emit('close'); + }); } +ReadStream.prototype.close = function(cb) { + this.destroy(null, cb); +}; + fs.createWriteStream = function(path, options) { return new WriteStream(path, options); }; @@ -2179,7 +2173,7 @@ function WriteStream(path, options) { // dispose on finish. this.once('finish', function() { if (this.autoClose) { - this.close(); + this.destroy(); } }); } @@ -2276,7 +2270,21 @@ WriteStream.prototype._writev = function(data, cb) { WriteStream.prototype._destroy = ReadStream.prototype._destroy; -WriteStream.prototype.close = ReadStream.prototype.close; +WriteStream.prototype.close = function(cb) { + if (this._writableState.ending) { + this.on('close', cb); + return; + } + + if (this._writableState.ended) { + process.nextTick(cb); + return; + } + + // we use end() instead of destroy() because of + // https://github.com/nodejs/node/issues/2006 + this.end(cb); +}; // There is no shutdown() for files. WriteStream.prototype.destroySoon = WriteStream.prototype.end; diff --git a/test/parallel/test-fs-read-stream-double-close.js b/test/parallel/test-fs-read-stream-double-close.js index ca337d4f8a576b..5f36c03bdbf243 100644 --- a/test/parallel/test-fs-read-stream-double-close.js +++ b/test/parallel/test-fs-read-stream-double-close.js @@ -3,7 +3,17 @@ const common = require('../common'); const fs = require('fs'); -const s = fs.createReadStream(__filename); +{ + const s = fs.createReadStream(__filename); -s.close(common.mustCall()); -s.close(common.mustCall()); + s.close(common.mustCall()); + s.close(common.mustCall()); +} + +{ + const s = fs.createReadStream(__filename); + + // this is a private API, but it is worth esting. close calls this + s.destroy(null, common.mustCall()); + s.destroy(null, common.mustCall()); +} From 650f09d8680faad0f6481c99df22d059a0192704 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Tue, 19 Sep 2017 11:22:35 -0400 Subject: [PATCH 05/12] http2: small fixes to compatibility layer Expand argument validation through compat API, adjust behaviour of response.end to not throw if stream already closed to match http1, adjust behaviour of writeContinue to not throw if stream already closed and other very small tweaks. Add tests for added and fixed behaviour. Add tests for edge case behaviours of setTimeout, createPushResponse, destroy, end and trailers. PR-URL: https://github.com/nodejs/node/pull/15473 Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Minwoo Jung --- lib/internal/http2/compat.js | 38 ++++----- ...test-http2-compat-expect-continue-check.js | 5 ++ ...t-http2-compat-serverrequest-settimeout.js | 15 +++- .../test-http2-compat-serverrequest.js | 5 +- ...ompat-serverresponse-createpushresponse.js | 13 +++ ...est-http2-compat-serverresponse-destroy.js | 3 + .../test-http2-compat-serverresponse-end.js | 80 +++++++++++++++++-- ...est-http2-compat-serverresponse-headers.js | 15 +++- ...-http2-compat-serverresponse-settimeout.js | 15 +++- ...st-http2-compat-serverresponse-trailers.js | 44 ++++++++++ 10 files changed, 200 insertions(+), 33 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 15e7358627dd8d..2523fb0ba372ea 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -92,7 +92,7 @@ function onStreamError(error) { // errors in compatibility mode are // not forwarded to the request // and response objects. However, - // they are forwarded to 'clientError' + // they are forwarded to 'streamError' // on the server by Http2Stream } @@ -248,9 +248,9 @@ class Http2ServerRequest extends Readable { } setTimeout(msecs, callback) { - const stream = this[kStream]; - if (stream === undefined) return; - stream.setTimeout(msecs, callback); + if (this[kState].closed) + return; + this[kStream].setTimeout(msecs, callback); } [kFinish](code) { @@ -445,7 +445,7 @@ class Http2ServerResponse extends Stream { if (stream === undefined) { const err = new errors.Error('ERR_HTTP2_STREAM_CLOSED'); - if (cb) + if (typeof cb === 'function') process.nextTick(cb, err); else throw err; @@ -461,12 +461,11 @@ class Http2ServerResponse extends Stream { if (typeof chunk === 'function') { cb = chunk; chunk = null; - encoding = 'utf8'; } else if (typeof encoding === 'function') { cb = encoding; encoding = 'utf8'; } - if (stream === undefined || stream.finished === true) { + if (this.finished === true) { return false; } if (chunk !== null && chunk !== undefined) { @@ -482,21 +481,21 @@ class Http2ServerResponse extends Stream { } destroy(err) { - const stream = this[kStream]; - if (stream === undefined) { - // nothing to do, already closed + if (this[kState].closed) return; - } - stream.destroy(err); + this[kStream].destroy(err); } setTimeout(msecs, callback) { const stream = this[kStream]; - if (stream === undefined) return; + if (this[kState].closed) + return; stream.setTimeout(msecs, callback); } createPushResponse(headers, callback) { + if (typeof callback !== 'function') + throw new errors.TypeError('ERR_INVALID_CALLBACK'); const stream = this[kStream]; if (stream === undefined) { process.nextTick(callback, new errors.Error('ERR_HTTP2_STREAM_CLOSED')); @@ -513,12 +512,9 @@ class Http2ServerResponse extends Stream { if (stream !== undefined && stream.destroyed === false && stream.headersSent === false) { - options = options || Object.create(null); - const state = this[kState]; const headers = this[kHeaders]; - headers[HTTP2_HEADER_STATUS] = state.statusCode; - if (stream.finished === true) - options.endStream = true; + headers[HTTP2_HEADER_STATUS] = this[kState].statusCode; + options = options || Object.create(null); options.getTrailers = (trailers) => { Object.assign(trailers, this[kTrailers]); }; @@ -542,7 +538,11 @@ class Http2ServerResponse extends Stream { // TODO doesn't support callbacks writeContinue() { const stream = this[kStream]; - if (stream === undefined) return false; + if (stream === undefined || + stream.headersSent === true || + stream.destroyed === true) { + return false; + } this[kStream].additionalHeaders({ [HTTP2_HEADER_STATUS]: HTTP_STATUS_CONTINUE }); diff --git a/test/parallel/test-http2-compat-expect-continue-check.js b/test/parallel/test-http2-compat-expect-continue-check.js index a956697b5cb5a7..14e78155204770 100644 --- a/test/parallel/test-http2-compat-expect-continue-check.js +++ b/test/parallel/test-http2-compat-expect-continue-check.js @@ -21,6 +21,11 @@ function handler(req, res) { 'abcd': '1' }); res.end(testResBody); + // should simply return false if already too late to write + assert.strictEqual(res.writeContinue(), false); + res.on('finish', common.mustCall( + () => process.nextTick(() => assert.strictEqual(res.writeContinue(), false)) + )); } const server = http2.createServer( diff --git a/test/parallel/test-http2-compat-serverrequest-settimeout.js b/test/parallel/test-http2-compat-serverrequest-settimeout.js index 6e02fe0cffb2ab..7cdae697cc0b63 100644 --- a/test/parallel/test-http2-compat-serverrequest-settimeout.js +++ b/test/parallel/test-http2-compat-serverrequest-settimeout.js @@ -4,13 +4,25 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); +const assert = require('assert'); const http2 = require('http2'); +const msecs = common.platformTimeout(1); const server = http2.createServer(); server.on('request', (req, res) => { - req.setTimeout(common.platformTimeout(1), common.mustCall(() => { + req.setTimeout(msecs, common.mustCall(() => { res.end(); + req.setTimeout(msecs, common.mustNotCall()); + })); + res.on('finish', common.mustCall(() => { + req.setTimeout(msecs, common.mustNotCall()); + process.nextTick(() => { + assert.doesNotThrow( + () => req.setTimeout(msecs, common.mustNotCall()) + ); + server.close(); + }); })); }); @@ -24,7 +36,6 @@ server.listen(0, common.mustCall(() => { ':authority': `localhost:${port}` }); req.on('end', common.mustCall(() => { - server.close(); client.destroy(); })); req.resume(); diff --git a/test/parallel/test-http2-compat-serverrequest.js b/test/parallel/test-http2-compat-serverrequest.js index a6882c6a9bf1c4..8c72e3876527c2 100644 --- a/test/parallel/test-http2-compat-serverrequest.js +++ b/test/parallel/test-http2-compat-serverrequest.js @@ -34,7 +34,10 @@ server.listen(0, common.mustCall(function() { response.on('finish', common.mustCall(function() { assert.strictEqual(request.closed, true); assert.strictEqual(request.code, h2.constants.NGHTTP2_NO_ERROR); - server.close(); + process.nextTick(() => { + assert.strictEqual(request.socket, undefined); + server.close(); + }); })); response.end(); })); diff --git a/test/parallel/test-http2-compat-serverresponse-createpushresponse.js b/test/parallel/test-http2-compat-serverresponse-createpushresponse.js index 8c5ff31274322e..ecc8fea74ce637 100644 --- a/test/parallel/test-http2-compat-serverresponse-createpushresponse.js +++ b/test/parallel/test-http2-compat-serverresponse-createpushresponse.js @@ -16,6 +16,19 @@ const server = h2.createServer((request, response) => { assert.strictEqual(response.stream.id % 2, 1); response.write(servExpect); + // callback must be specified (and be a function) + common.expectsError( + () => response.createPushResponse({ + ':path': '/pushed', + ':method': 'GET' + }, undefined), + { + code: 'ERR_INVALID_CALLBACK', + type: TypeError, + message: 'Callback must be a function' + } + ); + response.createPushResponse({ ':path': '/pushed', ':method': 'GET' diff --git a/test/parallel/test-http2-compat-serverresponse-destroy.js b/test/parallel/test-http2-compat-serverresponse-destroy.js index f2b3ae7cfefa49..20329e0d8fdfb4 100644 --- a/test/parallel/test-http2-compat-serverresponse-destroy.js +++ b/test/parallel/test-http2-compat-serverresponse-destroy.js @@ -24,6 +24,9 @@ const server = http2.createServer(common.mustCall((req, res) => { res.on('finish', common.mustCall(() => { assert.doesNotThrow(() => res.destroy(nextError)); assert.strictEqual(res.closed, true); + process.nextTick(() => { + assert.doesNotThrow(() => res.destroy(nextError)); + }); })); if (req.url !== '/') { diff --git a/test/parallel/test-http2-compat-serverresponse-end.js b/test/parallel/test-http2-compat-serverresponse-end.js index 92a0fab4e0a49d..957983b0aedb6d 100644 --- a/test/parallel/test-http2-compat-serverresponse-end.js +++ b/test/parallel/test-http2-compat-serverresponse-end.js @@ -4,7 +4,7 @@ const { mustCall, mustNotCall, hasCrypto, skip } = require('../common'); if (!hasCrypto) skip('missing crypto'); -const { doesNotThrow, strictEqual } = require('assert'); +const { strictEqual } = require('assert'); const { createServer, connect, @@ -15,18 +15,82 @@ const { } = require('http2'); { - // Http2ServerResponse.end callback is called only the first time, - // but may be invoked repeatedly without throwing errors. + // Http2ServerResponse.end accepts chunk, encoding, cb as args + // It may be invoked repeatedly without throwing errors + // but callback will only be called once const server = createServer(mustCall((request, response) => { strictEqual(response.closed, false); - response.on('finish', mustCall(() => process.nextTick( - mustCall(() => doesNotThrow(() => response.end('test', mustNotCall()))) - ))); + response.end('end', 'utf8', mustCall(() => { + strictEqual(response.closed, true); + response.end(mustNotCall()); + process.nextTick(() => { + response.end(mustNotCall()); + server.close(); + }); + })); + response.end(mustNotCall()); + })); + server.listen(0, mustCall(() => { + let data = ''; + const { port } = server.address(); + const url = `http://localhost:${port}`; + const client = connect(url, mustCall(() => { + const headers = { + ':path': '/', + ':method': 'GET', + ':scheme': 'http', + ':authority': `localhost:${port}` + }; + const request = client.request(headers); + request.setEncoding('utf8'); + request.on('data', (chunk) => (data += chunk)); + request.on('end', mustCall(() => { + strictEqual(data, 'end'); + client.destroy(); + })); + request.end(); + request.resume(); + })); + })); +} + +{ + // Http2ServerResponse.end can omit encoding arg, sets it to utf-8 + const server = createServer(mustCall((request, response) => { + response.end('test\uD83D\uDE00', mustCall(() => { + server.close(); + })); + })); + server.listen(0, mustCall(() => { + let data = ''; + const { port } = server.address(); + const url = `http://localhost:${port}`; + const client = connect(url, mustCall(() => { + const headers = { + ':path': '/', + ':method': 'GET', + ':scheme': 'http', + ':authority': `localhost:${port}` + }; + const request = client.request(headers); + request.setEncoding('utf8'); + request.on('data', (chunk) => (data += chunk)); + request.on('end', mustCall(() => { + strictEqual(data, 'test\uD83D\uDE00'); + client.destroy(); + })); + request.end(); + request.resume(); + })); + })); +} + +{ + // Http2ServerResponse.end can omit chunk & encoding args + const server = createServer(mustCall((request, response) => { response.end(mustCall(() => { server.close(); })); - response.end(mustNotCall()); - strictEqual(response.closed, true); })); server.listen(0, mustCall(() => { const { port } = server.address(); diff --git a/test/parallel/test-http2-compat-serverresponse-headers.js b/test/parallel/test-http2-compat-serverresponse-headers.js index 2cc82d4dd3c82f..d753b3e6cc4230 100644 --- a/test/parallel/test-http2-compat-serverresponse-headers.js +++ b/test/parallel/test-http2-compat-serverresponse-headers.js @@ -103,6 +103,14 @@ server.listen(0, common.mustCall(function() { message: 'The "name" argument must be of type string' } ); + common.expectsError( + () => response.setHeader(''), + { + code: 'ERR_INVALID_HTTP_TOKEN', + type: TypeError, + message: 'Header name must be a valid HTTP token [""]' + } + ); response.setHeader(real, expectedValue); const expectedHeaderNames = [real]; @@ -122,7 +130,12 @@ server.listen(0, common.mustCall(function() { response.on('finish', common.mustCall(function() { assert.strictEqual(response.code, h2.constants.NGHTTP2_NO_ERROR); assert.strictEqual(response.headersSent, true); - server.close(); + process.nextTick(() => { + // can access headersSent after stream is undefined + assert.strictEqual(response.stream, undefined); + assert.strictEqual(response.headersSent, true); + server.close(); + }); })); response.end(); })); diff --git a/test/parallel/test-http2-compat-serverresponse-settimeout.js b/test/parallel/test-http2-compat-serverresponse-settimeout.js index 66441d390ae938..6b5bf03d522045 100644 --- a/test/parallel/test-http2-compat-serverresponse-settimeout.js +++ b/test/parallel/test-http2-compat-serverresponse-settimeout.js @@ -4,13 +4,25 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); +const assert = require('assert'); const http2 = require('http2'); +const msecs = common.platformTimeout(1); const server = http2.createServer(); server.on('request', (req, res) => { - res.setTimeout(common.platformTimeout(1), common.mustCall(() => { + res.setTimeout(msecs, common.mustCall(() => { res.end(); + res.setTimeout(msecs, common.mustNotCall()); + })); + res.on('finish', common.mustCall(() => { + res.setTimeout(msecs, common.mustNotCall()); + process.nextTick(() => { + assert.doesNotThrow( + () => res.setTimeout(msecs, common.mustNotCall()) + ); + server.close(); + }); })); }); @@ -24,7 +36,6 @@ server.listen(0, common.mustCall(() => { ':authority': `localhost:${port}` }); req.on('end', common.mustCall(() => { - server.close(); client.destroy(); })); req.resume(); diff --git a/test/parallel/test-http2-compat-serverresponse-trailers.js b/test/parallel/test-http2-compat-serverresponse-trailers.js index 17c2d734425137..3f3aa2045b4929 100644 --- a/test/parallel/test-http2-compat-serverresponse-trailers.js +++ b/test/parallel/test-http2-compat-serverresponse-trailers.js @@ -14,6 +14,49 @@ server.listen(0, common.mustCall(() => { response.addTrailers({ ABC: 123 }); + response.setTrailer('ABCD', 123); + + common.expectsError( + () => response.addTrailers({ '': 'test' }), + { + code: 'ERR_INVALID_HTTP_TOKEN', + type: TypeError, + message: 'Header name must be a valid HTTP token [""]' + } + ); + common.expectsError( + () => response.setTrailer('test', undefined), + { + code: 'ERR_HTTP2_INVALID_HEADER_VALUE', + type: TypeError, + message: 'Value must not be undefined or null' + } + ); + common.expectsError( + () => response.setTrailer('test', null), + { + code: 'ERR_HTTP2_INVALID_HEADER_VALUE', + type: TypeError, + message: 'Value must not be undefined or null' + } + ); + common.expectsError( + () => response.setTrailer(), // trailer name undefined + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "name" argument must be of type string' + } + ); + common.expectsError( + () => response.setTrailer(''), + { + code: 'ERR_INVALID_HTTP_TOKEN', + type: TypeError, + message: 'Header name must be a valid HTTP token [""]' + } + ); + response.end('hello'); })); @@ -22,6 +65,7 @@ server.listen(0, common.mustCall(() => { const request = client.request(); request.on('trailers', common.mustCall((headers) => { assert.strictEqual(headers.abc, '123'); + assert.strictEqual(headers.abcd, '123'); })); request.resume(); request.on('end', common.mustCall(() => { From 11d72f275048b9d3a45e7cfcae2324ff09f07bc9 Mon Sep 17 00:00:00 2001 From: Vse Mozhet Byt Date: Thu, 21 Sep 2017 02:13:47 +0300 Subject: [PATCH 06/12] doc: delete link to removed doc part PR-URL: https://github.com/nodejs/node/pull/15510 Refs: https://github.com/nodejs/node/pull/15412 Reviewed-By: Colin Ihrig Reviewed-By: Franziska Hinkelmann Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- doc/api/deprecations.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index d1175d480bc230..82391a32e84031 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -148,7 +148,7 @@ instead. Type: End-of-Life -[`Domain.dispose()`][] is removed. Recover from failed I/O actions +`Domain.dispose()` is removed. Recover from failed I/O actions explicitly via error event handlers set on the domain instead. @@ -688,7 +688,6 @@ difference is that `querystring.parse()` does url encoding: [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array [`Buffer.from(buffer)`]: buffer.html#buffer_class_method_buffer_from_buffer [`Buffer.isBuffer()`]: buffer.html#buffer_class_method_buffer_isbuffer_obj -[`Domain.dispose()`]: domain.html#domain_domain_dispose [`EventEmitter.listenerCount(emitter, eventName)`]: events.html#events_eventemitter_listenercount_emitter_eventname [`Server.connections`]: net.html#net_server_connections [`Server.getConnections()`]: net.html#net_server_getconnections_callback From 60ce845d808d18be69b0b24d84cc8bd9d2a420fe Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 18 Sep 2017 16:38:58 +0200 Subject: [PATCH 07/12] src: remove unused static variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/15458 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Tobias Nießen Reviewed-By: Anna Henningsen --- src/module_wrap.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 5d1d60e2be0ff9..294bf9fa9a1ca8 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -327,7 +327,7 @@ inline const struct read_result read_file(uv_file file) { struct file_check { bool failed = true; uv_file file = -1; -} file_check; +}; inline const struct file_check check_file(URL search, bool close = false, bool allow_dir = false) { From 51a9cfcaf1b38912c2f2c960d2966aa08ef404cc Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 18 Sep 2017 16:38:59 +0200 Subject: [PATCH 08/12] src: handle uv_async_init() failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix CHECKED_RETURN, RESOURCE_LEAK) and UNINIT Coverity warnings in MarkGarbageCollectionEnd(). PR-URL: https://github.com/nodejs/node/pull/15458 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Tobias Nießen Reviewed-By: Anna Henningsen --- src/node_perf.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/node_perf.cc b/src/node_perf.cc index 95e93259f478c7..f5aafbab63a781 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -212,13 +212,14 @@ void MarkGarbageCollectionEnd(Isolate* isolate, v8::GCCallbackFlags flags, void* data) { Environment* env = static_cast(data); - uv_async_t *async = new uv_async_t; + uv_async_t* async = new uv_async_t(); // coverity[leaked_storage] + if (uv_async_init(env->event_loop(), async, PerformanceGCCallback)) + return delete async; async->data = new PerformanceEntry::Data(env, "gc", "gc", performance_last_gc_start_mark_, PERFORMANCE_NOW(), type); - uv_async_init(env->event_loop(), async, PerformanceGCCallback); - uv_async_send(async); + CHECK_EQ(0, uv_async_send(async)); } inline void SetupGarbageCollectionTracking(Environment* env) { From dddd5106c25eb41d6c0fb7b23247c5c1ebd88fb6 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 18 Sep 2017 16:38:59 +0200 Subject: [PATCH 09/12] src: return references from getters, not copies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/15458 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Tobias Nießen Reviewed-By: Anna Henningsen --- src/node_perf.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node_perf.h b/src/node_perf.h index ca4374ebc58c19..fe5d2d996819d7 100644 --- a/src/node_perf.h +++ b/src/node_perf.h @@ -73,11 +73,11 @@ class PerformanceEntry : public BaseObject { return env_; } - std::string name() const { + const std::string& name() const { return name_; } - std::string type() const { + const std::string& type() const { return type_; } @@ -135,11 +135,11 @@ class PerformanceEntry : public BaseObject { ~PerformanceEntry() {} - std::string name() const { + const std::string& name() const { return name_; } - std::string type() const { + const std::string& type() const { return type_; } From 8af6ce9c789537ca48c4ffeadf90bab56cb8eb0f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 18 Sep 2017 16:38:59 +0200 Subject: [PATCH 10/12] src: constify PerformanceEntry data members MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/15458 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Tobias Nießen Reviewed-By: Anna Henningsen --- src/node_perf.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/node_perf.h b/src/node_perf.h index fe5d2d996819d7..3d9bb144ff297d 100644 --- a/src/node_perf.h +++ b/src/node_perf.h @@ -94,12 +94,12 @@ class PerformanceEntry : public BaseObject { } private: - Environment* env_; - std::string name_; - std::string type_; - uint64_t startTime_; - uint64_t endTime_; - int data_; + Environment* const env_; + const std::string name_; + const std::string type_; + const uint64_t startTime_; + const uint64_t endTime_; + const int data_; }; static void NotifyObservers(Environment* env, PerformanceEntry* entry); @@ -160,10 +160,10 @@ class PerformanceEntry : public BaseObject { } private: - std::string name_; - std::string type_; - uint64_t startTime_; - uint64_t endTime_; + const std::string name_; + const std::string type_; + const uint64_t startTime_; + const uint64_t endTime_; }; enum PerformanceGCKind { From 5d611c3e6e5fbcd51930d58a13c6cfed8162e2fc Mon Sep 17 00:00:00 2001 From: Gibson Fahnestock Date: Sun, 17 Sep 2017 00:43:54 +0100 Subject: [PATCH 11/12] build: don't fail `make test` on source tarballs Tries to achieve the same effect as https://github.com/nodejs/node/pull/13658 without breaking source tarballs. Presumably if `tools/eslint` wasn't there at all, people would notice in the PR review! PR-URL: https://github.com/nodejs/node/pull/15441 Fixes: https://github.com/nodejs/node/issues/14513 Reviewed-By: Vse Mozhet Byt Reviewed-By: Refael Ackermann Reviewed-By: Michael Dawson Reviewed-By: James M Snell --- Makefile | 3 +-- vcbuild.bat | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index e9c5a2e6bbcf17..66e23f905ffe1a 100644 --- a/Makefile +++ b/Makefile @@ -922,7 +922,7 @@ cpplint: @$(PYTHON) tools/cpplint.py $(CPPLINT_FILES) @$(PYTHON) tools/check-imports.py -ifneq ("","$(wildcard tools/eslint/bin/eslint.js)") +ifneq ("","$(wildcard tools/eslint/)") lint: @EXIT_STATUS=0 ; \ $(MAKE) jslint || EXIT_STATUS=$$? ; \ @@ -943,7 +943,6 @@ lint: @echo "Linting is not available through the source tarball." @echo "Use the git repo instead:" \ "$ git clone https://github.com/nodejs/node.git" - exit 1 lint-ci: lint endif diff --git a/vcbuild.bat b/vcbuild.bat index 8e6c89e2a07b80..77a49edeb6edd5 100644 --- a/vcbuild.bat +++ b/vcbuild.bat @@ -505,7 +505,7 @@ goto exit if defined enable_static goto exit if defined jslint_ci goto jslint-ci if not defined jslint goto exit -if not exist tools\eslint\bin\eslint.js goto no-lint +if not exist tools\eslint goto no-lint echo running jslint %config%\node tools\eslint\bin\eslint.js --cache --rule "linebreak-style: 0" --rulesdir=tools\eslint-rules --ext=.js,.md benchmark doc lib test tools goto exit @@ -518,7 +518,7 @@ goto exit :no-lint echo Linting is not available through the source tarball. echo Use the git repo instead: $ git clone https://github.com/nodejs/node.git -exit /b 1 +goto exit :create-msvs-files-failed echo Failed to create vc project files. From 08a80744bb398828e4e6d4c0235d200570483baf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Szewczak?= Date: Fri, 22 Sep 2017 00:21:41 +0200 Subject: [PATCH 12/12] docs: clarify usage cli options -e,-p on windows Fixes: https://github.com/nodejs/node/issues/15522 --- doc/api/cli.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/api/cli.md b/doc/api/cli.md index b23311281df807..0dea115d4628a0 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -65,6 +65,7 @@ changes: Identical to `-e` but prints the result. +*Note*: For options `-e` and `-p` please use double quote for the script, although it does not matter on the Linux that you use double quote or a single quote, on the Windows it makes a difference. On the Windows a single quote will not work correctly because Windows shell traditionally uses double quote as the quote char. ### `-c`, `--check`