From 0babd181a0c5d775e62a12b3b04fe4d7654fe80a Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 12 Dec 2017 11:34:17 -0800 Subject: [PATCH] http2: cleanup Http2Stream/Http2Session destroy PR-URL: https://github.com/nodejs/node/pull/17406 Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Anatoli Papirovski This is a significant cleanup and refactoring of the cleanup/close/destroy logic for Http2Stream and Http2Session. There are significant changes here in the timing and ordering of cleanup logic, JS apis. and various related necessary edits. --- doc/api/errors.md | 23 +- doc/api/http2.md | 310 ++-- lib/internal/errors.js | 7 +- lib/internal/http2/compat.js | 32 +- lib/internal/http2/core.js | 1649 +++++++++-------- src/node_http2.cc | 650 +++++-- src/node_http2.h | 42 +- test/parallel/test-http2-client-data-end.js | 45 +- test/parallel/test-http2-client-destroy.js | 212 +-- .../test-http2-client-http1-server.js | 11 +- .../test-http2-client-onconnect-errors.js | 19 +- test/parallel/test-http2-client-port-80.js | 8 +- ...st-http2-client-priority-before-connect.js | 28 +- .../test-http2-client-promisify-connect.js | 3 +- ...est-http2-client-request-options-errors.js | 44 +- ...t-http2-client-rststream-before-connect.js | 26 +- .../test-http2-client-set-priority.js | 20 +- ...st-http2-client-settings-before-connect.js | 75 +- ...st-http2-client-shutdown-before-connect.js | 12 +- .../test-http2-client-socket-destroy.js | 31 +- ...p2-client-stream-destroy-before-connect.js | 23 +- .../test-http2-client-unescaped-path.js | 15 +- test/parallel/test-http2-client-upload.js | 20 +- .../test-http2-client-write-before-connect.js | 37 +- test/parallel/test-http2-compat-errors.js | 26 +- ...test-http2-compat-expect-continue-check.js | 47 +- .../test-http2-compat-expect-continue.js | 17 +- .../test-http2-compat-expect-handling.js | 2 +- .../test-http2-compat-method-connect.js | 2 +- .../test-http2-compat-serverrequest-end.js | 15 +- ...test-http2-compat-serverrequest-headers.js | 2 +- .../test-http2-compat-serverrequest-pause.js | 2 +- .../test-http2-compat-serverrequest-pipe.js | 2 +- ...t-http2-compat-serverrequest-settimeout.js | 3 +- ...est-http2-compat-serverrequest-trailers.js | 2 +- .../test-http2-compat-serverrequest.js | 2 +- .../test-http2-compat-serverresponse-close.js | 19 +- ...ompat-serverresponse-createpushresponse.js | 4 +- ...est-http2-compat-serverresponse-destroy.js | 86 +- .../test-http2-compat-serverresponse-drain.js | 2 +- .../test-http2-compat-serverresponse-end.js | 18 +- ...st-http2-compat-serverresponse-finished.js | 2 +- ...ttp2-compat-serverresponse-flushheaders.js | 2 +- ...at-serverresponse-headers-after-destroy.js | 6 +- ...est-http2-compat-serverresponse-headers.js | 2 +- ...-http2-compat-serverresponse-settimeout.js | 3 +- ...-http2-compat-serverresponse-statuscode.js | 2 +- ...rverresponse-statusmessage-property-set.js | 2 +- ...t-serverresponse-statusmessage-property.js | 2 +- ...tp2-compat-serverresponse-statusmessage.js | 2 +- ...st-http2-compat-serverresponse-trailers.js | 2 +- ...http2-compat-serverresponse-write-no-cb.js | 65 +- ...t-http2-compat-serverresponse-writehead.js | 4 +- test/parallel/test-http2-compat-socket-set.js | 2 +- test/parallel/test-http2-compat-socket.js | 2 +- test/parallel/test-http2-connect-method.js | 9 +- test/parallel/test-http2-connect.js | 8 +- test/parallel/test-http2-cookies.js | 2 +- .../test-http2-create-client-connect.js | 12 +- .../test-http2-create-client-session.js | 26 +- ...test-http2-createsecureserver-nooptions.js | 8 +- test/parallel/test-http2-createwritereq.js | 2 +- test/parallel/test-http2-date-header.js | 2 +- test/parallel/test-http2-dont-lose-data.js | 58 + test/parallel/test-http2-dont-override.js | 2 +- .../test-http2-generic-streams-sendfile.js | 6 +- test/parallel/test-http2-goaway-opaquedata.js | 23 +- test/parallel/test-http2-head-request.js | 2 +- test/parallel/test-http2-https-fallback.js | 2 +- .../test-http2-info-headers-errors.js | 27 +- test/parallel/test-http2-info-headers.js | 2 +- .../test-http2-invalidargtypes-errors.js | 44 +- .../test-http2-max-concurrent-streams.js | 72 +- test/parallel/test-http2-methods.js | 2 +- ...t-http2-misbehaving-flow-control-paused.js | 20 +- .../test-http2-misbehaving-flow-control.js | 36 +- .../test-http2-misused-pseudoheaders.js | 31 +- .../test-http2-multi-content-length.js | 41 +- test/parallel/test-http2-multiheaders-raw.js | 2 +- test/parallel/test-http2-multiheaders.js | 2 +- test/parallel/test-http2-multiplex.js | 16 +- test/parallel/test-http2-no-more-streams.js | 2 +- ...-http2-options-max-headers-block-length.js | 41 +- ...test-http2-options-max-reserved-streams.js | 55 +- test/parallel/test-http2-padding-callback.js | 2 +- test/parallel/test-http2-ping.js | 2 +- test/parallel/test-http2-pipe.js | 31 +- test/parallel/test-http2-priority-event.js | 2 +- test/parallel/test-http2-respond-errors.js | 7 +- test/parallel/test-http2-respond-file-204.js | 2 +- test/parallel/test-http2-respond-file-304.js | 2 +- test/parallel/test-http2-respond-file-404.js | 2 +- .../test-http2-respond-file-compat.js | 2 +- .../test-http2-respond-file-error-dir.js | 8 +- .../test-http2-respond-file-errors.js | 40 +- .../test-http2-respond-file-fd-errors.js | 43 +- .../test-http2-respond-file-fd-invalid.js | 2 +- .../test-http2-respond-file-fd-range.js | 23 +- test/parallel/test-http2-respond-file-fd.js | 2 +- test/parallel/test-http2-respond-file-push.js | 5 +- .../parallel/test-http2-respond-file-range.js | 2 +- test/parallel/test-http2-respond-file.js | 2 +- test/parallel/test-http2-respond-no-data.js | 2 +- .../test-http2-respond-with-fd-errors.js | 8 +- .../parallel/test-http2-response-splitting.js | 2 +- test/parallel/test-http2-rststream-errors.js | 94 - test/parallel/test-http2-serve-file.js | 2 +- test/parallel/test-http2-server-errors.js | 11 - .../test-http2-server-http1-client.js | 9 +- .../test-http2-server-push-disabled.js | 2 +- ...st-http2-server-push-stream-errors-args.js | 2 +- .../test-http2-server-push-stream-errors.js | 41 +- .../test-http2-server-push-stream-head.js | 23 +- .../parallel/test-http2-server-push-stream.js | 5 +- .../test-http2-server-rst-before-respond.js | 17 +- test/parallel/test-http2-server-rst-stream.js | 23 +- .../test-http2-server-sessionerror.js | 48 + test/parallel/test-http2-server-set-header.js | 2 +- ...st-http2-server-shutdown-before-respond.js | 16 +- ...st-http2-server-shutdown-options-errors.js | 90 +- .../test-http2-server-shutdown-redundant.js | 29 +- .../test-http2-server-socket-destroy.js | 30 +- .../parallel/test-http2-server-socketerror.js | 56 - ...est-http2-server-stream-session-destroy.js | 91 +- test/parallel/test-http2-server-timeout.js | 10 +- test/parallel/test-http2-session-settings.js | 105 +- .../test-http2-session-stream-state.js | 4 +- test/parallel/test-http2-shutdown-errors.js | 75 - test/parallel/test-http2-single-headers.js | 47 +- test/parallel/test-http2-socket-proxy.js | 13 +- .../test-http2-status-code-invalid.js | 2 +- test/parallel/test-http2-status-code.js | 2 +- test/parallel/test-http2-stream-client.js | 2 +- .../test-http2-stream-destroy-event-order.js | 15 +- test/parallel/test-http2-timeouts.js | 2 +- test/parallel/test-http2-too-large-headers.js | 2 +- test/parallel/test-http2-too-many-headers.js | 2 +- test/parallel/test-http2-too-many-settings.js | 4 +- test/parallel/test-http2-trailers.js | 2 +- test/parallel/test-http2-window-size.js | 2 +- test/parallel/test-http2-write-callbacks.js | 2 +- .../parallel/test-http2-write-empty-string.js | 2 +- test/parallel/test-http2-zero-length-write.js | 3 +- test/sequential/test-http2-session-timeout.js | 2 +- .../test-http2-timeout-large-write-file.js | 2 +- .../test-http2-timeout-large-write.js | 2 +- 146 files changed, 2638 insertions(+), 2692 deletions(-) create mode 100644 test/parallel/test-http2-dont-lose-data.js delete mode 100644 test/parallel/test-http2-rststream-errors.js create mode 100644 test/parallel/test-http2-server-sessionerror.js delete mode 100644 test/parallel/test-http2-server-socketerror.js delete mode 100644 test/parallel/test-http2-shutdown-errors.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 776639d2e4cb3f..918cfa343136af 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -810,6 +810,11 @@ Status code was outside the regular status code range (100-999). The `Trailer` header was set even though the transfer encoding does not support that. + +### ERR_HTTP2_ALREADY_SHUTDOWN + +Occurs with multiple attempts to shutdown an HTTP/2 session. + ### ERR_HTTP2_CONNECT_AUTHORITY @@ -833,6 +838,12 @@ forbidden. A failure occurred sending an individual frame on the HTTP/2 session. + +### ERR_HTTP2_GOAWAY_SESSION + +New HTTP/2 Streams may not be opened after the `Http2Session` has received a +`GOAWAY` frame from the connected peer. + ### ERR_HTTP2_HEADER_REQUIRED @@ -972,6 +983,11 @@ client. An attempt was made to use the `Http2Stream.prototype.responseWithFile()` API to send something other than a regular file. + +### ERR_HTTP2_SESSION_ERROR + +The `Http2Session` closed with a non-zero error code. + ### ERR_HTTP2_SOCKET_BOUND @@ -989,10 +1005,11 @@ Use of the `101` Informational status code is forbidden in HTTP/2. An invalid HTTP status code has been specified. Status codes must be an integer between `100` and `599` (inclusive). - -### ERR_HTTP2_STREAM_CLOSED + +### ERR_HTTP2_STREAM_CANCEL -An action was performed on an HTTP/2 Stream that had already been closed. +An `Http2Stream` was destroyed before any data was transmitted to the connected +peer. ### ERR_HTTP2_STREAM_ERROR diff --git a/doc/api/http2.md b/doc/api/http2.md index 6b92f76a1becf9..8facdf465b2db7 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -67,8 +67,8 @@ const fs = require('fs'); const client = http2.connect('https://localhost:8443', { ca: fs.readFileSync('localhost-cert.pem') }); -client.on('socketError', (err) => console.error(err)); client.on('error', (err) => console.error(err)); +client.on('socketError', (err) => console.error(err)); const req = client.request({ ':path': '/' }); @@ -83,7 +83,7 @@ let data = ''; req.on('data', (chunk) => { data += chunk; }); req.on('end', () => { console.log(`\n${data}`); - client.destroy(); + client.close(); }); req.end(); ``` @@ -127,7 +127,7 @@ solely on the API of the `Http2Session`. added: v8.4.0 --> -The `'close'` event is emitted once the `Http2Session` has been terminated. +The `'close'` event is emitted once the `Http2Session` has been destroyed. #### Event: 'connect' - -The `'socketError'` event is emitted when an `'error'` is emitted on the -`Socket` instance bound to the `Http2Session`. If this event is not handled, -the `'error'` event will be re-emitted on the `Socket`. - -For `ServerHttp2Session` instances, a `'socketError'` event listener is always -registered that will, by default, forward the event on to the owning -`Http2Server` instance if no additional handlers are registered. - #### Event: 'timeout' + +* `callback` {Function} + +Gracefully closes the `Http2Session`, allowing any existing streams to +complete on their own and preventing new `Http2Stream` instances from being +created. Once closed, `http2session.destroy()` *might* be called if there +are no open `Http2Stream` instances. + +If specified, the `callback` function is registered as a handler for the +`'close'` event. + +#### http2session.closed + + +* Value: {boolean} + +Will be `true` if this `Http2Session` instance has been closed, otherwise +`false`. + +#### http2session.destroy([error,][code]) +* `error` {Error} An `Error` object if the `Http2Session` is being destroyed + due to an error. +* `code` {number} The HTTP/2 error code to send in the final `GOAWAY` frame. + If unspecified, and `error` is not undefined, the default is `INTERNAL_ERROR`, + otherwise defaults to `NO_ERROR`. * Returns: {undefined} Immediately terminates the `Http2Session` and the associated `net.Socket` or `tls.TLSSocket`. +Once destroyed, the `Http2Session` will emit the `'close'` event. If `error` +is not undefined, an `'error'` event will be emitted immediately after the +`'close'` event. + +If there are any remaining open `Http2Streams` associatd with the +`Http2Session`, those will also be destroyed. + #### http2session.destroyed + +* `code` {number} An HTTP/2 error code +* `lastStreamID` {number} The numeric ID of the last processed `Http2Stream` +* `opaqueData` {Buffer|TypedArray|DataView} A `TypedArray` or `DataView` + instance containing additional data to be carried within the GOAWAY frame. + +Transmits a `GOAWAY` frame to the connected peer *without* shutting down the +`Http2Session`. + #### http2session.localSettings * `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. **Default:** `0x00` (No Error). * `lastStreamID` {number} The Stream ID of the last successfully processed - `Http2Stream` on this `Http2Session`. + `Http2Stream` on this `Http2Session`. If unspecified, will default to the + ID of the most recently received stream. * `opaqueData` {Buffer|Uint8Array} A `Buffer` or `Uint8Array` instance containing arbitrary additional data to send to the peer upon disconnection. This is used, typically, to provide additional data for debugging failures, @@ -487,19 +523,16 @@ 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. +If the `Http2Session` instance is a server-side session and the `errorCode` +option is `0x00` (No Error), a "graceful" shutdown will be initiated. During a +"graceful" shutdown, the session will first send a `GOAWAY` frame to +the connected peer identifying the last processed stream as 231-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, +session.close({ opaqueData: Buffer.from('add some debugging data here') }, () => session.destroy()); ``` @@ -627,7 +660,7 @@ is not yet ready for use. All [`Http2Stream`][] instances are destroyed either when: * An `RST_STREAM` frame for the stream is received by the connected peer. -* The `http2stream.rstStream()` methods is called. +* The `http2stream.close()` method is called. * The `http2stream.destroy()` or `http2session.destroy()` methods are called. When an `Http2Stream` instance is destroyed, an attempt will be made to send an @@ -720,6 +753,29 @@ added: v8.4.0 Set to `true` if the `Http2Stream` instance was aborted abnormally. When set, the `'aborted'` event will have been emitted. +#### http2stream.close(code[, callback]) + + +* code {number} Unsigned 32-bit integer identifying the error code. **Default:** + `http2.constant.NGHTTP2_NO_ERROR` (`0x00`) +* `callback` {Function} An optional function registered to listen for the + `'close'` event. +* Returns: {undefined} + +Closes the `Http2Stream` instance by sending an `RST_STREAM` frame to the +connected HTTP/2 peer. + +#### http2stream.closed + + +* Value: {boolean} + +Set to `true` if the `Http2Stream` instance has been closed. + #### http2stream.destroyed + +* Value: {boolean} + +Set to `true` if the `Http2Stream` instance has not yet been assigned a +numeric stream identifier. + #### http2stream.priority(options) - -* code {number} Unsigned 32-bit integer identifying the error code. **Default:** - `http2.constant.NGHTTP2_NO_ERROR` (`0x00`) -* Returns: {undefined} - -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.rstWithNoError() - - -* Returns: {undefined} - -Shortcut for `http2stream.rstStream()` using error code `0x00` (No Error). - -#### http2stream.rstWithProtocolError() - - -* Returns: {undefined} - -Shortcut for `http2stream.rstStream()` using error code `0x01` (Protocol Error). - -#### http2stream.rstWithCancel() - - -* Returns: {undefined} - -Shortcut for `http2stream.rstStream()` using error code `0x08` (Cancel). - -#### http2stream.rstWithRefuse() - - -* Returns: {undefined} - -Shortcut for `http2stream.rstStream()` using error code `0x07` (Refused Stream). - -#### http2stream.rstWithInternalError() - - -* Returns: {undefined} - -Shortcut for `http2stream.rstStream()` using error code `0x02` (Internal Error). - #### http2stream.session + +* `request` {http2.Http2ServerRequest} +* `response` {http2.Http2ServerResponse} + +If a [`'request'`][] listener is registered or [`http2.createServer()`][] is +supplied a callback function, the `'checkContinue'` event is emitted each time +a request with an HTTP `Expect: 100-continue` is received. If this event is +not listened for, the server will automatically respond with a status +`100 Continue` as appropriate. -#### Event: 'socketError' +Handling this event involves calling [`response.writeContinue()`][] if the client +should continue to send the request body, or generating an appropriate HTTP +response (e.g. 400 Bad Request) if the client should not continue to send the +request body. + +Note that when this event is emitted and handled, the [`'request'`][] event will +not be emitted. + +#### Event: 'request' -The `'socketError'` event is emitted when a `'socketError'` event is emitted by -an `Http2Session` associated with the server. +* `request` {http2.Http2ServerRequest} +* `response` {http2.Http2ServerResponse} + +Emitted each time there is a request. Note that there may be multiple requests +per session. See the [Compatibility API][]. + +#### Event: 'session' + + +The `'session'` event is emitted when a new `Http2Session` is created by the +`Http2Server`. #### Event: 'sessionError' The `'sessionError'` event is emitted when an `'error'` event is emitted by -an `Http2Session` object. If no listener is registered for this event, an -`'error'` event is emitted. +an `Http2Session` object associated with the `Http2Server`. #### Event: 'streamError' -* `socket` {http2.ServerHttp2Stream} - If an `ServerHttp2Stream` emits an `'error'` event, it will be forwarded here. The stream will already be destroyed when this event is triggered. @@ -1325,17 +1364,6 @@ server.on('stream', (stream, headers, flags) => { }); ``` -#### Event: 'request' - - -* `request` {http2.Http2ServerRequest} -* `response` {http2.Http2ServerResponse} - -Emitted each time there is a request. Note that there may be multiple requests -per session. See the [Compatibility API][]. - #### Event: 'timeout' - -* `request` {http2.Http2ServerRequest} -* `response` {http2.Http2ServerResponse} - -If a [`'request'`][] listener is registered or [`http2.createServer()`][] is -supplied a callback function, the `'checkContinue'` event is emitted each time -a request with an HTTP `Expect: 100-continue` is received. If this event is -not listened for, the server will automatically respond with a status -`100 Continue` as appropriate. - -Handling this event involves calling [`response.writeContinue()`][] if the client -should continue to send the request body, or generating an appropriate HTTP -response (e.g. 400 Bad Request) if the client should not continue to send the -request body. - -Note that when this event is emitted and handled, the [`'request'`][] event will -not be emitted. - ### Class: Http2SecureServer The `'sessionError'` event is emitted when an `'error'` event is emitted by -an `Http2Session` object. If no listener is registered for this event, an -`'error'` event is emitted on the `Http2Session` instance instead. - -#### Event: 'socketError' - - -The `'socketError'` event is emitted when a `'socketError'` event is emitted by -an `Http2Session` associated with the server. +an `Http2Session` object associated with the `Http2SecureServer`. #### Event: 'unknownProtocol'