From e31078e3d240cd22dfec131ad6910ee42c19cbda Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 31 May 2017 10:42:11 -0700 Subject: [PATCH] http2: additional cleanups, fixes and docs * Follow the model of the existing http code and pass the headers in to js as an array. Building the array on the native side then converting it to an object on the js side is generally faster than building the object on the native side. This also allows us to eliminate some duplicated checks. * Fill in additional doc details * Work on timeouts --- doc/api/http2.md | 203 ++++++++++++++++++------- lib/internal/http2/core.js | 40 +++-- lib/internal/http2/util.js | 33 ++++- src/node_http2.cc | 214 +++++---------------------- src/node_http2_core-inl.h | 62 ++++++-- src/node_http2_core.cc | 27 ++++ src/node_http2_core.h | 11 +- test/parallel/test-http2-timeouts.js | 31 ++++ 8 files changed, 355 insertions(+), 266 deletions(-) create mode 100644 test/parallel/test-http2-timeouts.js diff --git a/doc/api/http2.md b/doc/api/http2.md index 650136ab32..adc0b0c118 100755 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -130,9 +130,9 @@ session.on('stream', (stream, headers, flags) => { }); ``` -*Note*: User code will typically not listen for this event directly, and would -instead register a handler for the `'stream'` event emitted by the `net.Server` -or `tls.Server` instances returned by `http2.createServer()` and +On the server side, user code will typically not listen for this event directly, +and would instead register a handler for the `'stream'` event emitted by the +`net.Server` or `tls.Server` instances returned by `http2.createServer()` and `http2.createSecureServer()`, respectively, as in the example below: ```js @@ -164,7 +164,14 @@ not handled, the `'error'` event will be re-emitted on the `Http2Session`. #### Event: 'timeout' -(TODO: fill in detail) +After the `http2session.setTimeout()` method is used to set the timeout period +for this `Http2Session`, the `'timeout'` event is emitted if there is no +activity on the `Http2Session` after the configured number of milliseconds. + +```js +session.setTimeout(2000); +session.on('timeout', () => { /** .. **/ }); +``` #### http2session.destroy() @@ -182,15 +189,15 @@ longer be used, otherwise `false`. * Value: {[Settings Object][]} -An object describing the current local settings of this `Http2Session`. The -local settings are local to *this* `Http2Session` instance. +A prototype-less object describing the current local settings of this +`Http2Session`. The local settings are local to *this* `Http2Session` instance. #### http2session.remoteSettings * Value: {[Settings Object][]} -An object describing the current remote settings of this `Http2Session`. The -remote settings are set by the *connected* HTTP/2 peer. +A prototype-less object describing the current remote settings of this +`Http2Session`. The remote settings are set by the *connected* HTTP/2 peer. #### http2session.request(headers[, options]) @@ -216,41 +223,77 @@ HTTP/2 request to the connected server. This method is only available if `http2session.type` is equal to `http2.constants.NGHTTP2_SESSION_CLIENT`. -(TODO: fill in detail) +```js +const http2 = require('http2'); +const clientSession = http2.connect('https://localhost:1234'); +const { + HTTP2_HEADER_PATH, + HTTP2_HEADER_STATUS +} = http2.constants; + +const req = clientSession.request({ [HTTP2_HEADER_PATH]: '/' }); +req.on('response', (headers) => { + console.log(HTTP2_HEADER_STATUS); + req.on('data', (chunk) => { /** .. **/ }); + req.on('end', () => { /** .. **/ }); +}); +``` #### http2session.rstStream(stream, code) * stream {Http2Stream} -* code {number} +* code {number} Unsigned 32-bit integer identifying the error code. Defaults to + `http2.constant.NGHTTP2_NO_ERROR` (`0x00`) Sends an `RST_STREAM` frame to the connected HTTP/2 peer, causing the given -`Http2Stream` to be closed on both sides using error code `code`. +`Http2Stream` to be closed on both sides using [error code][] `code`. #### http2session.setTimeout(msecs, callback) * `msecs` {number} * `callback` {Function} -(TODO: fill in detail) +Used to set a callback function that is called when there is no activity on +the `Http2Session` after `msecs` milliseconds. The given `callback` is +registered as a listener on the `'timeout'` event. #### http2session.shutdown(options[, callback]) * `options` {Object} * `graceful` {boolean} `true` to attempt a polite shutdown of the `Http2Session`. - * `errorCode` {number} The HTTP/2 Error Code to return. Note that this is - *not* the same thing as an HTTP Response Status Code. + * `errorCode` {number} The HTTP/2 [error code][] to return. Note that this is + *not* the same thing as an HTTP Response Status Code. Defaults to `0x00` + (No Error). * `lastStreamID` {number} The Stream ID of the last successfully processed `Http2Stream` on this `Http2Session`. * `opaqueData` {Buffer} A `Buffer` instance containing arbitrary additional data to send to the peer upon disconnection. This is used, typically, to provide additional data for debugging failures, if necessary. -* `callback` {Function} +* `callback` {Function} A callback that is invoked after the session shutdown + has been completed. Attempts to shutdown this `Http2Session` using HTTP/2 defined procedures. If specified, the given `callback` function will be invoked once the shutdown process has completed. +Note that calling `http2session.shutdown()` does *not* destroy the session or +tear down the `Socket` connection. It merely prompts both sessions to begin +preparing to cease activity. + +During a "graceful" shutdown, the session will first send a `GOAWAY` frame to +the connected peer identifying the last processed stream as 232-1. +Then, on the next tick of the event loop, a second `GOAWAY` frame identifying +the most recently processed stream identifier is sent. This process allows the +remote peer to begin preparing for the connection to be terminated. + +```js +session.shutdown({ + graceful: true, + opaqueData: Buffer.from('add some debugging data here') +}, () => session.destroy()); +``` + #### http2session.socket * Value: {net.Socket|tls.TLSSocket} @@ -417,7 +460,9 @@ this event is emitted, the `Http2Stream` instance is no longer usable. #### Event: 'timeout' -(TODO: fill in detail) +The `'timeout'` event is emitted after no activity is received for this +`'Http2Stream'` within the number of millseconds set using +`http2stream.setTimeout()`. #### Event: 'trailers' @@ -427,7 +472,7 @@ trailing header fields is received. The listener callback is passed the ```js stream.on('trailers', (headers, flags) => { - // TODO(jasnell): Fill in example + console.log(headers); }); ``` @@ -459,36 +504,38 @@ peer. * Value: {number} -Set to the `RST_STREAM` error code when the `Http2Stream` is destroyed after -either receiving an `RST_STREAM` frame from the connected peer, calling -`http2stream.rstStream()`, or `http2stream.destroy()`. +Set to the `RST_STREAM` [error code][] reported when the `Http2Stream` is +destroyed after either receiving an `RST_STREAM` frame from the connected peer, +calling `http2stream.rstStream()`, or `http2stream.destroy()`. Will be +`undefined` if the `Http2Stream` has not been closed. #### http2stream.rstStream(code) -* `code` {number} +* code {number} Unsigned 32-bit integer identifying the error code. Defaults to + `http2.constant.NGHTTP2_NO_ERROR` (`0x00`) Sends an `RST_STREAM` frame to the connected HTTP/2 peer, causing this -`Http2Stream` to be closed on both sides using error code `code`. +`Http2Stream` to be closed on both sides using [error code][] `code`. #### http2stream.rstWithNoError() -Shortcut for `http2stream.rstStream()` using error code `NO_ERROR`. +Shortcut for `http2stream.rstStream()` using error code `0x00` (No Error). #### http2stream.rstWithProtocolError() { -Shortcut for `http2stream.rstStream()` using error code `PROTOCOL_ERROR`. +Shortcut for `http2stream.rstStream()` using error code `0x01` (Protocol Error). #### http2stream.rstWithCancel() { -Shortcut for `http2stream.rstStream()` using error code `CANCEL`. +Shortcut for `http2stream.rstStream()` using error code `0x08` (Cancel). #### http2stream.rstWithRefuse() { -Shortcut for `http2stream.rstStream()` using error code `REFUSED_STREAM`. +Shortcut for `http2stream.rstStream()` using error code `0x07` (Refused Stream). #### http2stream.rstWithInternalError() { -Shortcut for `http2stream.rstStream()` using error code `INTERNAL_ERROR`. +Shortcut for `http2stream.rstStream()` using error code `0x02` (Internal Error). #### http2stream.session @@ -502,7 +549,15 @@ value will be `undefined` after the `Http2Stream` instance is destroyed. * `msecs` {number} * `callback` {Function} -(TODO: fill in detail) +```js +const http2 = require('http2'); +const client = http2.connect('http://example.org:8000'); + +const req = client.request({':path': '/'}); + +// Cancel the stream if there's no activity after 5 seconds +req.setTimeout(5000, () => req.rstStreamWithCancel()); +``` #### http2stream.state @@ -534,7 +589,7 @@ the headers. ```js stream.on('headers', (headers, flags) => { - // TODO(jasnell): Fill in example + console.log(headers); }); ``` @@ -546,7 +601,7 @@ associated with the headers. ```js stream.on('push', (headers, flags) => { - // TODO(jasnell): Fill in example + console.log(headers); }); ``` @@ -577,18 +632,6 @@ used exclusively on HTTP/2 Servers. `Http2Stream` instances on the server provide additional methods such as `http2stream.pushStream()` and `http2stream.respond()` that are only relevant on the server. -#### Event: 'request' - -The `'request'` event is emitted when a block of headers associated with an -HTTP request is received. The listener callback is passed the [Headers Object][] -and flags associated with the headers. - -```js -stream.on('request', (headers, flags) => { - // TODO(jasnell): Fill in example -}); -``` - #### http2stream.additionalHeaders(headers) * `headers` {[Headers Object][]} @@ -608,18 +651,38 @@ Sends an additional informational `HEADERS` frame to the connected HTTP/2 peer. * `weight` {number} Specifies the relative dependency of a stream in relation to other streams with the same `parent`. The value is a number between `1` and `256` (inclusive). -* `callback` {Function} +* `callback` {Function} Callback that is called once the push stream has been + initiated. + +Initiates a push stream. The callback is invoked with the new `Htt2Stream` +instance created for the push stream. -Initiates a push stream. -(TODO: fill in detail) +```js +const http2 = require('http2'); +const server = http2.createServer(); +server.on('stream', (stream) => { + stream.respond({':status': 200}); + stream.pushStream({':path': '/'}, (pushStream) => { + pushStream.respond({':status': 200}); + pushStream.end('some pushed data'); + }); + stream.end('some data'); +}); +``` #### http2stream.respond([headers[, options]]) * `headers` {[Headers Object][]} * `options` {Object} -Initiates a response. -(TODO: fill in detail) +```js +const http2 = require('http2'); +const server = http2.createServer(); +server.on('stream', (stream) => { + stream.respond({':status': 200}); + stream.end('some data'); +}); +``` ### Class: Http2Server @@ -667,7 +730,8 @@ server.on('stream', (stream, headers, flags) => { #### Event: 'timeout' -(TODO: fill in detail) +The `'timeout'` event is emitted when there is no activity on the Server for +a given number of milliseconds set using `http2server.setTimeout()`. ### Class: Http2SecureServer @@ -717,7 +781,8 @@ server.on('stream', (stream, headers, flags) => { #### Event: 'timeout' -(TODO: fill in detail) +The `'timeout'` event is emitted when there is no activity on the Server for +a given number of milliseconds set using `http2server.setTimeout()`. ### http2.getDefaultSettings() @@ -882,11 +947,37 @@ server.listen(80); * Returns `Http2Session` Returns a HTTP/2 client `Http2Session` instance. -(TODO: fill in detail) + +```js +const http2 = require('http2'); +const client = http2.connect('https://localhost:1234'); + +/** use the client **/ + +client.destroy(); +``` ### http2.constants -(TODO: Fill in details) +#### Error Codes for RST_STREAM and GOAWAY + + +| Value | Name | Constant | +|-------|---------------------|-----------------------------------------------| +| 0x00 | No Error | `http2.constants.NGHTTP2_NO_ERROR` | +| 0x01 | Protocol Error | `http2.constants.NGHTTP2_PROTOCOL_ERROR` | +| 0x02 | Internal Error | `http2.constants.NGHTTP2_INTERNAL_ERROR` | +| 0x03 | Flow Control Error | `http2.constants.NGHTTP2_FLOW_CONTROL_ERROR` | +| 0x04 | Settings Timeout | `http2.constants.NGHTTP2_SETTINGS_TIMEOUT` | +| 0x05 | Stream Closed | `http2.constants.NGHTTP2_STREAM_CLOSED` | +| 0x06 | Frame Size Error | `http2.constants.NGHTTP2_FRAME_SIZE_ERROR` | +| 0x07 | Refused Stream | `http2.constants.NGHTTP2_REFUSED_STREAM` | +| 0x08 | Cancel | `http2.constants.NGHTTP2_CANCEL` | +| 0x09 | Compression Error | `http2.constants.NGHTTP2_COMPRESSION_ERROR` | +| 0x0a | Connect Error | `http2.constants.NGHTTP2_CONNECT_ERROR` | +| 0x0b | Enhance Your Calm | `http2.constants.NGHTTP2_ENHANCE_YOUR_CALM` | +| 0x0c | Inadequate Security | `http2.constants.NGHTTP2_INADEQUATE_SECURITY` | +| 0x0d | HTTP/1.1 Required | `http2.constants.NGHTTP2_HTTP_1_1_REQUIRED` | ### Headers Object @@ -912,7 +1003,14 @@ prototype. This means that normal JavaScript object methods such as `Object.prototype.toString()` and `Object.prototype.hasOwnProperty()` will not work. -(TODO: Fill in more detail) +```js +const http2 = require('http2'); +const server = http2.createServer(); +server.on('stream', (stream, headers) => { + console.log(headers[':path']); + console.log(headers.ABC); +}); +``` ### Settings Object @@ -1012,3 +1110,4 @@ TBD [ServerHttp2Stream]: #http2_class_serverhttp2stream [Settings Object]: #http2_settings_object [Using options.selectPadding]: #http2_using_options_selectpadding +[error code]: #error_codes diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 82ca3c8ac6..9771bcc325 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -25,7 +25,8 @@ const { getSettings, getStreamState, mapToHeaders, - NghttpError + NghttpError, + toHeaderObject } = require('internal/http2/util'); const { @@ -57,7 +58,6 @@ const { NGHTTP2_CANCEL, NGHTTP2_DEFAULT_WEIGHT, NGHTTP2_FLAG_END_STREAM, - NGHTTP2_HCAT_REQUEST, NGHTTP2_HCAT_HEADERS, NGHTTP2_HCAT_PUSH_RESPONSE, NGHTTP2_HCAT_RESPONSE, @@ -78,7 +78,6 @@ const { HTTP2_HEADER_PATH, HTTP2_HEADER_SCHEME, HTTP2_HEADER_STATUS, - HTTP2_HEADER_COOKIE, HTTP_STATUS_CONTENT_RESET, HTTP_STATUS_OK, @@ -101,18 +100,8 @@ function onSessionHeaders(id, cat, flags, headers) { const endOfStream = !!(flags & NGHTTP2_FLAG_END_STREAM); let stream = streams.get(id); - // https://tools.ietf.org/html/rfc7540#section-8.1.2.5 - // "...If there are multiple Cookie header fields after decompression, - // these MUST be concatenated into a single octet string using the - // two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ") before being - // passed into a non-HTTP/2 context." - if (Array.isArray(headers[HTTP2_HEADER_COOKIE])) - headers[HTTP2_HEADER_COOKIE] = - headers[HTTP2_HEADER_COOKIE].join('; '); - - if (headers[HTTP2_HEADER_STATUS]) { - headers[HTTP2_HEADER_STATUS] |= 0; - } + // Convert the array of header name value pairs into an object + const obj = toHeaderObject(headers); if (stream === undefined) { switch (owner.type) { @@ -128,16 +117,13 @@ function onSessionHeaders(id, cat, flags, headers) { 'report this as a bug in Node.js'); } streams.set(id, stream); - owner.emit('stream', stream, headers, flags); + owner.emit('stream', stream, obj, flags); } else { let event; let status; switch (cat) { - case NGHTTP2_HCAT_REQUEST: - event = 'request'; - break; case NGHTTP2_HCAT_RESPONSE: - status = headers[HTTP2_HEADER_STATUS]; + status = obj[HTTP2_HEADER_STATUS]; if (!endOfStream && status !== undefined && status >= 100 && @@ -151,7 +137,7 @@ function onSessionHeaders(id, cat, flags, headers) { event = 'push'; break; case NGHTTP2_HCAT_HEADERS: - status = headers[HTTP2_HEADER_STATUS]; + status = obj[HTTP2_HEADER_STATUS]; if (!endOfStream && status !== undefined && status >= 200) { event = 'response'; } else { @@ -164,7 +150,7 @@ function onSessionHeaders(id, cat, flags, headers) { 'report this as a bug in Node.js'); } debug(`emitting stream '${event}' event`); - stream.emit(event, headers, flags); + stream.emit(event, obj, flags); } } @@ -862,6 +848,10 @@ class Http2Session extends EventEmitter { debug('sending shutdown'); submitShutdown.call(this, options); } + + _onTimeout() { + this.emit('timeout'); + } } class ClientHttp2Session extends Http2Session { @@ -1094,6 +1084,10 @@ class Http2Stream extends Duplex { return `Http2Stream ${util.format(obj)}`; } + _onTimeout() { + this.emit('timeout'); + } + get rstCode() { return this._state.rst ? this._state.rstCode : undefined; } @@ -1265,6 +1259,7 @@ class Http2Stream extends Duplex { // Remove the stream from the session const session = this.session; + setImmediate(() => { if (session._handle !== undefined) session._handle.destroyStream(this.id); @@ -1529,6 +1524,7 @@ function socketDestroy(error) { // send data while we close the socket. this.session.destroy(); this.destroy = this[kDestroySocket]; + debug('destroying the socket'); this.destroy(error); } diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index 52b1a43cf5..ceab1abee4 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -18,6 +18,7 @@ const { HTTP2_HEADER_CONTENT_MD5, HTTP2_HEADER_CONTENT_RANGE, HTTP2_HEADER_CONTENT_TYPE, + HTTP2_HEADER_COOKIE, HTTP2_HEADER_DATE, HTTP2_HEADER_ETAG, HTTP2_HEADER_EXPIRES, @@ -326,6 +327,35 @@ function assertWithinRange(name, value, min = 0, max = Infinity) { } } +function toHeaderObject(headers) { + const obj = Object.create(null); + for (var n = 0; n < headers.length; n = n + 2) { + var name = headers[n]; + var value = headers[n + 1]; + if (name === HTTP2_HEADER_STATUS) + value |= 0; + var existing = obj[name]; + if (existing === undefined) { + obj[name] = value; + } else if (!kSingleValueHeaders.has(name)) { + if (name === HTTP2_HEADER_COOKIE) { + // https://tools.ietf.org/html/rfc7540#section-8.1.2.5 + // "...If there are multiple Cookie header fields after decompression, + // these MUST be concatenated into a single octet string using the + // two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ") before + // being passed into a non-HTTP/2 context." + obj[name] = `${existing}; ${value}`; + } else { + if (Array.isArray(existing)) + existing.push(value); + else + obj[name] = [existing, value]; + } + } + } + return obj; +} + module.exports = { assertIsObject, assertValidPseudoHeaderResponse, @@ -336,5 +366,6 @@ module.exports = { getSettings, getStreamState, mapToHeaders, - NghttpError + NghttpError, + toHeaderObject }; diff --git a/src/node_http2.cc b/src/node_http2.cc index af04410c42..9bb0ea7776 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -7,6 +7,7 @@ namespace node { using v8::ArrayBuffer; using v8::Boolean; using v8::Context; +using v8::Function; using v8::Integer; namespace http2 { @@ -759,150 +760,6 @@ void Http2Session::OnTrailers(Nghttp2Stream* stream, } } -static inline bool CheckHeaderAllowsMultiple(nghttp2_vec* name) { - switch (name->len) { - case 3: - if (memcmp(name->base, "age", 3) == 0) - return false; - break; - case 4: - switch (name->base[3]) { - case 'e': - if (memcmp(name->base, "dat", 3) == 0) - return false; - break; - case 'g': - if (memcmp(name->base, "eta", 3) == 0) - return false; - break; - case 'm': - if (memcmp(name->base, "fro", 3) == 0) - return false; - break; - case 't': - if (memcmp(name->base, "hos", 3) == 0) - return false; - break; - } - case 5: - if (memcmp(name->base, "range", 5) == 0) - return false; - break; - case 6: - if (memcmp(name->base, "server", 6) == 0) - return false; - break; - case 7: - switch (name->base[6]) { - case 's': - if (memcmp(name->base, "expire", 6) == 0) - return false; - break; - case 'r': - if (memcmp(name->base, "refere", 6) == 0) - return false; - break; - } - break; - case 8: - switch (name->base[7]) { - case 'e': - if (memcmp(name->base, "if-rang", 7) == 0) - return false; - break; - case 'h': - if (memcmp(name->base, "if-matc", 7) == 0) - return false; - break; - case 'n': - if (memcmp(name->base, "locatio", 7) == 0) - return false; - break; - } - break; - case 10: - if (memcmp(name->base, "user-agent", 10) == 0) - return false; - break; - case 11: - switch (name->base[10]) { - case '5': - if (memcmp(name->base, "content-md", 10) == 0) - return false; - break; - case 'r': - if (memcmp(name->base, "retry-afte", 10) == 0) - return false; - break; - } - break; - case 12: - switch (name->base[11]) { - case 'e': - if (memcmp(name->base, "content-typ", 11) == 0) - return false; - break; - case 's': - if (memcmp(name->base, "max-forward", 11) == 0) - return false; - break; - } - break; - case 13: - switch (name->base[12]) { - case 'd': - if (memcmp(name->base, "last-modifie", 12) == 0) - return false; - break; - case 'h': - if (memcmp(name->base, "if-none-matc", 12) == 0) - return false; - break; - case 'n': - if (memcmp(name->base, "authorizatio", 12) == 0) - return false; - break; - } - break; - case 14: - if (memcmp(name->base, "content-length", 14) == 0) - return false; - break; - case 16: - switch (name->base[15]) { - case 'e': - if (memcmp(name->base, "content-languag", 15) == 0) - return false; - break; - case 'g': - if (memcmp(name->base, "content-encodin", 15) == 0) - return false; - break; - case 'n': - if (memcmp(name->base, "content-locatio", 15) == 0) - return false; - break; - } - break; - case 17: - if (memcmp(name->base, "if-modified-since", 17) == 0) - return false; - break; - case 19: - switch (name->base[18]) { - case 'e': - if (memcmp(name->base, "if-unmodified-sinc", 18) == 0) - return false; - break; - case 'n': - if (memcmp(name->base, "proxy-authorizatio", 18) == 0) - return false; - break; - } - } - return true; -} - void Http2Session::OnHeaders(Nghttp2Stream* stream, nghttp2_header_list* headers, nghttp2_headers_category cat, @@ -912,38 +769,42 @@ void Http2Session::OnHeaders(Nghttp2Stream* stream, Isolate* isolate = env()->isolate(); HandleScope scope(isolate); - Local holder = Object::New(isolate); - holder->SetPrototype(context, v8::Null(isolate)).ToChecked(); Local name_str; Local value_str; - Local array; - while (headers != nullptr) { - nghttp2_header_list* item = headers; - name_str = ExternalHeaderNameResource::New(isolate, item->name); - nghttp2_vec name = nghttp2_rcbuf_get_buf(item->name); - nghttp2_vec value = nghttp2_rcbuf_get_buf(item->value); - value_str = String::NewFromUtf8(isolate, - reinterpret_cast(value.base), - v8::NewStringType::kNormal, - value.len).ToLocalChecked(); - if (holder->Has(context, name_str).FromJust()) { - if (CheckHeaderAllowsMultiple(&name)) { - Local existing = holder->Get(context, name_str).ToLocalChecked(); - if (existing->IsArray()) { - array = existing.As(); - array->Set(context, array->Length(), value_str).FromJust(); - } else { - array = Array::New(isolate, 2); - array->Set(context, 0, existing).FromJust(); - array->Set(context, 1, value_str).FromJust(); - holder->Set(context, name_str, array).FromJust(); - } - } // Ignore singleton headers that appear more than once - } else { - holder->Set(context, name_str, value_str).FromJust(); + + Local holder = Array::New(isolate); + Local fn = env()->push_values_to_array_function(); + Local argv[NODE_PUSH_VAL_TO_ARRAY_MAX * 2]; + + // The headers are passed in above as a linked list of nghttp2_header_list + // structs. The following converts that into a JS array with the structure: + // [name1, value1, name2, value2, name3, value3, name3, value4] and so on. + // That array is passed up to the JS layer and converted into an Object form + // like {name1: value1, name2: value2, name3: [value3, value4]}. We do it + // this way for performance reasons (it's faster to generate and pass an + // array than it is to generate and pass the object). + do { + size_t j = 0; + while (headers != nullptr && j < arraysize(argv) / 2) { + nghttp2_header_list* item = headers; + name_str = ExternalHeaderNameResource::New(isolate, item->name); + nghttp2_vec value = nghttp2_rcbuf_get_buf(item->value); + value_str = String::NewFromUtf8(isolate, + reinterpret_cast(value.base), + v8::NewStringType::kNormal, + value.len).ToLocalChecked(); + argv[j * 2] = name_str; + argv[j * 2 + 1] = value_str; + headers = item->next; + j++; } - headers = item->next; - } + // For performance, we pass name and value pairs to array.protototype.push + // in batches of size NODE_PUSH_VAL_TO_ARRAY_MAX * 2 until there are no + // more items to push. + if (j > 0) { + fn->Call(env()->context(), holder, j * 2, argv).ToLocalChecked(); + } + } while (headers != nullptr); if (object()->Has(context, env()->onheaders_string()).FromJust()) { Local argv[4] = { @@ -1061,7 +922,12 @@ void Http2Session::OnStreamReadImpl(ssize_t nread, return; } uv_buf_t buf[] { uv_buf_init((*bufs).base, nread) }; - session->Write(buf, 1); + ssize_t ret = session->Write(buf, 1); + if (ret < 0) { + DEBUG_HTTP2("Http2Session: fatal error receiving data: %d\n", ret); + nghttp2_session_terminate_session(session->session(), + NGHTTP2_PROTOCOL_ERROR); + } } diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h index 80bf5b4605..75274485c4 100644 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -40,6 +40,8 @@ inline void Nghttp2Session::SubmitShutdownNotice() { } // Sends a SETTINGS frame on the current session +// Note that this *should* send a SETTINGS frame even if niv == 0 and there +// are no settings entries to send. inline int Nghttp2Session::SubmitSettings(const nghttp2_settings_entry iv[], size_t niv) { DEBUG_HTTP2("Nghttp2Session %d: submitting settings, count: %d\n", @@ -59,6 +61,18 @@ inline Nghttp2Stream* Nghttp2Session::FindStream(int32_t id) { } } +inline void Nghttp2Stream::FlushDataChunks() { + while (data_chunks_head_ != nullptr) { + DEBUG_HTTP2("Nghttp2Stream %d: emitting data chunk\n", id_); + nghttp2_data_chunk_t* item = data_chunks_head_; + data_chunks_head_ = item->next; + session_->OnDataChunk(this, item); + delete[] item->buf.base; + data_chunk_free_list.push(item); + } + data_chunks_tail_ = nullptr; +} + // Passes all of the the chunks for a data frame out to the JS layer // The chunks are collected as the frame is being processed and sent out // to the JS side only when the frame is fully processed. @@ -69,17 +83,7 @@ inline void Nghttp2Session::HandleDataFrame(const nghttp2_frame* frame) { Nghttp2Stream* stream = this->FindStream(id); // If the stream does not exist, something really bad happened CHECK_NE(stream, nullptr); - - while (stream->data_chunks_head_ != nullptr) { - DEBUG_HTTP2("Nghttp2Session %d: emitting data chunk for stream %d\n", - session_type_, id); - nghttp2_data_chunk_t* item = stream->data_chunks_head_; - stream->data_chunks_head_ = item->next; - OnDataChunk(stream, item); - delete[] item->buf.base; - data_chunk_free_list.push(item); - } - stream->data_chunks_tail_ = nullptr; + stream->FlushDataChunks(); } // Passes all of the collected headers for a HEADERS frame out to the JS layer. @@ -292,7 +296,7 @@ inline void Nghttp2Stream::ResetState( inline void Nghttp2Stream::Destroy() { - DEBUG_HTTP2("Nghttp2Stream %d: destroying stream\n", id_); + DEBUG_HTTP2("Nghttp2Stream %d: destroying stream\n", id_); // Do nothing if this stream instance is already destroyed if (IsDestroyed() || IsDestroying()) return; @@ -305,7 +309,7 @@ inline void Nghttp2Stream::Destroy() { session_ = nullptr; } - // Free any remaining data chunks. + // Free any remaining incoming data chunks. while (data_chunks_head_ != nullptr) { nghttp2_data_chunk_t* chunk = data_chunks_head_; data_chunks_head_ = chunk->next; @@ -314,7 +318,16 @@ inline void Nghttp2Stream::Destroy() { } data_chunks_tail_ = nullptr; - // Free any remainingin headers + // Free any remaining outgoing data chunks. + while (queue_head_ != nullptr) { + nghttp2_stream_write_queue* head = queue_head_; + queue_head_ = head->next; + head->cb(head->req, UV_ECANCELED); + delete head; + } + queue_tail_ = nullptr; + + // Free any remaining headers FreeHeaders(); // Return this stream instance to the freelist @@ -337,6 +350,7 @@ inline void Nghttp2Stream::FreeHeaders() { inline int Nghttp2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) { DEBUG_HTTP2("Nghttp2Stream %d: sending informational headers, count: %d\n", id_, len); + CHECK_GT(len, 0); return nghttp2_submit_headers(session_->session(), NGHTTP2_FLAG_NONE, id_, nullptr, @@ -364,12 +378,13 @@ inline int Nghttp2Stream::SubmitRstStream(const uint32_t code) { code); } -// Submit a push promise +// Submit a push promise. inline int32_t Nghttp2Stream::SubmitPushPromise( nghttp2_nv* nva, size_t len, Nghttp2Stream** assigned, bool emptyPayload) { + CHECK_GT(len, 0); DEBUG_HTTP2("Nghttp2Stream %d: sending push promise\n", id_); int32_t ret = nghttp2_submit_push_promise(session_->session(), NGHTTP2_FLAG_NONE, @@ -390,6 +405,7 @@ inline int32_t Nghttp2Stream::SubmitPushPromise( inline int Nghttp2Stream::SubmitResponse(nghttp2_nv* nva, size_t len, bool emptyPayload) { + CHECK_GT(len, 0); DEBUG_HTTP2("Nghttp2Stream %d: submitting response\n", id_); nghttp2_data_provider* provider = nullptr; nghttp2_data_provider prov; @@ -412,6 +428,7 @@ inline int32_t Nghttp2Session::SubmitRequest( size_t len, Nghttp2Stream** assigned, bool emptyPayload) { + CHECK_GT(len, 0); DEBUG_HTTP2("Nghttp2Session: submitting request\n"); nghttp2_data_provider* provider = nullptr; nghttp2_data_provider prov; @@ -468,6 +485,9 @@ inline int Nghttp2Stream::Write(nghttp2_stream_write_t* req, } inline void Nghttp2Stream::ReadStart() { + // Has no effect if IsReading() is true. + if (IsReading()) + return; DEBUG_HTTP2("Nghttp2Stream %d: start reading\n", id_); if (IsPaused()) { // If handle->reading is less than zero, read_start had never previously @@ -482,7 +502,9 @@ inline void Nghttp2Stream::ReadStart() { } flags_ |= NGHTTP2_STREAM_READ_START; flags_ &= ~NGHTTP2_STREAM_READ_PAUSED; - // TODO(jasnell): Drain the queued data chunks... + + // Flush any queued data chunks immediately out to the JS layer + FlushDataChunks(); } inline void Nghttp2Stream::ReadStop() { @@ -524,6 +546,14 @@ Nghttp2Session::Callbacks::Callbacks(bool kHasGetPaddingCallback) { nghttp2_session_callbacks_set_on_data_chunk_recv_callback( callbacks, OnDataChunkReceived); + // nghttp2_session_callbacks_set_on_invalid_frame_recv( + // callbacks, OnInvalidFrameReceived); + +#ifdef NODE_DEBUG_HTTP2 + nghttp2_session_callbacks_set_error_callback( + callbacks, OnNghttpError); +#endif + if (kHasGetPaddingCallback) { nghttp2_session_callbacks_set_select_padding_callback( callbacks, OnSelectPadding); diff --git a/src/node_http2_core.cc b/src/node_http2_core.cc index 55460fc98f..164def1267 100644 --- a/src/node_http2_core.cc +++ b/src/node_http2_core.cc @@ -3,6 +3,18 @@ namespace node { namespace http2 { +#ifdef NODE_DEBUG_HTTP2 +int Nghttp2Session::OnNghttpError(nghttp2_session* session, + const char* message, + size_t len, + void* user_data) { + Nghttp2Session* handle = static_cast(user_data); + DEBUG_HTTP2("Nghttp2Session %d: Error '%.*s'\n", + handle->session_type_, len, message); + return 0; +} +#endif + // nghttp2 calls this at the beginning a new HEADERS or PUSH_PROMISE frame. // We use it to ensure that an Nghttp2Stream instance is allocated to store // the state. @@ -15,6 +27,7 @@ int Nghttp2Session::OnBeginHeadersCallback(nghttp2_session* session, frame->hd.stream_id; DEBUG_HTTP2("Nghttp2Session %d: beginning headers for stream %d\n", handle->session_type_, id); + Nghttp2Stream* stream = handle->FindStream(id); if (stream == nullptr) { Nghttp2Stream::Init(id, handle, frame->headers.cat); @@ -149,6 +162,9 @@ ssize_t Nghttp2Session::OnStreamRead(nghttp2_session* session, size_t remaining = length; size_t offset = 0; + // While there is data in the queue, copy data into buf until it is full. + // There may be data left over, which will be sent the next time nghttp + // calls this callback. while (stream->queue_head_ != nullptr) { DEBUG_HTTP2("Nghttp2Session %d: processing outbound data chunk\n", handle->session_type_); @@ -183,6 +199,13 @@ ssize_t Nghttp2Session::OnStreamRead(nghttp2_session* session, stream->queue_tail_ = nullptr; end: + // If we are no longer writable and there is no more data in the queue, + // then we need to set the NGHTTP2_DATA_FLAG_EOF flag. + // If we are still writable but there is not yet any data to send, set the + // NGHTTP2_ERR_DEFERRED flag. This will put the stream into a pending state + // that will wait for data to become available. + // If neither of these flags are set, then nghttp2 will call this callback + // again to get the data for the next DATA frame. int writable = stream->queue_head_ != nullptr || stream->IsWritable(); if (offset == 0 && writable && stream->queue_head_ == nullptr) { DEBUG_HTTP2("Nghttp2Session %d: deferring stream %d\n", @@ -194,6 +217,10 @@ ssize_t Nghttp2Session::OnStreamRead(nghttp2_session* session, handle->session_type_, id); *flags |= NGHTTP2_DATA_FLAG_EOF; + // Only when we are done sending the last chunk of data do we check for + // any trailing headers that are to be sent. This is the only opportunity + // we have to make this check. If there are trailers, then the + // NGHTTP2_DATA_FLAG_NO_END_STREAM flag must be set. MaybeStackBuffer trailers; handle->OnTrailers(stream, &trailers); if (trailers.length() > 0) { diff --git a/src/node_http2_core.h b/src/node_http2_core.h index f4d0372868..cd21620696 100644 --- a/src/node_http2_core.h +++ b/src/node_http2_core.h @@ -167,6 +167,13 @@ class Nghttp2Session { inline void HandleDataFrame(const nghttp2_frame* frame); /* callbacks for nghttp2 */ +#ifdef NODE_DEBUG_HTTP2 + static int OnNghttpError(nghttp2_session* session, + const char* message, + size_t len, + void* user_data); +#endif + static int OnBeginHeadersCallback(nghttp2_session* session, const nghttp2_frame* frame, void* user_data); @@ -238,9 +245,11 @@ class Nghttp2Stream { CHECK_EQ(data_chunks_tail_, nullptr); CHECK_EQ(current_headers_head_, nullptr); CHECK_EQ(current_headers_tail_, nullptr); - DEBUG_HTTP2("Nghttp2Stream: freed\n"); + DEBUG_HTTP2("Nghttp2Stream %d: freed\n", id_); } + inline void FlushDataChunks(); + // Resets the state of the stream instance to defaults inline void ResetState( int32_t id, diff --git a/test/parallel/test-http2-timeouts.js b/test/parallel/test-http2-timeouts.js new file mode 100644 index 0000000000..2262472a41 --- /dev/null +++ b/test/parallel/test-http2-timeouts.js @@ -0,0 +1,31 @@ +'use strict'; + +const common = require('../common'); +const h2 = require('http2'); + +const server = h2.createServer(); + +// we use the lower-level API here +server.on('stream', common.mustCall((stream) => { + stream.setTimeout(1, common.mustCall(() => { + stream.respond({':status': 200}); + stream.end('hello world'); + })); +})); +server.listen(0); + +server.on('listening', common.mustCall(() => { + const client = h2.connect(`http://localhost:${server.address().port}`); + client.setTimeout(1, common.mustCall(() => { + const req = client.request({ ':path': '/' }); + req.setTimeout(1, common.mustCall(() => { + req.on('response', common.mustCall()); + req.resume(); + req.on('end', common.mustCall(() => { + server.close(); + client.destroy(); + })); + req.end(); + })); + })); +}));