From 9ec78101fad90e6469677598b6bc3e8196d1b17f Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Fri, 24 Nov 2017 14:33:59 -0500 Subject: [PATCH 1/7] doc: add maclover7 to collaborators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/17289 Reviewed-By: Rich Trott Reviewed-By: James M Snell Reviewed-By: Michaël Zasso Reviewed-By: Refael Ackermann --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 01d5ec1ba21..fdff7d21726 100644 --- a/README.md +++ b/README.md @@ -412,6 +412,8 @@ For more information about the governance of the Node.js project, see **Luigi Pinca** <luigipinca@gmail.com> (he/him) * [lucamaraschi](https://github.com/lucamaraschi) - **Luca Maraschi** <luca.maraschi@gmail.com> (he/him) +* [maclover7](https://github.com/maclover7) - +**Jon Moss** <me@jonathanmoss.me> (he/him) * [matthewloring](https://github.com/matthewloring) - **Matthew Loring** <mattloring@google.com> * [mcollina](https://github.com/mcollina) - From 9d87082d7df8b509d3e035b0222899d9cd8e58a3 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 21 Nov 2017 17:09:32 -0800 Subject: [PATCH 2/7] http2: general cleanups in core.js * fixup js debug messages * simplify and improve rstStream * improve and simplify _read * simplify and improve priority * simplify on ready a bit * simplify and improve respond/push * reduce duplication with _unrefActive * simplify stream close handling PR-URL: https://github.com/nodejs/node/pull/17209 Reviewed-By: Anatoli Papirovski --- lib/internal/http2/core.js | 317 +++++++----------- .../test-http2-priority-parent-self.js | 57 ---- 2 files changed, 125 insertions(+), 249 deletions(-) delete mode 100644 test/parallel/test-http2-priority-parent-self.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index de20eb23f2c..39f67c48238 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -79,6 +79,7 @@ const kServer = Symbol('server'); const kSession = Symbol('session'); const kState = Symbol('state'); const kType = Symbol('type'); +const kUpdateTimer = Symbol('update-timer'); const kDefaultSocketTimeout = 2 * 60 * 1000; const kRenegTest = /TLS session renegotiation disabled for this socket/; @@ -149,9 +150,9 @@ function emit(self, ...args) { function onSessionHeaders(handle, id, cat, flags, headers) { const owner = this[kOwner]; const type = owner[kType]; - _unrefActive(owner); - debug(`[${sessionName(type)}] headers were received on ` + - `stream ${id}: ${cat}`); + owner[kUpdateTimer](); + debug(`Http2Stream ${id} [Http2Session ` + + `${sessionName(type)}]: headers received`); const streams = owner[kState].streams; const endOfStream = !!(flags & NGHTTP2_FLAG_END_STREAM); @@ -196,7 +197,8 @@ function onSessionHeaders(handle, id, cat, flags, headers) { } else { event = endOfStream ? 'trailers' : 'headers'; } - debug(`[${sessionName(type)}] emitting stream '${event}' event`); + debug(`Http2Stream ${id} [Http2Session ` + + `${sessionName(type)}]: emitting stream '${event}' event`); process.nextTick(emit, stream, event, obj, flags, headers); } if (endOfStream) { @@ -228,8 +230,7 @@ function onStreamTrailers() { // Readable and Writable sides of the Duplex. function onStreamClose(code) { const stream = this[kOwner]; - _unrefActive(stream); - _unrefActive(stream[kSession]); + stream[kUpdateTimer](); abort(stream); const state = stream[kState]; state.rst = true; @@ -248,7 +249,7 @@ function afterFDClose(err) { // Called when an error event needs to be triggered function onSessionError(error) { const owner = this[kOwner]; - _unrefActive(owner); + owner[kUpdateTimer](); process.nextTick(emit, owner, 'error', error); } @@ -256,8 +257,7 @@ function onSessionError(error) { // to the Http2Stream Duplex for processing. function onStreamRead(nread, buf, handle) { const stream = handle[kOwner]; - _unrefActive(stream); - _unrefActive(stream[kSession]); + stream[kUpdateTimer](); if (nread >= 0 && !stream.destroyed) { if (!stream.push(buf)) { handle.readStop(); @@ -272,8 +272,8 @@ function onStreamRead(nread, buf, handle) { // Resets the cached settings. function onSettings(ack) { const owner = this[kOwner]; - debug(`[${sessionName(owner[kType])}] new settings received`); - _unrefActive(owner); + debug(`Http2Session ${sessionName(owner[kType])}: new settings received`); + owner[kUpdateTimer](); let event = 'remoteSettings'; if (ack) { if (owner[kState].pendingAck > 0) @@ -293,10 +293,10 @@ function onSettings(ack) { // session (which may, in turn, forward it on to the server) function onPriority(id, parent, weight, exclusive) { const owner = this[kOwner]; - debug(`[${sessionName(owner[kType])}] priority advisement for stream ` + - `${id}: \n parent: ${parent},\n weight: ${weight},\n` + - ` exclusive: ${exclusive}`); - _unrefActive(owner); + debug(`Http2Stream ${id} [Http2Session ` + + `${sessionName(owner[kType])}]: priority [parent: ${parent}, ` + + `weight: ${weight}, exclusive: ${exclusive}]`); + owner[kUpdateTimer](); const streams = owner[kState].streams; const stream = streams.get(id); const emitter = stream === undefined ? owner : stream; @@ -315,9 +315,9 @@ function emitFrameError(self, id, type, code) { // frame. This should be exceedingly rare. function onFrameError(id, type, code) { const owner = this[kOwner]; - debug(`[${sessionName(owner[kType])}] error sending frame type ` + + debug(`Http2Session ${sessionName(owner[kType])}: error sending frame type ` + `${type} on stream ${id}, code: ${code}`); - _unrefActive(owner); + owner[kUpdateTimer](); const streams = owner[kState].streams; const stream = streams.get(id); const emitter = stream !== undefined ? stream : owner; @@ -340,7 +340,8 @@ function emitGoaway(self, code, lastStreamID, buf) { // Called by the native layer when a goaway frame has been received function onGoawayData(code, lastStreamID, buf) { const owner = this[kOwner]; - debug(`[${sessionName(owner[kType])}] goaway data received`); + debug(`Http2Session ${sessionName(owner[kType])}: goaway ${code} received ` + + `[last stream id: ${lastStreamID}]`); process.nextTick(emitGoaway, owner, code, lastStreamID, buf); } @@ -350,7 +351,6 @@ function onGoawayData(code, lastStreamID, buf) { // frameLen <= n <= maxPayloadLen. function onSelectPadding(fn) { return function getPadding() { - debug('fetching padding for frame'); const frameLen = paddingBuffer[PADDING_BUF_FRAME_LENGTH]; const maxFramePayloadLen = paddingBuffer[PADDING_BUF_MAX_PAYLOAD_LENGTH]; paddingBuffer[PADDING_BUF_RETURN_VALUE] = @@ -366,7 +366,8 @@ function onSelectPadding(fn) { // will be deferred until the socket is ready to go. function requestOnConnect(headers, options) { const session = this[kSession]; - debug(`[${sessionName(session[kType])}] connected.. initializing request`); + debug(`Http2Session ${sessionName(session[kType])}: connected, ` + + 'initializing request'); const streams = session[kState].streams; validatePriorityOptions(options); @@ -480,7 +481,7 @@ function onSessionInternalError(code) { // of the socket. No other code should read from or write to the socket. function setupHandle(session, socket, type, options) { return function() { - debug(`[${sessionName(type)}] setting up session handle`); + debug(`Http2Session ${sessionName(type)}: setting up session handle`); session[kState].connecting = false; updateOptionsBuffer(options); @@ -515,20 +516,23 @@ function setupHandle(session, socket, type, options) { // Submits a SETTINGS frame to be sent to the remote peer. function submitSettings(settings) { const type = this[kType]; - debug(`[${sessionName(type)}] submitting actual settings`); - _unrefActive(this); + debug(`Http2Session ${sessionName(type)}: submitting settings`); + this[kUpdateTimer](); this[kLocalSettings] = undefined; updateSettingsBuffer(settings); this[kHandle].settings(); - debug(`[${sessionName(type)}] settings complete`); } // Submits a PRIORITY frame to be sent to the remote peer // Note: If the silent option is true, the change will be made // locally with no PRIORITY frame sent. function submitPriority(options) { - _unrefActive(this); - _unrefActive(this[kSession]); + this[kUpdateTimer](); + + // If the parent is the id, do nothing because a + // stream cannot be made to depend on itself. + if (options.parent === this[kID]) + return; this[kHandle].priority(options.parent | 0, options.weight | 0, @@ -539,8 +543,13 @@ function submitPriority(options) { // Submit an RST-STREAM frame to be sent to the remote peer. // This will cause the Http2Stream to be closed. function submitRstStream(code) { - _unrefActive(this); - _unrefActive(this[kSession]); + this[kUpdateTimer](); + + const state = this[kState]; + if (state.rst) return; + state.rst = true; + state.rstCode = code; + const ret = this[kHandle].rstStream(code); if (ret < 0) { const err = new NghttpError(ret); @@ -561,19 +570,18 @@ function doShutdown(options) { state.shuttingDown = false; state.shutdown = true; if (ret < 0) { - debug(`[${sessionName(this[kType])}] shutdown failed! code: ${ret}`); + debug(`Http2Session ${sessionName(this[kType])}: shutdown failed`); const err = new NghttpError(ret); process.nextTick(emit, this, 'error', err); return; } process.nextTick(emit, this, 'shutdown', options); - debug(`[${sessionName(this[kType])}] shutdown is complete`); } // Submit a graceful or immediate shutdown request for the Http2Session. function submitShutdown(options) { const type = this[kType]; - debug(`[${sessionName(type)}] submitting actual shutdown request`); + debug(`Http2Session ${sessionName(type)}: submitting shutdown request`); if (type === NGHTTP2_SESSION_SERVER && options.graceful === true) { // first send a shutdown notice this[kHandle].shutdownNotice(); @@ -712,7 +720,11 @@ class Http2Session extends EventEmitter { // of concurrent streams (2^31-1 is the upper limit on the number // of streams) this.setMaxListeners(kMaxStreams); - debug(`[${sessionName(type)}] http2session created`); + debug(`Http2Session ${sessionName(type)}: created`); + } + + [kUpdateTimer]() { + _unrefActive(this); } setNextStreamID(id) { @@ -854,12 +866,10 @@ class Http2Session extends EventEmitter { throw new errors.Error('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK', this[kState].pendingAck); } - debug(`[${sessionName(this[kType])}] sending settings`); + debug(`Http2Session ${sessionName(this[kType])}: sending settings`); state.pendingAck++; if (state.connecting) { - debug(`[${sessionName(this[kType])}] session still connecting, ` + - 'queue settings'); this.once('connect', submitSettings.bind(this, settings)); return; } @@ -871,7 +881,7 @@ class Http2Session extends EventEmitter { const state = this[kState]; if (state.destroyed || state.destroying) return; - debug(`[${sessionName(this[kType])}] destroying nghttp2session`); + debug(`Http2Session ${sessionName(this[kType])}: destroying`); state.destroying = true; state.destroyed = false; @@ -942,7 +952,7 @@ class Http2Session extends EventEmitter { options.lastStreamID); } - debug(`[${sessionName(type)}] initiating shutdown`); + debug(`Http2Session ${sessionName(type)}: initiating shutdown`); state.shuttingDown = true; if (callback) { @@ -950,13 +960,11 @@ class Http2Session extends EventEmitter { } if (state.connecting) { - debug(`[${sessionName(type)}] session still connecting, queue ` + - 'shutdown'); this.once('connect', submitShutdown.bind(this, options)); return; } - debug(`[${sessionName(type)}] sending shutdown`); + debug(`Http2Session ${sessionName(type)}: sending shutdown`); submitShutdown.call(this, options); } @@ -972,7 +980,7 @@ class Http2Session extends EventEmitter { handle.chunksSentSinceLastWrite : null; if (chunksSentSinceLastWrite !== null && chunksSentSinceLastWrite !== handle.updateChunksSent()) { - _unrefActive(this); + this[kUpdateTimer](); return; } } @@ -995,7 +1003,6 @@ class ServerHttp2Session extends Http2Session { class ClientHttp2Session extends Http2Session { constructor(options, socket) { super(NGHTTP2_SESSION_CLIENT, options, socket); - debug(`[${sessionName(this[kType])}] clienthttp2session created`); } // Submits a new HTTP2 request to the connected peer. Returns the @@ -1004,13 +1011,15 @@ class ClientHttp2Session extends Http2Session { const state = this[kState]; if (state.destroyed || state.destroying) throw new errors.Error('ERR_HTTP2_INVALID_SESSION'); - debug(`[${sessionName(this[kType])}] initiating request`); - _unrefActive(this); + debug(`Http2Session ${sessionName(this[kType])}: initiating request`); + + this[kUpdateTimer](); + assertIsObject(headers, 'headers'); assertIsObject(options, 'options'); headers = Object.assign(Object.create(null), headers); - options = Object.assign(Object.create(null), options); + options = Object.assign({}, options); if (headers[HTTP2_HEADER_METHOD] === undefined) headers[HTTP2_HEADER_METHOD] = HTTP2_METHOD_GET; @@ -1055,19 +1064,14 @@ class ClientHttp2Session extends Http2Session { const stream = new ClientHttp2Stream(this, undefined, undefined, {}); - const onConnect = requestOnConnect.bind(stream, headers, options); - // Close the writable side of the stream if options.endStream is set. if (options.endStream) stream.end(); + const onConnect = requestOnConnect.bind(stream, headers, options); if (state.connecting) { - debug(`[${sessionName(this[kType])}] session still connecting, queue ` + - 'stream init'); stream.on('connect', onConnect); } else { - debug(`[${sessionName(this[kType])}] session connected, immediate ` + - 'stream init'); onConnect(); } return stream; @@ -1107,15 +1111,13 @@ function afterDoStreamWrite(status, handle, req) { const stream = handle[kOwner]; const session = stream[kSession]; - _unrefActive(stream); + stream[kUpdateTimer](); const { bytes } = req; stream[kState].writeQueueSize -= bytes; - if (session !== undefined) { - _unrefActive(session); + if (session !== undefined) session[kState].writeQueueSize -= bytes; - } if (typeof req.callback === 'function') req.callback(); @@ -1142,12 +1144,6 @@ function onSessionClose(hadError, code) { this.end(); // Close the writable side } -function onStreamClosed(code) { - abort(this); - this.push(null); // Close the readable side - this.end(); // Close the writable side -} - function streamOnResume() { if (this[kID] === undefined) { this.once('ready', streamOnResume); @@ -1166,18 +1162,11 @@ function handleFlushData(handle) { function streamOnSessionConnect() { const session = this[kSession]; - debug(`[${sessionName(session[kType])}] session connected. emiting stream ` + - 'connect'); + debug(`Http2Session ${sessionName(session[kType])}: session connected`); this[kState].connecting = false; process.nextTick(emit, this, 'connect'); } -function streamOnceReady() { - const session = this[kSession]; - debug(`[${sessionName(session[kType])}] stream ${this[kID]} is ready`); - this.uncork(); -} - function abort(stream) { if (!stream[kState].aborted && !(stream._writableState.ended || stream._writableState.ending)) { @@ -1207,20 +1196,21 @@ class Http2Stream extends Duplex { writeQueueSize: 0 }; - this.once('ready', streamOnceReady); - this.once('streamClosed', onStreamClosed); this.once('finish', onHandleFinish); this.on('resume', streamOnResume); this.on('pause', streamOnPause); session.once('close', state.closeHandler); if (session[kState].connecting) { - debug(`[${sessionName(session[kType])}] session is still connecting, ` + - 'queuing stream init'); state.connecting = true; session.once('connect', streamOnSessionConnect.bind(this)); } - debug(`[${sessionName(session[kType])}] http2stream created`); + } + + [kUpdateTimer]() { + _unrefActive(this); + if (this[kSession]) + _unrefActive(this[kSession]); } [kInit](id, handle) { @@ -1231,6 +1221,7 @@ class Http2Stream extends Duplex { handle.ontrailers = onStreamTrailers; handle.onstreamclose = onStreamClose; handle.onread = onStreamRead; + this.uncork(); this.emit('ready'); } @@ -1267,8 +1258,7 @@ class Http2Stream extends Duplex { handle.chunksSentSinceLastWrite : null; if (chunksSentSinceLastWrite !== null && chunksSentSinceLastWrite !== handle.updateChunksSent()) { - _unrefActive(this); - _unrefActive(this[kSession]); + this[kUpdateTimer](); return; } } @@ -1311,8 +1301,7 @@ class Http2Stream extends Duplex { return; } - _unrefActive(this); - _unrefActive(this[kSession]); + this[kUpdateTimer](); if (!this[kState].headersSent) this[kProceed](); @@ -1336,8 +1325,7 @@ class Http2Stream extends Duplex { return; } - _unrefActive(this); - _unrefActive(this[kSession]); + this[kUpdateTimer](); if (!this[kState].headersSent) this[kProceed](); @@ -1362,16 +1350,12 @@ class Http2Stream extends Duplex { } _read(nread) { - if (this[kID] === undefined) { - this.once('ready', this._read.bind(this, nread)); - return; - } if (this.destroyed) { this.push(null); return; } - _unrefActive(this); - process.nextTick(handleFlushData, this[kHandle]); + if (this[kHandle] !== undefined) + process.nextTick(handleFlushData, this[kHandle]); } // Submits an RST-STREAM frame to shutdown this stream. @@ -1382,30 +1366,15 @@ class Http2Stream extends Duplex { rstStream(code = NGHTTP2_NO_ERROR) { if (typeof code !== 'number') throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'code', 'number'); + if (code < 0 || code > 2 ** 32 - 1) + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'code'); + const fn = submitRstStream.bind(this, code); if (this[kID] === undefined) { - this.once('ready', this.rstStream.bind(this, code)); + this.once('ready', fn); return; } - - const state = this[kState]; - if (state.rst) { - // rst has already been set by self or peer, do not set again - return; - } - state.rst = true; - state.rstCode = code; - - _unrefActive(this); - _unrefActive(this[kSession]); - - const id = this[kID]; - - if (id === undefined) { - this.once('ready', submitRstStream.bind(this, code)); - return; - } - submitRstStream.call(this, code); + fn(); } rstWithNoError() { @@ -1431,36 +1400,17 @@ class Http2Stream extends Duplex { priority(options) { if (this.destroyed) throw new errors.Error('ERR_HTTP2_INVALID_STREAM'); - const session = this[kSession]; - if (this[kID] === undefined) { - debug(`[${sessionName(session[kType])}] queuing priority for new stream`); - this.once('ready', this.priority.bind(this, options)); - return; - } - debug(`[${sessionName(session[kType])}] sending priority for stream ` + - `${this[kID]}`); - _unrefActive(this); assertIsObject(options, 'options'); - options = Object.assign(Object.create(null), options); + options = Object.assign({}, options); validatePriorityOptions(options); - const id = this[kID]; - debug(`[${sessionName(session[kType])}] sending priority for stream ` + - `${id}`); - - // A stream cannot be made to depend on itself - if (options.parent === id) { - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', - 'parent', - options.parent); - } - - if (id === undefined) { - this.once('ready', submitPriority.bind(this, options)); + const fn = submitPriority.bind(this, options); + if (this[kID] === undefined) { + this.once('ready', fn); return; } - submitPriority.call(this, options); + fn(); } // Called by this.destroy(). @@ -1472,11 +1422,13 @@ class Http2Stream extends Duplex { _destroy(err, callback) { const session = this[kSession]; if (this[kID] === undefined) { - debug(`[${sessionName(session[kType])}] queuing destroy for new stream`); this.once('ready', this._destroy.bind(this, err, callback)); return; } + debug(`Http2Stream ${this[kID]} [Http2Session ` + + `${sessionName(session[kType])}]: destroying stream`); + const state = this[kState]; session[kState].writeQueueSize -= state.writeQueueSize; state.writeQueueSize = 0; @@ -1494,8 +1446,6 @@ function continueStreamDestroy(err, callback) { const session = this[kSession]; const state = this[kState]; - debug(`[${sessionName(session[kType])}] destroying stream ${this[kID]}`); - // Submit RST-STREAM frame if one hasn't been sent already and the // stream hasn't closed normally... const rst = state.rst; @@ -1520,8 +1470,10 @@ function continueStreamDestroy(err, callback) { err = new errors.Error('ERR_HTTP2_STREAM_ERROR', code); } callback(err); + abort(this); + this.push(null); // Close the readable side + this.end(); // Close the writable side process.nextTick(emit, this, 'streamClosed', code); - debug(`[${sessionName(session[kType])}] stream ${this[kID]} destroyed`); } function finishStreamDestroy() { @@ -1696,7 +1648,8 @@ function afterOpen(session, options, headers, streamOptions, err, fd) { function streamOnError(err) { // we swallow the error for parity with HTTP1 // all the errors that ends here are not critical for the project - debug('ServerHttp2Stream errored, avoiding uncaughtException', err); + debug(`Http2Stream ${this[kID]} [Http2Session ` + + `${this[kSession][kType]}: error`, err); } @@ -1707,7 +1660,6 @@ class ServerHttp2Stream extends Http2Stream { this[kProtocol] = headers[HTTP2_HEADER_SCHEME]; this[kAuthority] = headers[HTTP2_HEADER_AUTHORITY]; this.on('error', streamOnError); - debug(`[${sessionName(session[kType])}] created serverhttp2stream`); } // true if the HEADERS frame has been sent @@ -1730,12 +1682,10 @@ class ServerHttp2Stream extends Http2Stream { if (!session.remoteSettings.enablePush) throw new errors.Error('ERR_HTTP2_PUSH_DISABLED'); - debug(`[${sessionName(session[kType])}] initiating push stream for stream` + - ` ${this[kID]}`); + debug(`Http2Stream ${this[kID]} [Http2Session ` + + `${sessionName(session[kType])}]: initiating push stream`); - _unrefActive(this); - const state = session[kState]; - const streams = state.streams; + this[kUpdateTimer](); if (typeof options === 'function') { callback = options; @@ -1746,7 +1696,7 @@ class ServerHttp2Stream extends Http2Stream { throw new errors.TypeError('ERR_INVALID_CALLBACK'); assertIsObject(options, 'options'); - options = Object.assign(Object.create(null), options); + options = Object.assign({}, options); options.endStream = !!options.endStream; assertIsObject(headers, 'headers'); @@ -1762,16 +1712,13 @@ class ServerHttp2Stream extends Http2Stream { headers[HTTP2_HEADER_PATH] = '/'; let headRequest = false; - if (headers[HTTP2_HEADER_METHOD] === HTTP2_METHOD_HEAD) { - headRequest = true; - options.endStream = true; - } + if (headers[HTTP2_HEADER_METHOD] === HTTP2_METHOD_HEAD) + headRequest = options.endStream = true; + options.readable = !options.endStream; const headersList = mapToHeaders(headers); - if (!Array.isArray(headersList)) { - // An error occurred! + if (!Array.isArray(headersList)) throw headersList; - } const streamOptions = options.endStream ? STREAM_OPTION_EMPTY_PAYLOAD : 0; @@ -1794,19 +1741,14 @@ class ServerHttp2Stream extends Http2Stream { } const id = ret.id(); - debug(`[${sessionName(session[kType])}] push stream ${id} created`); - options.readable = !options.endStream; - const stream = new ServerHttp2Stream(session, ret, id, options, headers); - streams.set(id, stream); + session[kState].streams.set(id, stream); - // If the push stream is a head request, close the writable side of - // the stream immediately as there won't be any data sent. - if (headRequest) { + if (options.endStream) stream.end(); - const state = stream[kState]; - state.headRequest = true; - } + + if (headRequest) + stream[kState].headRequest = true; process.nextTick(callback, stream, headers, 0); } @@ -1816,16 +1758,16 @@ class ServerHttp2Stream extends Http2Stream { const session = this[kSession]; if (this.destroyed) throw new errors.Error('ERR_HTTP2_INVALID_STREAM'); - debug(`[${sessionName(session[kType])}] initiating response for stream ` + - `${this[kID]}`); - _unrefActive(this); + debug(`Http2Stream ${this[kID]} [Http2Session ` + + `${sessionName(session[kType])}]: initiating response`); + this[kUpdateTimer](); const state = this[kState]; if (state.headersSent) throw new errors.Error('ERR_HTTP2_HEADERS_SENT'); assertIsObject(options, 'options'); - options = Object.assign(Object.create(null), options); + options = Object.assign({}, options); options.endStream = !!options.endStream; let streamOptions = 0; @@ -1856,10 +1798,9 @@ class ServerHttp2Stream extends Http2Stream { } const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse); - if (!Array.isArray(headersList)) { - // An error occurred! + if (!Array.isArray(headersList)) throw headersList; - } + state.headersSent = true; // Close the writable side if the endStream option is set @@ -1883,9 +1824,9 @@ class ServerHttp2Stream extends Http2Stream { const session = this[kSession]; if (this.destroyed) throw new errors.Error('ERR_HTTP2_INVALID_STREAM'); - debug(`[${sessionName(session[kType])}] initiating response for stream ` + - `${this[kID]}`); - _unrefActive(this); + debug(`Http2Stream ${this[kID]} [Http2Session ` + + `${sessionName(session[kType])}]: initiating response`); + this[kUpdateTimer](); const state = this[kState]; if (state.headersSent) @@ -1966,9 +1907,9 @@ class ServerHttp2Stream extends Http2Stream { const session = this[kSession]; if (this.destroyed) throw new errors.Error('ERR_HTTP2_INVALID_STREAM'); - debug(`[${sessionName(session[kType])}] initiating response for stream ` + - `${this[kID]}`); - _unrefActive(this); + debug(`Http2Stream ${this[kID]} [Http2Session ` + + `${sessionName(session[kType])}]: initiating response`); + this[kUpdateTimer](); const state = this[kState]; if (state.headersSent) @@ -2033,7 +1974,8 @@ class ServerHttp2Stream extends Http2Stream { throw new errors.Error('ERR_HTTP2_HEADERS_AFTER_RESPOND'); const session = this[kSession]; - debug(`[${sessionName(session[kType])}] sending additional headers`); + debug(`Http2Stream ${this[kID]} [Http2Session ` + + `${sessionName(session[kType])}]: sending additional headers`); assertIsObject(headers, 'headers'); headers = Object.assign(Object.create(null), headers); @@ -2047,7 +1989,7 @@ class ServerHttp2Stream extends Http2Stream { } } - _unrefActive(this); + this[kUpdateTimer](); const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse); @@ -2072,7 +2014,6 @@ class ClientHttp2Stream extends Http2Stream { if (id !== undefined) this[kInit](id, handle); this.on('headers', handleHeaderContinue); - debug(`[${sessionName(session[kType])}] clienthttp2stream created`); } } @@ -2101,7 +2042,7 @@ const setTimeout = { } } else { enroll(this, msecs); - _unrefActive(this); + this[kUpdateTimer](); if (callback !== undefined) { if (typeof callback !== 'function') throw new errors.TypeError('ERR_INVALID_CALLBACK'); @@ -2121,13 +2062,12 @@ Object.defineProperty(Http2Session.prototype, 'setTimeout', setTimeout); function socketDestroy(error) { const session = this[kSession]; const type = session[kType]; - debug(`[${sessionName(type)}] socket destroy called`); + debug(`Http2Session ${sessionName(type)}: socket destroy called`); delete this[kServer]; // destroy the session first so that it will stop trying to // send data while we close the socket. session.destroy(); this.destroy = this[kDestroySocket]; - debug(`[${sessionName(type)}] destroying the socket`); this.destroy(error); } @@ -2136,7 +2076,8 @@ function socketDestroy(error) { // a sessionError; failing that, destroy, remove the error listener, and // re-emit the error event function sessionOnError(error) { - debug(`[${sessionName(this[kType])}] server session error: ${error.message}`); + debug(`Http2Session ${sessionName(this[kType])}: session error: ` + + `${error.message}`); if (this[kServer] !== undefined && this[kServer].emit('sessionError', error)) return; if (this[kSocket] !== undefined && this[kSocket].emit('sessionError', error)) @@ -2151,7 +2092,7 @@ function sessionOnError(error) { function socketOnError(error) { const session = this[kSession]; const type = session && session[kType]; - debug(`[${sessionName(type)}] server socket error: ${error.message}`); + debug(`Http2Session ${sessionName(type)}: socket error: ${error.message}`); if (kRenegTest.test(error.message)) return this.destroy(); if (session !== undefined && @@ -2164,12 +2105,11 @@ function socketOnError(error) { // Handles the on('stream') event for a session and forwards // it on to the server object. function sessionOnStream(stream, headers, flags, rawHeaders) { - debug(`[${sessionName(this[kType])}] emit server stream event`); this[kServer].emit('stream', stream, headers, flags, rawHeaders); } function sessionOnPriority(stream, parent, weight, exclusive) { - debug(`[${sessionName(this[kType])}] priority change received`); + debug(`Http2Session ${sessionName(this[kType])}: priority change received`); this[kServer].emit('priority', stream, parent, weight, exclusive); } @@ -2180,7 +2120,6 @@ function sessionOnSocketError(error, socket) { // When the session times out on the server, attempt a graceful shutdown function sessionOnTimeout() { - debug('session timeout'); process.nextTick(() => { const state = this[kState]; // if destroyed or destryoing, do nothing @@ -2204,7 +2143,7 @@ function sessionOnTimeout() { } function connectionListener(socket) { - debug('[server] received a connection'); + debug('Http2Session server: received a connection'); const options = this[kOptions] || {}; if (socket.alpnProtocol === false || socket.alpnProtocol === 'http/1.1') { @@ -2273,7 +2212,6 @@ class Http2SecureServer extends TLSServer { if (typeof requestListener === 'function') this.on('request', requestListener); this.on('tlsClientError', onErrorSecureServerSession); - debug('http2secureserver created'); } setTimeout(msecs, callback) { @@ -2295,7 +2233,6 @@ class Http2Server extends NETServer { this.on('newListener', setupCompat); if (typeof requestListener === 'function') this.on('request', requestListener); - debug('http2server created'); } setTimeout(msecs, callback) { @@ -2311,7 +2248,6 @@ class Http2Server extends NETServer { function setupCompat(ev) { if (ev === 'request') { - debug('setting up compatibility handler'); this.removeListener('newListener', setupCompat); this.on('stream', onServerStream); } @@ -2329,7 +2265,8 @@ function socketOnClose() { // If the session emits an error, forward it to the socket as a sessionError; // failing that, destroy the session, remove the listener and re-emit the error function clientSessionOnError(error) { - debug(`[${sessionName(this[kType])}] session error: ${error.message}`); + debug(`Http2Session ${sessionName(this[kType])}]: session error: ` + + `${error.message}`); if (this[kSocket] !== undefined && this[kSocket].emit('sessionError', error)) return; this.destroy(); @@ -2351,8 +2288,6 @@ function connect(authority, options, listener) { assertIsObject(authority, 'authority', ['string', 'Object', 'URL']); - debug(`connecting to ${authority}`); - const protocol = authority.protocol || options.protocol || 'https:'; const port = '' + (authority.port !== '' ? authority.port : (authority.protocol === 'http:' ? 80 : 443)); @@ -2406,7 +2341,6 @@ function createSecureServer(options, handler) { 'options', 'Object'); } - debug('creating http2secureserver'); return new Http2SecureServer(options, handler); } @@ -2415,7 +2349,6 @@ function createServer(options, handler) { handler = options; options = Object.create(null); } - debug('creating htt2pserver'); return new Http2Server(options, handler); } diff --git a/test/parallel/test-http2-priority-parent-self.js b/test/parallel/test-http2-priority-parent-self.js deleted file mode 100644 index 55a161bf17f..00000000000 --- a/test/parallel/test-http2-priority-parent-self.js +++ /dev/null @@ -1,57 +0,0 @@ -'use strict'; - -const common = require('../common'); -if (!common.hasCrypto) - common.skip('missing crypto'); -const h2 = require('http2'); - -const server = h2.createServer(); -const invalidOptValueError = (value) => ({ - type: TypeError, - code: 'ERR_INVALID_OPT_VALUE', - message: `The value "${value}" is invalid for option "parent"` -}); - -// we use the lower-level API here -server.on('stream', common.mustCall((stream) => { - common.expectsError( - () => stream.priority({ - parent: stream.id, - weight: 1, - exclusive: false - }), - invalidOptValueError(stream.id) - ); - stream.respond({ - 'content-type': 'text/html', - ':status': 200 - }); - stream.end('hello world'); -})); - -server.listen(0, common.mustCall(() => { - - const client = h2.connect(`http://localhost:${server.address().port}`); - const req = client.request({ ':path': '/' }); - - req.on( - 'ready', - () => common.expectsError( - () => req.priority({ - parent: req.id, - weight: 1, - exclusive: false - }), - invalidOptValueError(req.id) - ) - ); - - req.on('response', common.mustCall()); - req.resume(); - req.on('end', common.mustCall(() => { - server.close(); - client.destroy(); - })); - req.end(); - -})); From dc389bf7cc8b297d4f8dd24603ee1793116f3414 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Thu, 23 Nov 2017 15:25:14 +0100 Subject: [PATCH 3/7] n-api: use nullptr instead of NULL in node_api.cc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit changes two checks which use NULL to use nullptr. I'm not very familiar with N-API but wanted to bring this up in case it was something that was overlooked. PR-URL: https://github.com/nodejs/node/pull/17276 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Benjamin Gruenbaum Reviewed-By: Timothy Gu Reviewed-By: Ali Ijaz Sheikh Reviewed-By: Michael Dawson Reviewed-By: Lance Ball Reviewed-By: Alexey Orlenko Reviewed-By: James M Snell Reviewed-By: MichaëZasso --- src/node_api.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 59ea7635120..f16ceefd5eb 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -1220,7 +1220,7 @@ napi_status napi_delete_property(napi_env env, v8::Maybe delete_maybe = obj->Delete(context, k); CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure); - if (result != NULL) + if (result != nullptr) *result = delete_maybe.FromMaybe(false); return GET_RETURN_STATUS(env); @@ -1398,7 +1398,7 @@ napi_status napi_delete_element(napi_env env, v8::Maybe delete_maybe = obj->Delete(context, index); CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure); - if (result != NULL) + if (result != nullptr) *result = delete_maybe.FromMaybe(false); return GET_RETURN_STATUS(env); From ad80c2120672975018f5d93dad5e5cb9cf900de2 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Fri, 24 Nov 2017 16:08:47 +0100 Subject: [PATCH 4/7] trace_events: add executionAsyncId to init events async_hooks emits trace_events. This adds the executionAsyncId to the init events. In theory this could be inferred from the before and after events but this is much simpler and doesn't require knowledge of all events. PR-URL: https://github.com/nodejs/node/pull/17196 Reviewed-By: James M Snell Reviewed-By: Franziska Hinkelmann Reviewed-By: Colin Ihrig --- lib/internal/trace_events_async_hooks.js | 4 ++- src/async_wrap.cc | 7 ++++-- src/node_trace_events.cc | 25 +++++++++++++------ .../parallel/test-trace-events-async-hooks.js | 10 +++++++- test/parallel/test-trace-events-binding.js | 19 +++++++++++--- 5 files changed, 51 insertions(+), 14 deletions(-) diff --git a/lib/internal/trace_events_async_hooks.js b/lib/internal/trace_events_async_hooks.js index 9f47f2aa5fd..6724e0fdef5 100644 --- a/lib/internal/trace_events_async_hooks.js +++ b/lib/internal/trace_events_async_hooks.js @@ -28,7 +28,9 @@ const hook = async_hooks.createHook({ typeMemory.set(asyncId, type); trace_events.emit(BEFORE_EVENT, 'node.async_hooks', - type, asyncId, 'triggerAsyncId', triggerAsyncId); + type, asyncId, + 'triggerAsyncId', triggerAsyncId, + 'executionAsyncId', async_hooks.executionAsyncId()); }, before(asyncId) { diff --git a/src/async_wrap.cc b/src/async_wrap.cc index b1a8f689ab3..c5e97bd4a66 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -703,9 +703,12 @@ void AsyncWrap::AsyncReset(double execution_async_id, bool silent) { switch (provider_type()) { #define V(PROVIDER) \ case PROVIDER_ ## PROVIDER: \ - TRACE_EVENT_NESTABLE_ASYNC_BEGIN1("node.async_hooks", \ + TRACE_EVENT_NESTABLE_ASYNC_BEGIN2("node.async_hooks", \ #PROVIDER, static_cast(get_async_id()), \ - "triggerAsyncId", static_cast(get_trigger_async_id())); \ + "executionAsyncId", \ + static_cast(env()->execution_async_id()), \ + "triggerAsyncId", \ + static_cast(get_trigger_async_id())); \ break; NODE_ASYNC_PROVIDER_TYPES(V) #undef V diff --git a/src/node_trace_events.cc b/src/node_trace_events.cc index b0ffe68eae3..f269b32fbef 100644 --- a/src/node_trace_events.cc +++ b/src/node_trace_events.cc @@ -70,20 +70,19 @@ static void Emit(const FunctionCallbackInfo& args) { id = args[3]->IntegerValue(context).ToChecked(); } - // TODO(AndreasMadsen): Two extra arguments are not supported. // TODO(AndreasMadsen): String values are not supported. int32_t num_args = 0; - const char* arg_names[1]; - uint8_t arg_types[1]; - uint64_t arg_values[1]; + const char* arg_names[2]; + uint8_t arg_types[2]; + uint64_t arg_values[2]; // Create Utf8Value in the function scope to prevent deallocation. // The c-string will be copied by TRACE_EVENT_API_ADD_TRACE_EVENT at the end. Utf8Value arg1NameValue(env->isolate(), args[4]); + Utf8Value arg2NameValue(env->isolate(), args[6]); - if (args.Length() < 6 || (args[4]->IsUndefined() && args[5]->IsUndefined())) { - num_args = 0; - } else { + if (args.Length() >= 6 && + (!args[4]->IsUndefined() || !args[5]->IsUndefined())) { num_args = 1; arg_types[0] = TRACE_VALUE_TYPE_INT; @@ -94,6 +93,18 @@ static void Emit(const FunctionCallbackInfo& args) { arg_values[0] = args[5]->IntegerValue(context).ToChecked(); } + if (args.Length() >= 8 && + (!args[6]->IsUndefined() || !args[7]->IsUndefined())) { + num_args = 2; + arg_types[1] = TRACE_VALUE_TYPE_INT; + + CHECK(args[6]->IsString()); + arg_names[1] = arg2NameValue.out(); + + CHECK(args[7]->IsNumber()); + arg_values[1] = args[7]->IntegerValue(context).ToChecked(); + } + // The TRACE_EVENT_FLAG_COPY flag indicates that the name and argument // name should be copied thus they don't need to long-lived pointers. // The category name still won't be copied and thus need to be a long-lived diff --git a/test/parallel/test-trace-events-async-hooks.js b/test/parallel/test-trace-events-async-hooks.js index 7f8fb106200..e1f78f791a6 100644 --- a/test/parallel/test-trace-events-async-hooks.js +++ b/test/parallel/test-trace-events-async-hooks.js @@ -43,7 +43,6 @@ proc.once('exit', common.mustCall(() => { return true; })); - // JavaScript async_hooks trace events should be generated. assert(traces.some((trace) => { if (trace.pid !== proc.pid) @@ -54,5 +53,14 @@ proc.once('exit', common.mustCall(() => { return false; return true; })); + + // Check args in init events + const initEvents = traces.filter((trace) => { + return (trace.ph === 'b' && !trace.name.includes('_CALLBACK')); + }); + assert(initEvents.every((trace) => { + return (trace.args.executionAsyncId > 0 && + trace.args.triggerAsyncId > 0); + })); })); })); diff --git a/test/parallel/test-trace-events-binding.js b/test/parallel/test-trace-events-binding.js index 628c9cace71..9a182821bac 100644 --- a/test/parallel/test-trace-events-binding.js +++ b/test/parallel/test-trace-events-binding.js @@ -10,7 +10,10 @@ const CODE = ` 'type-value', 10, 'extra-value', 20); process.binding("trace_events").emit( 'b'.charCodeAt(0), 'custom', - 'type-value', 20); + 'type-value', 20, 'first-value', 20, 'second-value', 30); + process.binding("trace_events").emit( + 'b'.charCodeAt(0), 'custom', + 'type-value', 30); process.binding("trace_events").emit( 'b'.charCodeAt(0), 'missing', 'type-value', 10, 'extra-value', 20); @@ -29,7 +32,7 @@ proc.once('exit', common.mustCall(() => { assert(common.fileExists(FILE_NAME)); fs.readFile(FILE_NAME, common.mustCall((err, data) => { const traces = JSON.parse(data.toString()).traceEvents; - assert.strictEqual(traces.length, 2); + assert.strictEqual(traces.length, 3); assert.strictEqual(traces[0].pid, proc.pid); assert.strictEqual(traces[0].ph, 'b'); @@ -43,6 +46,16 @@ proc.once('exit', common.mustCall(() => { assert.strictEqual(traces[1].cat, 'custom'); assert.strictEqual(traces[1].name, 'type-value'); assert.strictEqual(traces[1].id, '0x14'); - assert.deepStrictEqual(traces[1].args, { }); + assert.deepStrictEqual(traces[1].args, { + 'first-value': 20, + 'second-value': 30 + }); + + assert.strictEqual(traces[2].pid, proc.pid); + assert.strictEqual(traces[2].ph, 'b'); + assert.strictEqual(traces[2].cat, 'custom'); + assert.strictEqual(traces[2].name, 'type-value'); + assert.strictEqual(traces[2].id, '0x1e'); + assert.deepStrictEqual(traces[2].args, { }); })); })); From 73154c0341145985ac7e9b61841a58805d82f533 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 21 Nov 2017 17:01:31 -0800 Subject: [PATCH 5/7] doc: add ES Modules entry to who-to-cc Add ES Modules entry for who-to-cc. Resisted temptation to change it to "whom to CC". Did move async_hooks entry, though. PR-URL: https://github.com/nodejs/node/pull/17205 Reviewed-By: Refael Ackermann Reviewed-By: Gireesh Punathil Reviewed-By: Luigi Pinca Reviewed-By: Daniel Bevenius Reviewed-By: James M Snell Reviewed-By: Jon Moss Reviewed-By: Alexey Orlenko --- doc/onboarding-extras.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/onboarding-extras.md b/doc/onboarding-extras.md index bd6350ebfe3..0da3b1b5259 100644 --- a/doc/onboarding-extras.md +++ b/doc/onboarding-extras.md @@ -8,6 +8,7 @@ | `bootstrap_node.js` | @fishrock123 | | `doc/*`, `*.md` | @nodejs/documentation | | `lib/assert` | @nodejs/testing | +| `lib/async_hooks` | @nodejs/async\_hooks for bugs/reviews (+ @nodejs/diagnostics for API) | | `lib/buffer` | @nodejs/buffer | | `lib/child_process` | @bnoordhuis, @cjihrig | | `lib/cluster` | @bnoordhuis, @cjihrig, @mcollina | @@ -29,8 +30,8 @@ | `src/node_crypto.*` | @nodejs/crypto | | `test/*` | @nodejs/testing | | `tools/eslint`, `.eslintrc` | @not-an-aardvark, @silverwind, @trott | -| async\_hooks | @nodejs/async\_hooks for bugs/reviews (+ @nodejs/diagnostics for API) | | build | @nodejs/build | +| ES Modules | @bmeck, @Fishrock123, @guybedford, @MylesBorins, @targos | | GYP | @nodejs/gyp | | performance | @nodejs/performance | | platform specific | @nodejs/platform-{aix,arm,freebsd,macos,ppc,smartos,s390,windows} | From 07d34092b117c4b5bdad85cf916d9ae5e213651d Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 21 Nov 2017 04:47:57 +0800 Subject: [PATCH 6/7] fs: throw fs.access errors in JS - Migrate the type check of path to ERR_INVALID_ARG_TYPE - Add template counterparts of ASYNC_CALL, ASYNC_DEST_CALL, SYNC_CALL, SYNC_DEST_CALL - Port StringFromPath and UVException to JavaScript - Migrate the access binding to collect the error context in C++, then throw the error in JS PR-URL: https://github.com/nodejs/node/pull/17160 Reviewed-By: Franziska Hinkelmann Reviewed-By: James M Snell --- lib/fs.js | 18 ++++++++- lib/internal/errors.js | 42 ++++++++++++++++++++ src/node_file.cc | 68 ++++++++++++++++++++++++++++----- test/parallel/test-fs-access.js | 13 +++++-- test/parallel/test-fs-buffer.js | 13 +++++-- 5 files changed, 137 insertions(+), 17 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index a53290274c3..a61cfaf4697 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -297,6 +297,11 @@ fs.access = function(path, mode, callback) { if (handleError((path = getPathFromURL(path)), callback)) return; + if (typeof path !== 'string' && !(path instanceof Buffer)) { + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'path', + ['string', 'Buffer', 'URL']); + } + if (!nullCheck(path, callback)) return; @@ -308,6 +313,12 @@ fs.access = function(path, mode, callback) { fs.accessSync = function(path, mode) { handleError((path = getPathFromURL(path))); + + if (typeof path !== 'string' && !(path instanceof Buffer)) { + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'path', + ['string', 'Buffer', 'URL']); + } + nullCheck(path); if (mode === undefined) @@ -315,7 +326,12 @@ fs.accessSync = function(path, mode) { else mode = mode | 0; - binding.access(pathModule.toNamespacedPath(path), mode); + const ctx = {}; + binding.access(pathModule.toNamespacedPath(path), mode, undefined, ctx); + + if (ctx.code !== undefined) { + throw new errors.uvException(ctx); + } }; fs.exists = function(path, callback) { diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 3be2705648d..619732df5d6 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -194,7 +194,49 @@ function E(sym, val) { messages.set(sym, typeof val === 'function' ? val : String(val)); } +// JS counterpart of StringFromPath, although here path is a buffer. +function stringFromPath(path) { + const str = path.toString(); + if (process.platform !== 'win32') { + return str; + } + + if (str.startsWith('\\\\?\\UNC\\')) { + return '\\\\' + str.slice(8); + } else if (str.startsWith('\\\\?\\')) { + return str.slice(4); + } + return str; +} + +// This creates an error compatible with errors produced in UVException +// using the context collected in CollectUVExceptionInfo +// The goal is to migrate them to ERR_* errors later when +// compatibility is not a concern +function uvException(ctx) { + const err = new Error(); + err.errno = ctx.errno; + err.code = ctx.code; + err.syscall = ctx.syscall; + + let message = `${ctx.code}: ${ctx.message}, ${ctx.syscall}`; + if (ctx.path) { + const path = stringFromPath(ctx.path); + message += ` '${path}'`; + err.path = path; + } + if (ctx.dest) { + const dest = stringFromPath(ctx.dest); + message += ` -> '${dest}'`; + err.dest = dest; + } + err.message = message; + Error.captureStackTrace(err, uvException); + return err; +} + module.exports = exports = { + uvException, message, Error: makeNodeError(Error), TypeError: makeNodeError(TypeError), diff --git a/src/node_file.cc b/src/node_file.cc index 12a9f411bc3..2b3b007976d 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -349,6 +349,31 @@ class fs_req_wrap { DISALLOW_COPY_AND_ASSIGN(fs_req_wrap); }; +// Template counterpart of ASYNC_DEST_CALL +template +inline FSReqWrap* AsyncDestCall(Environment* env, Local req, + const char* dest, enum encoding enc, const char* syscall, + Func fn, Args... args) { + FSReqWrap* req_wrap = FSReqWrap::New(env, req, syscall, dest, enc); + int err = fn(env->event_loop(), req_wrap->req(), args..., After); + req_wrap->Dispatched(); + if (err < 0) { + uv_fs_t* uv_req = req_wrap->req(); + uv_req->result = err; + uv_req->path = nullptr; + After(uv_req); + req_wrap = nullptr; + } + + return req_wrap; +} + +// Template counterpart of ASYNC_CALL +template +inline FSReqWrap* AsyncCall(Environment* env, Local req, + enum encoding enc, const char* syscall, Func fn, Args... args) { + return AsyncDestCall(env, req, nullptr, enc, syscall, fn, args...); +} #define ASYNC_DEST_CALL(func, request, dest, encoding, ...) \ Environment* env = Environment::GetCurrent(args); \ @@ -373,6 +398,28 @@ class fs_req_wrap { #define ASYNC_CALL(func, req, encoding, ...) \ ASYNC_DEST_CALL(func, req, nullptr, encoding, __VA_ARGS__) \ +// Template counterpart of SYNC_DEST_CALL +template +inline void SyncDestCall(Environment* env, Local ctx, + const char* path, const char* dest, const char* syscall, + Func fn, Args... args) { + fs_req_wrap req_wrap; + env->PrintSyncTrace(); + int err = fn(env->event_loop(), &req_wrap.req, args..., nullptr); + if (err) { + Local context = env->context(); + Local ctx_obj = ctx->ToObject(context).ToLocalChecked(); + env->CollectUVExceptionInfo(ctx_obj, err, syscall, nullptr, path, dest); + } +} + +// Template counterpart of SYNC_CALL +template +inline void SyncCall(Environment* env, Local ctx, + const char* path, const char* syscall, Func fn, Args... args) { + return SyncDestCall(env, ctx, path, nullptr, syscall, fn, args...); +} + #define SYNC_DEST_CALL(func, path, dest, ...) \ fs_req_wrap req_wrap; \ env->PrintSyncTrace(); \ @@ -394,21 +441,22 @@ class fs_req_wrap { void Access(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args.GetIsolate()); HandleScope scope(env->isolate()); - - if (args.Length() < 2) - return TYPE_ERROR("path and mode are required"); - if (!args[1]->IsInt32()) - return TYPE_ERROR("mode must be an integer"); + Local context = env->context(); + CHECK_GE(args.Length(), 2); + CHECK(args[1]->IsInt32()); BufferValue path(env->isolate(), args[0]); - ASSERT_PATH(path) - - int mode = static_cast(args[1]->Int32Value()); + int mode = static_cast(args[1]->Int32Value(context).FromJust()); if (args[2]->IsObject()) { - ASYNC_CALL(access, args[2], UTF8, *path, mode); + Local req_obj = args[2]->ToObject(context).ToLocalChecked(); + FSReqWrap* req_wrap = AsyncCall( + env, req_obj, UTF8, "access", uv_fs_access, *path, mode); + if (req_wrap != nullptr) { + args.GetReturnValue().Set(req_wrap->persistent()); + } } else { - SYNC_CALL(access, *path, *path, mode); + SyncCall(env, args[3], *path, "access", uv_fs_access, *path, mode); } } diff --git a/test/parallel/test-fs-access.js b/test/parallel/test-fs-access.js index d3140941bd0..719e1108fe1 100644 --- a/test/parallel/test-fs-access.js +++ b/test/parallel/test-fs-access.js @@ -81,9 +81,16 @@ fs.access(readOnlyFile, fs.W_OK, common.mustCall((err) => { } })); -assert.throws(() => { - fs.access(100, fs.F_OK, common.mustNotCall()); -}, /^TypeError: path must be a string or Buffer$/); +common.expectsError( + () => { + fs.access(100, fs.F_OK, common.mustNotCall()); + }, + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "path" argument must be one of type string, Buffer, or URL' + } +); common.expectsError( () => { diff --git a/test/parallel/test-fs-buffer.js b/test/parallel/test-fs-buffer.js index 1cbead43446..bc2add52187 100644 --- a/test/parallel/test-fs-buffer.js +++ b/test/parallel/test-fs-buffer.js @@ -25,9 +25,16 @@ assert.doesNotThrow(() => { })); }); -assert.throws(() => { - fs.accessSync(true); -}, /path must be a string or Buffer/); +common.expectsError( + () => { + fs.accessSync(true); + }, + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "path" argument must be one of type string, Buffer, or URL' + } +); const dir = Buffer.from(fixtures.fixturesDir); fs.readdir(dir, 'hex', common.mustCall((err, hexList) => { From 29423b49c78f7880fa07f52447ce17c2c4aff5bc Mon Sep 17 00:00:00 2001 From: pkovacs Date: Sat, 25 Nov 2017 11:35:42 +0100 Subject: [PATCH 7/7] doc: fix typo in api doc of url.format(urlObject) PR-URL: https://github.com/nodejs/node/pull/17295 Fixes: https://github.com/nodejs/node/issues/17294 Reviewed-By: Vse Mozhet Byt Reviewed-By: Gireesh Punathil Reviewed-By: Luigi Pinca Reviewed-By: Evan Lucas Reviewed-By: Anatoli Papirovski --- doc/api/url.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/url.md b/doc/api/url.md index 632eef82e44..00211095627 100644 --- a/doc/api/url.md +++ b/doc/api/url.md @@ -971,7 +971,7 @@ changes: The `url.format()` method returns a formatted URL string derived from `urlObject`. -If `urlObject` is not an object or a string, `url.parse()` will throw a +If `urlObject` is not an object or a string, `url.format()` will throw a [`TypeError`][]. The formatting process operates as follows: