From 1724a4e4fe658959a805f605c43f9d5344cbe7e6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 21 Nov 2017 12:38:27 +0100 Subject: [PATCH 01/27] src: add optional keep-alive object to SetImmediate Adds the possibility to keep a strong persistent reference to a JS object while a `SetImmediate()` call is in effect. PR-URL: https://github.com/nodejs/node/pull/17183 Reviewed-By: James M Snell Reviewed-By: Franziska Hinkelmann Reviewed-By: Anatoli Papirovski --- src/env-inl.h | 11 +++++++++-- src/env.cc | 2 ++ src/env.h | 7 ++++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index b627cc159eafd9..955cf2688f88fa 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -506,8 +506,15 @@ Environment::scheduled_immediate_count() { return scheduled_immediate_count_; } -void Environment::SetImmediate(native_immediate_callback cb, void* data) { - native_immediate_callbacks_.push_back({ cb, data }); +void Environment::SetImmediate(native_immediate_callback cb, + void* data, + v8::Local obj) { + native_immediate_callbacks_.push_back({ + cb, + data, + std::unique_ptr>( + obj.IsEmpty() ? nullptr : new v8::Persistent(isolate_, obj)) + }); if (scheduled_immediate_count_[0] == 0) ActivateImmediateCheck(); scheduled_immediate_count_[0] = scheduled_immediate_count_[0] + 1; diff --git a/src/env.cc b/src/env.cc index 8f8255819b4a28..64fc2dea04e8d1 100644 --- a/src/env.cc +++ b/src/env.cc @@ -276,6 +276,8 @@ void Environment::RunAndClearNativeImmediates() { native_immediate_callbacks_.swap(list); for (const auto& cb : list) { cb.cb_(this, cb.data_); + if (cb.keep_alive_) + cb.keep_alive_->Reset(); } #ifdef DEBUG diff --git a/src/env.h b/src/env.h index 99491c5dbd11eb..9414e357162d93 100644 --- a/src/env.h +++ b/src/env.h @@ -661,7 +661,11 @@ class Environment { bool EmitNapiWarning(); typedef void (*native_immediate_callback)(Environment* env, void* data); - inline void SetImmediate(native_immediate_callback cb, void* data); + // cb will be called as cb(env, data) on the next event loop iteration. + // obj will be kept alive between now and after the callback has run. + inline void SetImmediate(native_immediate_callback cb, + void* data, + v8::Local obj = v8::Local()); // This needs to be available for the JS-land setImmediate(). void ActivateImmediateCheck(); @@ -741,6 +745,7 @@ class Environment { struct NativeImmediateCallback { native_immediate_callback cb_; void* data_; + std::unique_ptr> keep_alive_; }; std::vector native_immediate_callbacks_; void RunAndClearNativeImmediates(); From df3113341239c1e73ccb6723e2b826e593b5b82d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 21 Nov 2017 12:39:55 +0100 Subject: [PATCH 02/27] http2: don't call into JS from GC Calling into JS land from GC is not allowed, so delay the resolution of pending pings when a session is destroyed. PR-URL: https://github.com/nodejs/node/pull/17183 Reviewed-By: James M Snell Reviewed-By: Franziska Hinkelmann Reviewed-By: Anatoli Papirovski --- src/node_http2.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 0747789786b028..2ed5cbb4a3dc59 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -406,7 +406,11 @@ void Http2Session::Close() { while (!outstanding_pings_.empty()) { Http2Session::Http2Ping* ping = PopPing(); - ping->Done(false); + // Since this method may be called from GC, calling into JS directly + // is not allowed. + env()->SetImmediate([](Environment* env, void* data) { + static_cast(data)->Done(false); + }, static_cast(ping)); } Stop(); From 8c28e30d94e96f99eaf18bd4f4f0650734009cf1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 21 Nov 2017 12:42:54 +0100 Subject: [PATCH 03/27] http2: only schedule write when necessary Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle. PR-URL: https://github.com/nodejs/node/pull/17183 Reviewed-By: James M Snell Reviewed-By: Franziska Hinkelmann Reviewed-By: Anatoli Papirovski --- src/node_http2.cc | 101 +++++++++++------- src/node_http2.h | 22 +++- ...-http2-session-gc-while-write-scheduled.js | 32 ++++++ 3 files changed, 114 insertions(+), 41 deletions(-) create mode 100644 test/parallel/test-http2-session-gc-while-write-scheduled.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 2ed5cbb4a3dc59..f8b530c20a16cf 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -27,6 +27,26 @@ const Http2Session::Callbacks Http2Session::callback_struct_saved[2] = { Callbacks(false), Callbacks(true)}; +Http2Scope::Http2Scope(Http2Stream* stream) : Http2Scope(stream->session()) {} + +Http2Scope::Http2Scope(Http2Session* session) { + if (session->flags_ & (SESSION_STATE_HAS_SCOPE | + SESSION_STATE_WRITE_SCHEDULED)) { + // There is another scope further below on the stack, or it is already + // known that a write is scheduled. In either case, there is nothing to do. + return; + } + session->flags_ |= SESSION_STATE_HAS_SCOPE; + session_ = session; +} + +Http2Scope::~Http2Scope() { + if (session_ == nullptr) + return; + + session_->flags_ &= ~SESSION_STATE_HAS_SCOPE; + session_->MaybeScheduleWrite(); +} Http2Options::Http2Options(Environment* env) { nghttp2_option_new(&options_); @@ -346,8 +366,6 @@ Http2Session::Http2Session(Environment* env, // be catching before it gets this far. Either way, crash if this // fails. CHECK_EQ(fn(&session_, callbacks, this, *opts), 0); - - Start(); } @@ -356,40 +374,6 @@ Http2Session::~Http2Session() { Close(); } -// For every node::Http2Session instance, there is a uv_prepare_t handle -// whose callback is triggered on every tick of the event loop. When -// run, nghttp2 is prompted to send any queued data it may have stored. -// TODO(jasnell): Currently, this creates one uv_prepare_t per Http2Session, -// we should investigate to see if it's faster to create a -// single uv_prepare_t for all Http2Sessions, then iterate -// over each. -void Http2Session::Start() { - prep_ = new uv_prepare_t(); - uv_prepare_init(env()->event_loop(), prep_); - prep_->data = static_cast(this); - uv_prepare_start(prep_, [](uv_prepare_t* t) { - Http2Session* session = static_cast(t->data); - HandleScope scope(session->env()->isolate()); - Context::Scope context_scope(session->env()->context()); - - // Sending data may call arbitrary JS code, so keep track of - // async context. - InternalCallbackScope callback_scope(session); - session->SendPendingData(); - }); -} - -// Stop the uv_prep_t from further activity, destroy the handle -void Http2Session::Stop() { - DEBUG_HTTP2SESSION(this, "stopping uv_prep_t handle"); - CHECK_EQ(uv_prepare_stop(prep_), 0); - auto prep_close = [](uv_handle_t* handle) { - delete reinterpret_cast(handle); - }; - uv_close(reinterpret_cast(prep_), prep_close); - prep_ = nullptr; -} - void Http2Session::Close() { DEBUG_HTTP2SESSION(this, "closing session"); @@ -412,8 +396,6 @@ void Http2Session::Close() { static_cast(data)->Done(false); }, static_cast(ping)); } - - Stop(); } @@ -484,6 +466,7 @@ inline void Http2Session::SubmitShutdownNotice() { inline void Http2Session::Settings(const nghttp2_settings_entry iv[], size_t niv) { DEBUG_HTTP2SESSION2(this, "submitting %d settings", niv); + Http2Scope h2scope(this); // This will fail either if the system is out of memory, or if the settings // values are not within the appropriate range. We should be catching the // latter before it gets this far so crash in either case. @@ -736,7 +719,8 @@ Http2Stream::SubmitTrailers::SubmitTrailers( inline void Http2Stream::SubmitTrailers::Submit(nghttp2_nv* trailers, - size_t length) const { + size_t length) const { + Http2Scope h2scope(session_); if (length == 0) return; DEBUG_HTTP2SESSION2(session_, "sending trailers for stream %d, count: %d", @@ -891,14 +875,37 @@ inline void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { MakeCallback(env()->onsettings_string(), arraysize(argv), argv); } +void Http2Session::MaybeScheduleWrite() { + CHECK_EQ(flags_ & SESSION_STATE_WRITE_SCHEDULED, 0); + if (session_ != nullptr && nghttp2_session_want_write(session_)) { + flags_ |= SESSION_STATE_WRITE_SCHEDULED; + env()->SetImmediate([](Environment* env, void* data) { + Http2Session* session = static_cast(data); + if (session->session_ == nullptr || + !(session->flags_ & SESSION_STATE_WRITE_SCHEDULED)) { + // This can happen e.g. when a stream was reset before this turn + // of the event loop, in which case SendPendingData() is called early, + // or the session was destroyed in the meantime. + return; + } + + // Sending data may call arbitrary JS code, so keep track of + // async context. + InternalCallbackScope callback_scope(session); + session->SendPendingData(); + }, static_cast(this), object()); + } +} + -inline void Http2Session::SendPendingData() { +void Http2Session::SendPendingData() { DEBUG_HTTP2SESSION(this, "sending pending data"); // Do not attempt to send data on the socket if the destroying flag has // been set. That means everything is shutting down and the socket // will not be usable. if (IsDestroying()) return; + flags_ &= ~SESSION_STATE_WRITE_SCHEDULED; WriteWrap* req = nullptr; char* dest = nullptr; @@ -963,6 +970,7 @@ inline Http2Stream* Http2Session::SubmitRequest( int32_t* ret, int options) { DEBUG_HTTP2SESSION(this, "submitting request"); + Http2Scope h2scope(this); Http2Stream* stream = nullptr; Http2Stream::Provider::Stream prov(options); *ret = nghttp2_submit_request(session_, prispec, nva, len, *prov, nullptr); @@ -1019,6 +1027,7 @@ void Http2Session::OnStreamReadImpl(ssize_t nread, uv_handle_type pending, void* ctx) { Http2Session* session = static_cast(ctx); + Http2Scope h2scope(session); if (nread < 0) { uv_buf_t tmp_buf; tmp_buf.base = nullptr; @@ -1184,6 +1193,7 @@ inline void Http2Stream::Close(int32_t code) { inline void Http2Stream::Shutdown() { + Http2Scope h2scope(this); flags_ |= NGHTTP2_STREAM_FLAG_SHUT; CHECK_NE(nghttp2_session_resume_data(session_->session(), id_), NGHTTP2_ERR_NOMEM); @@ -1198,6 +1208,7 @@ int Http2Stream::DoShutdown(ShutdownWrap* req_wrap) { } inline void Http2Stream::Destroy() { + Http2Scope h2scope(this); DEBUG_HTTP2STREAM(this, "destroying stream"); // Do nothing if this stream instance is already destroyed if (IsDestroyed()) @@ -1249,6 +1260,7 @@ void Http2Stream::OnDataChunk( inline void Http2Stream::FlushDataChunks() { + Http2Scope h2scope(this); if (!data_chunks_.empty()) { uv_buf_t buf = data_chunks_.front(); data_chunks_.pop(); @@ -1266,6 +1278,7 @@ inline void Http2Stream::FlushDataChunks() { inline int Http2Stream::SubmitResponse(nghttp2_nv* nva, size_t len, int options) { + Http2Scope h2scope(this); DEBUG_HTTP2STREAM(this, "submitting response"); if (options & STREAM_OPTION_GET_TRAILERS) flags_ |= NGHTTP2_STREAM_FLAG_TRAILERS; @@ -1286,6 +1299,7 @@ inline int Http2Stream::SubmitFile(int fd, int64_t offset, int64_t length, int options) { + Http2Scope h2scope(this); DEBUG_HTTP2STREAM(this, "submitting file"); if (options & STREAM_OPTION_GET_TRAILERS) flags_ |= NGHTTP2_STREAM_FLAG_TRAILERS; @@ -1302,6 +1316,7 @@ inline int Http2Stream::SubmitFile(int fd, // Submit informational headers for a stream. inline int Http2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) { + Http2Scope h2scope(this); DEBUG_HTTP2STREAM2(this, "sending %d informational headers", len); int ret = nghttp2_submit_headers(session_->session(), NGHTTP2_FLAG_NONE, @@ -1314,6 +1329,7 @@ inline int Http2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) { inline int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, bool silent) { + Http2Scope h2scope(this); DEBUG_HTTP2STREAM(this, "sending priority spec"); int ret = silent ? nghttp2_session_change_stream_priority(session_->session(), @@ -1327,6 +1343,7 @@ inline int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, inline int Http2Stream::SubmitRstStream(const uint32_t code) { + Http2Scope h2scope(this); DEBUG_HTTP2STREAM2(this, "sending rst-stream with code %d", code); session_->SendPendingData(); CHECK_EQ(nghttp2_submit_rst_stream(session_->session(), @@ -1342,6 +1359,7 @@ inline Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva, size_t len, int32_t* ret, int options) { + Http2Scope h2scope(this); DEBUG_HTTP2STREAM(this, "sending push promise"); *ret = nghttp2_submit_push_promise(session_->session(), NGHTTP2_FLAG_NONE, id_, nva, len, nullptr); @@ -1381,6 +1399,7 @@ inline int Http2Stream::Write(nghttp2_stream_write_t* req, const uv_buf_t bufs[], unsigned int nbufs, nghttp2_stream_write_cb cb) { + Http2Scope h2scope(this); if (!IsWritable()) { if (cb != nullptr) cb(req, UV_EOF); @@ -1764,6 +1783,7 @@ void Http2Session::Goaway(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Local context = env->context(); ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); + Http2Scope h2scope(session); uint32_t errorCode = args[0]->Uint32Value(context).ToChecked(); int32_t lastStreamID = args[1]->Int32Value(context).ToChecked(); @@ -2039,6 +2059,7 @@ void Http2Session::Http2Ping::Send(uint8_t* payload) { memcpy(&data, &startTime_, arraysize(data)); payload = data; } + Http2Scope h2scope(session_); CHECK_EQ(nghttp2_submit_ping(**session_, NGHTTP2_FLAG_NONE, payload), 0); } diff --git a/src/node_http2.h b/src/node_http2.h index cd952e7ab01b20..5a61e465a40ce3 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -417,7 +417,9 @@ const char* nghttp2_errname(int rv) { enum session_state_flags { SESSION_STATE_NONE = 0x0, - SESSION_STATE_DESTROYING = 0x1 + SESSION_STATE_DESTROYING = 0x1, + SESSION_STATE_HAS_SCOPE = 0x2, + SESSION_STATE_WRITE_SCHEDULED = 0x4 }; // This allows for 4 default-sized frames with their frame headers @@ -429,6 +431,19 @@ typedef uint32_t(*get_setting)(nghttp2_session* session, class Http2Session; class Http2Stream; +// This scope should be present when any call into nghttp2 that may schedule +// data to be written to the underlying transport is made, and schedules +// such a write automatically once the scope is exited. +class Http2Scope { + public: + explicit Http2Scope(Http2Stream* stream); + explicit Http2Scope(Http2Session* session); + ~Http2Scope(); + + private: + Http2Session* session_ = nullptr; +}; + // The Http2Options class is used to parse the options object passed in to // a Http2Session object and convert those into an appropriate nghttp2_option // struct. This is the primary mechanism by which the Http2Session object is @@ -815,6 +830,9 @@ class Http2Session : public AsyncWrap { inline void MarkDestroying() { flags_ |= SESSION_STATE_DESTROYING; } inline bool IsDestroying() { return flags_ & SESSION_STATE_DESTROYING; } + // Schedule a write if nghttp2 indicates it wants to write to the socket. + void MaybeScheduleWrite(); + // Returns pointer to the stream, or nullptr if stream does not exist inline Http2Stream* FindStream(int32_t id); @@ -1004,6 +1022,8 @@ class Http2Session : public AsyncWrap { size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; std::queue outstanding_pings_; + + friend class Http2Scope; }; class Http2Session::Http2Ping : public AsyncWrap { diff --git a/test/parallel/test-http2-session-gc-while-write-scheduled.js b/test/parallel/test-http2-session-gc-while-write-scheduled.js new file mode 100644 index 00000000000000..bb23760cebf967 --- /dev/null +++ b/test/parallel/test-http2-session-gc-while-write-scheduled.js @@ -0,0 +1,32 @@ +// Flags: --expose-gc + +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); +const makeDuplexPair = require('../common/duplexpair'); + +// This tests that running garbage collection while an Http2Session has +// a write *scheduled*, it will survive that garbage collection. + +{ + // This creates a session and schedules a write (for the settings frame). + let client = http2.connect('http://localhost:80', { + createConnection: common.mustCall(() => makeDuplexPair().clientSide) + }); + + // First, wait for any nextTicks() and their responses + // from the `connect()` call to run. + tick(10, () => { + // This schedules a write. + client.settings(http2.getDefaultSettings()); + client = null; + global.gc(); + }); +} + +function tick(n, cb) { + if (n--) setImmediate(tick, n, cb); + else cb(); +} From 504ee241d55ee45a84d1439e2a9c733f4ab80eff Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 27 Nov 2017 13:20:29 -0800 Subject: [PATCH 04/27] http2: be sure to destroy the Http2Stream PR-URL: https://github.com/nodejs/node/pull/17406 Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Anatoli Papirovski --- src/node_http2.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index f8b530c20a16cf..1ce3c5cb46e231 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -619,13 +619,16 @@ inline int Http2Session::OnStreamClose(nghttp2_session* handle, if (stream != nullptr) { stream->Close(code); // It is possible for the stream close to occur before the stream is - // ever passed on to the javascript side. If that happens, ignore this. + // ever passed on to the javascript side. If that happens, skip straight + // to destroying the stream Local fn = stream->object()->Get(context, env->onstreamclose_string()) .ToLocalChecked(); if (fn->IsFunction()) { Local argv[1] = { Integer::NewFromUnsigned(isolate, code) }; stream->MakeCallback(fn.As(), arraysize(argv), argv); + } else { + stream->Destroy(); } } return 0; From ac89b78847de35b8839e44da6af1d5ded6c7e8ef Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 12 Dec 2017 11:34:17 -0800 Subject: [PATCH 05/27] 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 ac110a77445343..57b529271096da 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -758,6 +758,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 @@ -781,6 +786,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 @@ -920,6 +931,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 @@ -937,10 +953,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 0d298abb8057b7..4f2f8000cf80f7 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' -* Value: {[Settings Object][]} +Calls [`ref()`][`net.Socket.prototype.ref`] on this `Http2Session` +instance's underlying [`net.Socket`]. -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]) +#### http2session.remoteSettings -* `headers` {[Headers Object][]} -* `options` {Object} - * `endStream` {boolean} `true` if the `Http2Stream` *writable* side should - be closed initially, such as when sending a `GET` request that should not - expect a payload body. - * `exclusive` {boolean} When `true` and `parent` identifies a parent Stream, - the created stream is made the sole direct dependency of the parent, with - all other existing dependents made a dependent of the newly created stream. - **Default:** `false` - * `parent` {number} Specifies the numeric identifier of a stream the newly - created stream is dependent on. - * `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). - * `getTrailers` {Function} Callback function invoked to collect trailer - headers. - -* Returns: {ClientHttp2Stream} - -For HTTP/2 Client `Http2Session` instances only, the `http2session.request()` -creates and returns an `Http2Stream` instance that can be used to send an -HTTP/2 request to the connected server. - -This method is only available if `http2session.type` is equal to -`http2.constants.NGHTTP2_SESSION_CLIENT`. - -```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(headers[HTTP2_HEADER_STATUS]); - req.on('data', (chunk) => { /** .. **/ }); - req.on('end', () => { /** .. **/ }); -}); -``` - -When set, the `options.getTrailers()` function is called immediately after -queuing the last chunk of payload data to be sent. The callback is passed a -single object (with a `null` prototype) that the listener may used to specify -the trailing header fields to send to the peer. - -*Note*: The HTTP/1 specification forbids trailers from containing HTTP/2 -"pseudo-header" fields (e.g. `':method'`, `':path'`, etc). An `'error'` event -will be emitted if the `getTrailers` callback attempts to set such header -fields. - -The the `:method` and `:path` pseudoheaders are not specified within `headers`, -they respectively default to: +* Value: {[Settings Object][]} -* `:method` = `'GET'` -* `:path` = `/` +A prototype-less object describing the current remote settings of this +`Http2Session`. The remote settings are set by the *connected* HTTP/2 peer. #### http2session.setTimeout(msecs, callback) + +Calls [`unref()`][`net.Socket.prototype.unref`] on this `Http2Session` +instance's underlying [`net.Socket`]. + +### Class: ClientHttp2Session + + +#### clienthttp2session.request(headers[, options]) + + +* `headers` {[Headers Object][]} +* `options` {Object} + * `endStream` {boolean} `true` if the `Http2Stream` *writable* side should + be closed initially, such as when sending a `GET` request that should not + expect a payload body. + * `exclusive` {boolean} When `true` and `parent` identifies a parent Stream, + the created stream is made the sole direct dependency of the parent, with + all other existing dependents made a dependent of the newly created stream. + **Default:** `false` + * `parent` {number} Specifies the numeric identifier of a stream the newly + created stream is dependent on. + * `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). + * `getTrailers` {Function} Callback function invoked to collect trailer + headers. + +* Returns: {ClientHttp2Stream} + +For HTTP/2 Client `Http2Session` instances only, the `http2session.request()` +creates and returns an `Http2Stream` instance that can be used to send an +HTTP/2 request to the connected server. + +This method is only available if `http2session.type` is equal to +`http2.constants.NGHTTP2_SESSION_CLIENT`. + +```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(headers[HTTP2_HEADER_STATUS]); + req.on('data', (chunk) => { /** .. **/ }); + req.on('end', () => { /** .. **/ }); +}); +``` + +When set, the `options.getTrailers()` function is called immediately after +queuing the last chunk of payload data to be sent. The callback is passed a +single object (with a `null` prototype) that the listener may used to specify +the trailing header fields to send to the peer. + +*Note*: The HTTP/1 specification forbids trailers from containing HTTP/2 +"pseudo-header" fields (e.g. `':method'`, `':path'`, etc). An `'error'` event +will be emitted if the `getTrailers` callback attempts to set such header +fields. + +The `:method` and `:path` pseudoheaders are not specified within `headers`, +they respectively default to: + +* `:method` = `'GET'` +* `:path` = `/` + ### Class: Http2Stream + +#### serverhttp2session.altsvc(alt, originOrStream) + + +* `alt` {string} A description of the alternative service configuration as + defined by [RFC 7838][]. +* `originOrStream` {number|string|URL|Object} Either a URL string specifying + the origin (or an Object with an `origin` property) or the numeric identifier + of an active `Http2Stream` as given by the `http2stream.id` property. + +Submits an `ALTSVC` frame (as defined by [RFC 7838][]) to the connected client. + +```js +const http2 = require('http2'); + +const server = http2.createServer(); +server.on('session', (session) => { + // Set altsvc for origin https://example.org:80 + session.altsvc('h2=":8000"', 'https://example.org:80'); +}); + +server.on('stream', (stream) => { + // Set altsvc for a specific stream + stream.session.altsvc('h2=":8000"', stream.id); +}); +``` + +Sending an `ALTSVC` frame with a specific stream ID indicates that the alternate +service is associated with the origin of the given `Http2Stream`. + +The `alt` and origin string *must* contain only ASCII bytes and are +strictly interpreted as a sequence of ASCII bytes. The special value `'clear'` +may be passed to clear any previously set alternative service for a given +domain. + +When a string is passed for the `originOrStream` argument, it will be parsed as +a URL and the origin will be derived. For insetance, the origin for the +HTTP URL `'https://example.org/foo/bar'` is the ASCII string +`'https://example.org'`. An error will be thrown if either the given string +cannot be parsed as a URL or if a valid origin cannot be derived. + +A `URL` object, or any object with an `origin` property, may be passed as +`originOrStream`, in which case the value of the `origin` property will be +used. The value of the `origin` property *must* be a properly serialized +ASCII origin. + +#### Specifying alternative services + +The format of the `alt` parameter is strictly defined by [RFC 7838][] as an +ASCII string containing a comma-delimited list of "alternative" protocols +associated with a specific host and port. + +For example, the value `'h2="example.org:81"'` indicates that the HTTP/2 +protocol is available on the host `'example.org'` on TCP/IP port 81. The +host and port *must* be contained within the quote (`"`) characters. + +Multiple alternatives may be specified, for instance: `'h2="example.org:81", +h2=":82"'` + +The protocol identifier (`'h2'` in the examples) may be any valid +[ALPN Protocol ID][]. + +The syntax of these values is not validated by the Node.js implementation and +are passed through as provided by the user or received from the peer. + ### Class: ClientHttp2Session +#### Event: 'altsvc' + + +The `'altsvc'` event is emitted whenever an `ALTSVC` frame is received by +the client. The event is emitted with the `ALTSVC` value, origin, and stream +ID, if any. If no `origin` is provided in the `ALTSVC` frame, `origin` will +be an empty string. + +```js +const http2 = require('http2'); +const client = http2.connect('https://example.org'); + +client.on('altsvc', (alt, origin, stream) => { + console.log(alt); + console.log(origin); + console.log(stream); +}); +``` + #### clienthttp2session.request(headers[, options]) + +* Value: {string|undefined} + +Value will be `undefined` if the `Http2Session` is not yet connected to a +socket, `h2c` if the `Http2Session` is not connected to a `TLSSocket`, or +will return the value of the connected `TLSSocket`'s own `alpnProtocol` +property. + #### http2session.close([callback]) + +* Value: {boolean|undefined} + +Value is `undefined` if the `Http2Session` session socket has not yet been +connected, `true` if the `Http2Session` is connected with a `TLSSocket`, +and `false` if the `Http2Session` is connected to any other kind of socket +or stream. + #### http2session.goaway([code, [lastStreamID, [opaqueData]]]) + +* Value: {string[]|undefined} + +If the `Http2Session` is connected to a `TLSSocket`, the `originSet` property +will return an Array of origins for which the `Http2Session` may be +considered authoritative. + #### http2session.pendingSettingsAck + + + +```js +const http2 = require('../common/http2'); +``` + +### Class: Frame + +The `http2.Frame` is a base class that creates a `Buffer` containing a +serialized HTTP/2 frame header. + + + + + +```js +// length is a 24-bit unsigned integer +// type is an 8-bit unsigned integer identifying the frame type +// flags is an 8-bit unsigned integer containing the flag bits +// id is the 32-bit stream identifier, if any. +const frame = new http2.Frame(length, type, flags, id); + +// Write the frame data to a socket +socket.write(frame.data); +``` + +The serialized `Buffer` may be retrieved using the `frame.data` property. + +### Class: DataFrame extends Frame + +The `http2.DataFrame` is a subclass of `http2.Frame` that serializes a `DATA` +frame. + + + + + +```js +// id is the 32-bit stream identifier +// payload is a Buffer containing the DATA payload +// padlen is an 8-bit integer giving the number of padding bytes to include +// final is a boolean indicating whether the End-of-stream flag should be set, +// defaults to false. +const data = new http2.DataFrame(id, payload, padlen, final); + +socket.write(frame.data); +``` + +### Class: HeadersFrame + +The `http2.HeadersFrame` is a subclass of `http2.Frame` that serializes a +`HEADERS` frame. + + + + + +```js +// id is the 32-bit stream identifier +// payload is a Buffer containing the HEADERS payload (see either +// http2.kFakeRequestHeaders or http2.kFakeResponseHeaders). +// padlen is an 8-bit integer giving the number of padding bytes to include +// final is a boolean indicating whether the End-of-stream flag should be set, +// defaults to false. +const data = new http2.HeadersFrame(id, http2.kFakeRequestHeaders, + padlen, final); + +socket.write(frame.data); +``` + +### Class: SettingsFrame + +The `http2.SettingsFrame` is a subclass of `http2.Frame` that serializes an +empty `SETTINGS` frame. + + + + + +```js +// ack is a boolean indicating whether or not to set the ACK flag. +const frame = new http2.SettingsFrame(ack); + +socket.write(frame.data); +``` + +### http2.kFakeRequestHeaders + +Set to a `Buffer` instance that contains a minimal set of serialized HTTP/2 +request headers to be used as the payload of a `http2.HeadersFrame`. + + + + + +```js +const frame = new http2.HeadersFrame(1, http2.kFakeRequestHeaders, 0, true); + +socket.write(frame.data); +``` + +### http2.kFakeResponseHeaders + +Set to a `Buffer` instance that contains a minimal set of serialized HTTP/2 +response headers to be used as the payload a `http2.HeadersFrame`. + + + + + +```js +const frame = new http2.HeadersFrame(1, http2.kFakeResponseHeaders, 0, true); + +socket.write(frame.data); +``` + +### http2.kClientMagic + +Set to a `Buffer` containing the preamble bytes an HTTP/2 client must send +upon initial establishment of a connection. + + + + + +```js +socket.write(http2.kClientMagic); +``` + + [<Array>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array [<ArrayBufferView[]>]: https://developer.mozilla.org/en-US/docs/Web/API/ArrayBufferView [<Boolean>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Boolean_type From c1c4a6c3e21f7f04bde4812a8bfbe143e814952f Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 2 Jan 2018 13:00:12 -0500 Subject: [PATCH 21/27] src: silence http2 -Wunused-result warnings PR-URL: https://github.com/nodejs/node/pull/17954 Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell Reviewed-By: Anna Henningsen --- src/node_http2.cc | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 7bdd520eff193e..9caee6d40183a7 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -559,13 +559,13 @@ inline void Http2Stream::EmitStatistics() { FIXED_ONE_BYTE_STRING(env->isolate(), "timeToFirstByte"), Number::New(env->isolate(), (entry->first_byte() - entry->startTimeNano()) / 1e6), - attr); + attr).FromJust(); obj->DefineOwnProperty( context, FIXED_ONE_BYTE_STRING(env->isolate(), "timeToFirstHeader"), Number::New(env->isolate(), (entry->first_header() - entry->startTimeNano()) / 1e6), - attr); + attr).FromJust(); entry->Notify(obj); } delete entry; @@ -591,25 +591,29 @@ inline void Http2Session::EmitStatistics() { String::NewFromUtf8(env->isolate(), entry->typeName(), v8::NewStringType::kInternalized) - .ToLocalChecked(), attr); + .ToLocalChecked(), attr).FromJust(); if (entry->ping_rtt() != 0) { obj->DefineOwnProperty( context, FIXED_ONE_BYTE_STRING(env->isolate(), "pingRTT"), - Number::New(env->isolate(), entry->ping_rtt() / 1e6), attr); + Number::New(env->isolate(), entry->ping_rtt() / 1e6), + attr).FromJust(); } obj->DefineOwnProperty( context, FIXED_ONE_BYTE_STRING(env->isolate(), "framesReceived"), - Integer::NewFromUnsigned(env->isolate(), entry->frame_count()), attr); + Integer::NewFromUnsigned(env->isolate(), entry->frame_count()), + attr).FromJust(); obj->DefineOwnProperty( context, FIXED_ONE_BYTE_STRING(env->isolate(), "streamCount"), - Integer::New(env->isolate(), entry->stream_count()), attr); + Integer::New(env->isolate(), entry->stream_count()), + attr).FromJust(); obj->DefineOwnProperty( context, FIXED_ONE_BYTE_STRING(env->isolate(), "streamAverageDuration"), - Number::New(env->isolate(), entry->stream_average_duration()), attr); + Number::New(env->isolate(), entry->stream_average_duration()), + attr).FromJust(); entry->Notify(obj); } delete entry; From adbc38a5cfb8434d13ce68fcb0e7fa9c1c0a3e99 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 3 Jan 2018 11:15:57 -0800 Subject: [PATCH 22/27] http2: implement maxSessionMemory The maxSessionMemory is a cap for the amount of memory an Http2Session is permitted to consume. If exceeded, new `Http2Stream` sessions will be rejected with an `ENHANCE_YOUR_CALM` error and existing `Http2Stream` instances that are still receiving headers will be terminated with an `ENHANCE_YOUR_CALM` error. PR-URL: https://github.com/nodejs/node/pull/17967 Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina --- doc/api/http2.md | 27 +++++++++ lib/internal/http2/util.js | 8 ++- src/node_http2.cc | 55 +++++++++++++++---- src/node_http2.h | 45 ++++++++++++++- src/node_http2_state.h | 1 + .../test-http2-util-update-options-buffer.js | 7 ++- .../test-http2-max-session-memory.js | 44 +++++++++++++++ 7 files changed, 172 insertions(+), 15 deletions(-) create mode 100644 test/sequential/test-http2-max-session-memory.js diff --git a/doc/api/http2.md b/doc/api/http2.md index 33da4b53f7fde7..1822dfe7739326 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1633,6 +1633,15 @@ changes: * `options` {Object} * `maxDeflateDynamicTableSize` {number} Sets the maximum dynamic table size for deflating header fields. **Default:** `4Kib` + * `maxSessionMemory`{number} Sets the maximum memory that the `Http2Session` + is permitted to use. The value is expressed in terms of number of megabytes, + e.g. `1` equal 1 megabyte. The minimum value allowed is `1`. **Default:** + `10`. This is a credit based limit, existing `Http2Stream`s may cause this + limit to be exceeded, but new `Http2Stream` instances will be rejected + while this limit is exceeded. The current number of `Http2Stream` sessions, + the current memory use of the header compression tables, current data + queued to be sent, and unacknowledged PING and SETTINGS frames are all + counted towards the current limit. * `maxHeaderListPairs` {number} Sets the maximum number of header entries. **Default:** `128`. The minimum value is `4`. * `maxOutstandingPings` {number} Sets the maximum number of outstanding, @@ -1711,6 +1720,15 @@ changes: `false`. See the [`'unknownProtocol'`][] event. See [ALPN negotiation][]. * `maxDeflateDynamicTableSize` {number} Sets the maximum dynamic table size for deflating header fields. **Default:** `4Kib` + * `maxSessionMemory`{number} Sets the maximum memory that the `Http2Session` + is permitted to use. The value is expressed in terms of number of megabytes, + e.g. `1` equal 1 megabyte. The minimum value allowed is `1`. **Default:** + `10`. This is a credit based limit, existing `Http2Stream`s may cause this + limit to be exceeded, but new `Http2Stream` instances will be rejected + while this limit is exceeded. The current number of `Http2Stream` sessions, + the current memory use of the header compression tables, current data + queued to be sent, and unacknowledged PING and SETTINGS frames are all + counted towards the current limit. * `maxHeaderListPairs` {number} Sets the maximum number of header entries. **Default:** `128`. The minimum value is `4`. * `maxOutstandingPings` {number} Sets the maximum number of outstanding, @@ -1794,6 +1812,15 @@ changes: * `options` {Object} * `maxDeflateDynamicTableSize` {number} Sets the maximum dynamic table size for deflating header fields. **Default:** `4Kib` + * `maxSessionMemory`{number} Sets the maximum memory that the `Http2Session` + is permitted to use. The value is expressed in terms of number of megabytes, + e.g. `1` equal 1 megabyte. The minimum value allowed is `1`. **Default:** + `10`. This is a credit based limit, existing `Http2Stream`s may cause this + limit to be exceeded, but new `Http2Stream` instances will be rejected + while this limit is exceeded. The current number of `Http2Stream` sessions, + the current memory use of the header compression tables, current data + queued to be sent, and unacknowledged PING and SETTINGS frames are all + counted towards the current limit. * `maxHeaderListPairs` {number} Sets the maximum number of header entries. **Default:** `128`. The minimum value is `1`. * `maxOutstandingPings` {number} Sets the maximum number of outstanding, diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index fffea10ab6f819..1411ab7cf72ad7 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -175,7 +175,8 @@ const IDX_OPTIONS_PADDING_STRATEGY = 4; const IDX_OPTIONS_MAX_HEADER_LIST_PAIRS = 5; const IDX_OPTIONS_MAX_OUTSTANDING_PINGS = 6; const IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS = 7; -const IDX_OPTIONS_FLAGS = 8; +const IDX_OPTIONS_MAX_SESSION_MEMORY = 8; +const IDX_OPTIONS_FLAGS = 9; function updateOptionsBuffer(options) { var flags = 0; @@ -219,6 +220,11 @@ function updateOptionsBuffer(options) { optionsBuffer[IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS] = Math.max(1, options.maxOutstandingSettings); } + if (typeof options.maxSessionMemory === 'number') { + flags |= (1 << IDX_OPTIONS_MAX_SESSION_MEMORY); + optionsBuffer[IDX_OPTIONS_MAX_SESSION_MEMORY] = + Math.max(1, options.maxSessionMemory); + } optionsBuffer[IDX_OPTIONS_FLAGS] = flags; } diff --git a/src/node_http2.cc b/src/node_http2.cc index 9caee6d40183a7..85bdde9b92160f 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -174,6 +174,18 @@ Http2Options::Http2Options(Environment* env) { if (flags & (1 << IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS)) { SetMaxOutstandingSettings(buffer[IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS]); } + + // The HTTP2 specification places no limits on the amount of memory + // that a session can consume. In order to prevent abuse, we place a + // cap on the amount of memory a session can consume at any given time. + // this is a credit based system. Existing streams may cause the limit + // to be temporarily exceeded but once over the limit, new streams cannot + // created. + // Important: The maxSessionMemory option in javascript is expressed in + // terms of MB increments (i.e. the value 1 == 1 MB) + if (flags & (1 << IDX_OPTIONS_MAX_SESSION_MEMORY)) { + SetMaxSessionMemory(buffer[IDX_OPTIONS_MAX_SESSION_MEMORY] * 1e6); + } } void Http2Session::Http2Settings::Init() { @@ -482,11 +494,13 @@ Http2Session::Http2Session(Environment* env, // Capture the configuration options for this session Http2Options opts(env); - int32_t maxHeaderPairs = opts.GetMaxHeaderPairs(); + max_session_memory_ = opts.GetMaxSessionMemory(); + + uint32_t maxHeaderPairs = opts.GetMaxHeaderPairs(); max_header_pairs_ = type == NGHTTP2_SESSION_SERVER - ? std::max(maxHeaderPairs, 4) // minimum # of request headers - : std::max(maxHeaderPairs, 1); // minimum # of response headers + ? std::max(maxHeaderPairs, 4U) // minimum # of request headers + : std::max(maxHeaderPairs, 1U); // minimum # of response headers max_outstanding_pings_ = opts.GetMaxOutstandingPings(); max_outstanding_settings_ = opts.GetMaxOutstandingSettings(); @@ -672,18 +686,21 @@ inline bool Http2Session::CanAddStream() { size_t maxSize = std::min(streams_.max_size(), static_cast(maxConcurrentStreams)); // We can add a new stream so long as we are less than the current - // maximum on concurrent streams - return streams_.size() < maxSize; + // maximum on concurrent streams and there's enough available memory + return streams_.size() < maxSize && + IsAvailableSessionMemory(sizeof(Http2Stream)); } inline void Http2Session::AddStream(Http2Stream* stream) { CHECK_GE(++statistics_.stream_count, 0); streams_[stream->id()] = stream; + IncrementCurrentSessionMemory(stream->self_size()); } -inline void Http2Session::RemoveStream(int32_t id) { - streams_.erase(id); +inline void Http2Session::RemoveStream(Http2Stream* stream) { + streams_.erase(stream->id()); + DecrementCurrentSessionMemory(stream->self_size()); } // Used as one of the Padding Strategy functions. Will attempt to ensure @@ -1677,7 +1694,7 @@ Http2Stream::Http2Stream( Http2Stream::~Http2Stream() { if (session_ != nullptr) { - session_->RemoveStream(id_); + session_->RemoveStream(this); session_ = nullptr; } @@ -2007,7 +2024,7 @@ inline int Http2Stream::DoWrite(WriteWrap* req_wrap, i == nbufs - 1 ? req_wrap : nullptr, bufs[i] }); - available_outbound_length_ += bufs[i].len; + IncrementAvailableOutboundLength(bufs[i].len); } CHECK_NE(nghttp2_session_resume_data(**session_, id_), NGHTTP2_ERR_NOMEM); return 0; @@ -2029,7 +2046,10 @@ inline bool Http2Stream::AddHeader(nghttp2_rcbuf* name, if (this->statistics_.first_header == 0) this->statistics_.first_header = uv_hrtime(); size_t length = GetBufferLength(name) + GetBufferLength(value) + 32; - if (current_headers_.size() == max_header_pairs_ || + // A header can only be added if we have not exceeded the maximum number + // of headers and the session has memory available for it. + if (!session_->IsAvailableSessionMemory(length) || + current_headers_.size() == max_header_pairs_ || current_headers_length_ + length > max_header_length_) { return false; } @@ -2173,7 +2193,7 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, // Just return the length, let Http2Session::OnSendData take care of // actually taking the buffers out of the queue. *flags |= NGHTTP2_DATA_FLAG_NO_COPY; - stream->available_outbound_length_ -= amount; + stream->DecrementAvailableOutboundLength(amount); } } @@ -2196,6 +2216,15 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, return amount; } +inline void Http2Stream::IncrementAvailableOutboundLength(size_t amount) { + available_outbound_length_ += amount; + session_->IncrementCurrentSessionMemory(amount); +} + +inline void Http2Stream::DecrementAvailableOutboundLength(size_t amount) { + available_outbound_length_ -= amount; + session_->DecrementCurrentSessionMemory(amount); +} // Implementation of the JavaScript API @@ -2689,6 +2718,7 @@ Http2Session::Http2Ping* Http2Session::PopPing() { if (!outstanding_pings_.empty()) { ping = outstanding_pings_.front(); outstanding_pings_.pop(); + DecrementCurrentSessionMemory(ping->self_size()); } return ping; } @@ -2697,6 +2727,7 @@ bool Http2Session::AddPing(Http2Session::Http2Ping* ping) { if (outstanding_pings_.size() == max_outstanding_pings_) return false; outstanding_pings_.push(ping); + IncrementCurrentSessionMemory(ping->self_size()); return true; } @@ -2705,6 +2736,7 @@ Http2Session::Http2Settings* Http2Session::PopSettings() { if (!outstanding_settings_.empty()) { settings = outstanding_settings_.front(); outstanding_settings_.pop(); + DecrementCurrentSessionMemory(settings->self_size()); } return settings; } @@ -2713,6 +2745,7 @@ bool Http2Session::AddSettings(Http2Session::Http2Settings* settings) { if (outstanding_settings_.size() == max_outstanding_settings_) return false; outstanding_settings_.push(settings); + IncrementCurrentSessionMemory(settings->self_size()); return true; } diff --git a/src/node_http2.h b/src/node_http2.h index f63f8133a4a657..4fc98f0c68a3ba 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -82,6 +82,9 @@ void inline debug_vfprintf(const char* format, ...) { // Also strictly limit the number of outstanding SETTINGS frames a user sends #define DEFAULT_MAX_SETTINGS 10 +// Default maximum total memory cap for Http2Session. +#define DEFAULT_MAX_SESSION_MEMORY 1e7; + // These are the standard HTTP/2 defaults as specified by the RFC #define DEFAULT_SETTINGS_HEADER_TABLE_SIZE 4096 #define DEFAULT_SETTINGS_ENABLE_PUSH 1 @@ -501,8 +504,17 @@ class Http2Options { return max_outstanding_settings_; } + void SetMaxSessionMemory(uint64_t max) { + max_session_memory_ = max; + } + + uint64_t GetMaxSessionMemory() { + return max_session_memory_; + } + private: nghttp2_option* options_; + uint64_t max_session_memory_ = DEFAULT_MAX_SESSION_MEMORY; uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; padding_strategy_type padding_strategy_ = PADDING_STRATEGY_NONE; size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; @@ -629,6 +641,9 @@ class Http2Stream : public AsyncWrap, // Returns the stream identifier for this stream inline int32_t id() const { return id_; } + inline void IncrementAvailableOutboundLength(size_t amount); + inline void DecrementAvailableOutboundLength(size_t amount); + inline bool AddHeader(nghttp2_rcbuf* name, nghttp2_rcbuf* value, uint8_t flags); @@ -848,7 +863,7 @@ class Http2Session : public AsyncWrap { inline void AddStream(Http2Stream* stream); // Removes a stream instance from this session - inline void RemoveStream(int32_t id); + inline void RemoveStream(Http2Stream* stream); // Write data to the session inline ssize_t Write(const uv_buf_t* bufs, size_t nbufs); @@ -906,6 +921,30 @@ class Http2Session : public AsyncWrap { Http2Settings* PopSettings(); bool AddSettings(Http2Settings* settings); + void IncrementCurrentSessionMemory(uint64_t amount) { + current_session_memory_ += amount; + } + + void DecrementCurrentSessionMemory(uint64_t amount) { + current_session_memory_ -= amount; + } + + // Returns the current session memory including the current size of both + // the inflate and deflate hpack headers, the current outbound storage + // queue, and pending writes. + uint64_t GetCurrentSessionMemory() { + uint64_t total = current_session_memory_ + sizeof(Http2Session); + total += nghttp2_session_get_hd_deflate_dynamic_table_size(session_); + total += nghttp2_session_get_hd_inflate_dynamic_table_size(session_); + total += outgoing_storage_.size(); + return total; + } + + // Return true if current_session_memory + amount is less than the max + bool IsAvailableSessionMemory(uint64_t amount) { + return GetCurrentSessionMemory() + amount <= max_session_memory_; + } + struct Statistics { uint64_t start_time; uint64_t end_time; @@ -1035,6 +1074,10 @@ class Http2Session : public AsyncWrap { // The maximum number of header pairs permitted for streams on this session uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; + // The maximum amount of memory allocated for this session + uint64_t max_session_memory_ = DEFAULT_MAX_SESSION_MEMORY; + uint64_t current_session_memory_ = 0; + // The collection of active Http2Streams associated with this session std::unordered_map streams_; diff --git a/src/node_http2_state.h b/src/node_http2_state.h index ef8696ce60d8f8..af0740c994e765 100644 --- a/src/node_http2_state.h +++ b/src/node_http2_state.h @@ -50,6 +50,7 @@ namespace http2 { IDX_OPTIONS_MAX_HEADER_LIST_PAIRS, IDX_OPTIONS_MAX_OUTSTANDING_PINGS, IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS, + IDX_OPTIONS_MAX_SESSION_MEMORY, IDX_OPTIONS_FLAGS }; diff --git a/test/parallel/test-http2-util-update-options-buffer.js b/test/parallel/test-http2-util-update-options-buffer.js index 5768ce0204dc8c..6ab8bcff02866e 100644 --- a/test/parallel/test-http2-util-update-options-buffer.js +++ b/test/parallel/test-http2-util-update-options-buffer.js @@ -20,7 +20,8 @@ const IDX_OPTIONS_PADDING_STRATEGY = 4; const IDX_OPTIONS_MAX_HEADER_LIST_PAIRS = 5; const IDX_OPTIONS_MAX_OUTSTANDING_PINGS = 6; const IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS = 7; -const IDX_OPTIONS_FLAGS = 8; +const IDX_OPTIONS_MAX_SESSION_MEMORY = 8; +const IDX_OPTIONS_FLAGS = 9; { updateOptionsBuffer({ @@ -31,7 +32,8 @@ const IDX_OPTIONS_FLAGS = 8; paddingStrategy: 5, maxHeaderListPairs: 6, maxOutstandingPings: 7, - maxOutstandingSettings: 8 + maxOutstandingSettings: 8, + maxSessionMemory: 9 }); strictEqual(optionsBuffer[IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE], 1); @@ -42,6 +44,7 @@ const IDX_OPTIONS_FLAGS = 8; strictEqual(optionsBuffer[IDX_OPTIONS_MAX_HEADER_LIST_PAIRS], 6); strictEqual(optionsBuffer[IDX_OPTIONS_MAX_OUTSTANDING_PINGS], 7); strictEqual(optionsBuffer[IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS], 8); + strictEqual(optionsBuffer[IDX_OPTIONS_MAX_SESSION_MEMORY], 9); const flags = optionsBuffer[IDX_OPTIONS_FLAGS]; diff --git a/test/sequential/test-http2-max-session-memory.js b/test/sequential/test-http2-max-session-memory.js new file mode 100644 index 00000000000000..e16000d1261ab0 --- /dev/null +++ b/test/sequential/test-http2-max-session-memory.js @@ -0,0 +1,44 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); + +// Test that maxSessionMemory Caps work + +const largeBuffer = Buffer.alloc(1e6); + +const server = http2.createServer({ maxSessionMemory: 1 }); + +server.on('stream', common.mustCall((stream) => { + stream.respond(); + stream.end(largeBuffer); +})); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + + { + const req = client.request(); + + req.on('response', () => { + // This one should be rejected because the server is over budget + // on the current memory allocation + const req = client.request(); + req.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + type: Error, + message: 'Stream closed with error code 11' + })); + req.on('close', common.mustCall(() => { + server.close(); + client.destroy(); + })); + }); + + req.resume(); + req.on('close', common.mustCall()); + } +})); From 51dc318d7d024aa013e64f9ef4b4c25fa712fa18 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 3 Jan 2018 12:02:46 -0800 Subject: [PATCH 23/27] http2: verify that a dependency cycle may exist PR-URL: https://github.com/nodejs/node/pull/17968 Reviewed-By: Anna Henningsen --- test/parallel/test-http2-priority-cycle-.js | 69 +++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 test/parallel/test-http2-priority-cycle-.js diff --git a/test/parallel/test-http2-priority-cycle-.js b/test/parallel/test-http2-priority-cycle-.js new file mode 100644 index 00000000000000..af0d66d8343cbf --- /dev/null +++ b/test/parallel/test-http2-priority-cycle-.js @@ -0,0 +1,69 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); +const Countdown = require('../common/countdown'); + +const server = http2.createServer(); +const largeBuffer = Buffer.alloc(1e4); + +// Verify that a dependency cycle may exist, but that it doesn't crash anything + +server.on('stream', common.mustCall((stream) => { + stream.respond(); + setImmediate(() => { + stream.end(largeBuffer); + }); +}, 3)); +server.on('session', common.mustCall((session) => { + session.on('priority', (id, parent, weight, exclusive) => { + assert.strictEqual(weight, 16); + assert.strictEqual(exclusive, false); + switch (id) { + case 1: + assert.strictEqual(parent, 5); + break; + case 3: + assert.strictEqual(parent, 1); + break; + case 5: + assert.strictEqual(parent, 3); + break; + default: + assert.fail('should not happen'); + } + }); +})); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + + const countdown = new Countdown(3, () => { + client.close(); + server.close(); + }); + + { + const req = client.request(); + req.priority({ parent: 5 }); + req.resume(); + req.on('close', () => countdown.dec()); + } + + { + const req = client.request(); + req.priority({ parent: 1 }); + req.resume(); + req.on('close', () => countdown.dec()); + } + + { + const req = client.request(); + req.priority({ parent: 3 }); + req.resume(); + req.on('close', () => countdown.dec()); + } +})); From d1e75eb719636554149a431ce2c3ab66714249eb Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 3 Jan 2018 16:39:46 -0800 Subject: [PATCH 24/27] doc: grammar fixes in http2.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/17972 Reviewed-By: Weijia Wang Reviewed-By: Jon Moss Reviewed-By: Evan Lucas Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater Reviewed-By: Tobias Nießen --- doc/api/http2.md | 56 ++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index 1822dfe7739326..06734e9b78f7c0 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -166,7 +166,7 @@ When invoked, the handler function will receive three arguments: If the `'frameError'` event is associated with a stream, the stream will be closed and destroyed immediately following the `'frameError'` event. If the -event is not associated with a stream, the `Http2Session` will be shutdown +event is not associated with a stream, the `Http2Session` will be shut down immediately following the `'frameError'` event. #### Event: 'goaway' @@ -183,7 +183,7 @@ the handler function will receive three arguments: * `opaqueData` {Buffer} If additional opaque data was included in the GOAWAY frame, a `Buffer` instance will be passed containing that data. -*Note*: The `Http2Session` instance will be shutdown automatically when the +*Note*: The `Http2Session` instance will be shut down automatically when the `'goaway'` event is emitted. #### Event: 'localSettings' @@ -499,7 +499,7 @@ added: v8.4.0 has been completed. * Returns: {undefined} -Attempts to shutdown this `Http2Session` using HTTP/2 defined procedures. +Attempts to shut down this `Http2Session` using HTTP/2 defined procedures. If specified, the given `callback` function will be invoked once the shutdown process has completed. @@ -635,7 +635,7 @@ may be passed to clear any previously set alternative service for a given domain. When a string is passed for the `originOrStream` argument, it will be parsed as -a URL and the origin will be derived. For insetance, the origin for the +a URL and the origin will be derived. For instance, the origin for the HTTP URL `'https://example.org/foo/bar'` is the ASCII string `'https://example.org'`. An error will be thrown if either the given string cannot be parsed as a URL or if a valid origin cannot be derived. @@ -739,15 +739,15 @@ req.on('response', (headers) => { When set, the `options.getTrailers()` function is called immediately after queuing the last chunk of payload data to be sent. The callback is passed a -single object (with a `null` prototype) that the listener may used to specify +single object (with a `null` prototype) that the listener may use to specify the trailing header fields to send to the peer. *Note*: The HTTP/1 specification forbids trailers from containing HTTP/2 -"pseudo-header" fields (e.g. `':method'`, `':path'`, etc). An `'error'` event +pseudo-header fields (e.g. `':method'`, `':path'`, etc). An `'error'` event will be emitted if the `getTrailers` callback attempts to set such header fields. -The `:method` and `:path` pseudoheaders are not specified within `headers`, +The `:method` and `:path` pseudo-headers are not specified within `headers`, they respectively default to: * `:method` = `'GET'` @@ -774,7 +774,7 @@ On the client, `Http2Stream` instances are created and returned when either the `'push'` event. *Note*: The `Http2Stream` class is a base for the [`ServerHttp2Stream`][] and -[`ClientHttp2Stream`][] classes, each of which are used specifically by either +[`ClientHttp2Stream`][] classes, each of which is used specifically by either the Server or Client side, respectively. All `Http2Stream` instances are [`Duplex`][] streams. The `Writable` side of the @@ -798,7 +798,7 @@ On the client side, instances of [`ClientHttp2Stream`][] are created when the `http2session.request()` may not be immediately ready for use if the parent `Http2Session` has not yet been fully established. In such cases, operations called on the `Http2Stream` will be buffered until the `'ready'` event is -emitted. User code should rarely, if ever, have need to handle the `'ready'` +emitted. User code should rarely, if ever, need to handle the `'ready'` event directly. The ready status of an `Http2Stream` can be determined by checking the value of `http2stream.id`. If the value is `undefined`, the stream is not yet ready for use. @@ -1048,7 +1048,7 @@ added: v8.4.0 --> The `'headers'` event is emitted when an additional block of headers is received -for a stream, such as when a block of `1xx` informational headers are received. +for a stream, such as when a block of `1xx` informational headers is received. The listener callback is passed the [Headers Object][] and flags associated with the headers. @@ -1198,7 +1198,7 @@ server.on('stream', (stream) => { When set, the `options.getTrailers()` function is called immediately after queuing the last chunk of payload data to be sent. The callback is passed a -single object (with a `null` prototype) that the listener may used to specify +single object (with a `null` prototype) that the listener may use to specify the trailing header fields to send to the peer. ```js @@ -1215,7 +1215,7 @@ server.on('stream', (stream) => { ``` *Note*: The HTTP/1 specification forbids trailers from containing HTTP/2 -"pseudo-header" fields (e.g. `':status'`, `':path'`, etc). An `'error'` event +pseudo-header fields (e.g. `':status'`, `':path'`, etc). An `'error'` event will be emitted if the `getTrailers` callback attempts to set such header fields. @@ -1272,7 +1272,7 @@ requests. When set, the `options.getTrailers()` function is called immediately after queuing the last chunk of payload data to be sent. The callback is passed a -single object (with a `null` prototype) that the listener may used to specify +single object (with a `null` prototype) that the listener may use to specify the trailing header fields to send to the peer. ```js @@ -1299,7 +1299,7 @@ server.on('close', () => fs.closeSync(fd)); ``` *Note*: The HTTP/1 specification forbids trailers from containing HTTP/2 -"pseudo-header" fields (e.g. `':status'`, `':path'`, etc). An `'error'` event +pseudo-header fields (e.g. `':status'`, `':path'`, etc). An `'error'` event will be emitted if the `getTrailers` callback attempts to set such header fields. @@ -1331,7 +1331,7 @@ of the given file: If an error occurs while attempting to read the file data, the `Http2Stream` will be closed using an `RST_STREAM` frame using the standard `INTERNAL_ERROR` -code. If the `onError` callback is defined it will be called, otherwise +code. If the `onError` callback is defined, then it will be called. Otherwise the stream will be destroyed. Example using a file path: @@ -1391,7 +1391,7 @@ default behavior is to destroy the stream. When set, the `options.getTrailers()` function is called immediately after queuing the last chunk of payload data to be sent. The callback is passed a -single object (with a `null` prototype) that the listener may used to specify +single object (with a `null` prototype) that the listener may use to specify the trailing header fields to send to the peer. ```js @@ -1408,7 +1408,7 @@ server.on('stream', (stream) => { ``` *Note*: The HTTP/1 specification forbids trailers from containing HTTP/2 -"pseudo-header" fields (e.g. `':status'`, `':path'`, etc). An `'error'` event +pseudo-header fields (e.g. `':status'`, `':path'`, etc). An `'error'` event will be emitted if the `getTrailers` callback attempts to set such header fields. @@ -1478,7 +1478,7 @@ an `Http2Session` object associated with the `Http2Server`. added: v8.5.0 --> -If an `ServerHttp2Stream` emits an `'error'` event, it will be forwarded here. +If a `ServerHttp2Stream` emits an `'error'` event, it will be forwarded here. The stream will already be destroyed when this event is triggered. #### Event: 'stream' @@ -1664,7 +1664,7 @@ changes: * `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply enough padding to ensure that the total frame length, including the 9-byte header, is a multiple of 8. For each frame, however, there is a - maxmimum allowed number of padding bytes that is determined by current + maximum allowed number of padding bytes that is determined by current flow control state and settings. If this maximum is less than the calculated amount needed to ensure alignment, the maximum will be used and the total frame length will *not* necessarily be aligned at 8 bytes. @@ -1751,7 +1751,7 @@ changes: * `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply enough padding to ensure that the total frame length, including the 9-byte header, is a multiple of 8. For each frame, however, there is a - maxmimum allowed number of padding bytes that is determined by current + maximum allowed number of padding bytes that is determined by current flow control state and settings. If this maximum is less than the calculated amount needed to ensure alignment, the maximum will be used and the total frame length will *not* necessarily be aligned at 8 bytes. @@ -1847,7 +1847,7 @@ changes: * `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply enough padding to ensure that the total frame length, including the 9-byte header, is a multiple of 8. For each frame, however, there is a - maxmimum allowed number of padding bytes that is determined by current + maximum allowed number of padding bytes that is determined by current flow control state and settings. If this maximum is less than the calculated amount needed to ensure alignment, the maximum will be used and the total frame length will *not* necessarily be aligned at 8 bytes. @@ -2189,8 +2189,8 @@ req.end('Jane'); The Compatibility API has the goal of providing a similar developer experience of HTTP/1 when using HTTP/2, making it possible to develop applications -that supports both [HTTP/1][] and HTTP/2. This API targets only the -**public API** of the [HTTP/1][], however many modules uses internal +that support both [HTTP/1][] and HTTP/2. This API targets only the +**public API** of the [HTTP/1][]. However many modules use internal methods or state, and those _are not supported_ as it is a completely different implementation. @@ -2218,7 +2218,7 @@ the status message for HTTP codes is ignored. ### ALPN negotiation -ALPN negotiation allows to support both [HTTPS][] and HTTP/2 over +ALPN negotiation allows supporting both [HTTPS][] and HTTP/2 over the same socket. The `req` and `res` objects can be either HTTP/1 or HTTP/2, and an application **must** restrict itself to the public API of [HTTP/1][], and detect if it is possible to use the more advanced @@ -2260,7 +2260,7 @@ added: v8.4.0 A `Http2ServerRequest` object is created by [`http2.Server`][] or [`http2.SecureServer`][] and passed as the first argument to the -[`'request'`][] event. It may be used to access a request status, headers and +[`'request'`][] event. It may be used to access a request status, headers, and data. It implements the [Readable Stream][] interface, as well as the @@ -2321,7 +2321,7 @@ console.log(request.headers); See [Headers Object][]. -*Note*: In HTTP/2, the request path, host name, protocol, and method are +*Note*: In HTTP/2, the request path, hostname, protocol, and method are represented as special headers prefixed with the `:` character (e.g. `':path'`). These special headers will be included in the `request.headers` object. Care must be taken not to inadvertently modify these special headers or errors may @@ -2354,7 +2354,7 @@ added: v8.4.0 * {string} -The request method as a string. Read only. Example: +The request method as a string. Read-only. Example: `'GET'`, `'DELETE'`. #### request.rawHeaders @@ -3012,7 +3012,7 @@ If `name` is equal to `Http2Session`, the `PerformanceEntry` will contain the following additional properties: * `pingRTT` {number} The number of milliseconds elapsed since the transmission - of a `PING` frame and the reception of its acknowledgement. Only present if + of a `PING` frame and the reception of its acknowledgment. Only present if a `PING` frame has been sent on the `Http2Session`. * `streamCount` {number} The number of `Http2Stream` instances processed by the `Http2Session`. From cd6fcf37f4d02ca7a063949c7f00923c81c1f256 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 3 Jan 2018 13:34:45 -0800 Subject: [PATCH 25/27] http2: verify flood error and unsolicited frames * verify protections against ping and settings flooding * Strictly handle and verify handling of unsolicited ping and settings frame acks. PR-URL: https://github.com/nodejs/node/pull/17969 Reviewed-By: Anna Henningsen Reviewed-By: Anatoli Papirovski Reviewed-By: Matteo Collina --- src/node_http2.cc | 40 ++++++++++++- test/common/http2.js | 10 ++++ .../test-http2-ping-unsolicited-ack.js | 43 ++++++++++++++ .../test-http2-settings-unsolicited-ack.js | 50 +++++++++++++++++ test/sequential/sequential.status | 2 + test/sequential/test-http2-ping-flood.js | 56 +++++++++++++++++++ test/sequential/test-http2-settings-flood.js | 53 ++++++++++++++++++ 7 files changed, 252 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http2-ping-unsolicited-ack.js create mode 100644 test/parallel/test-http2-settings-unsolicited-ack.js create mode 100644 test/sequential/test-http2-ping-flood.js create mode 100644 test/sequential/test-http2-settings-flood.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 85bdde9b92160f..404606189cbbd6 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1305,8 +1305,24 @@ inline void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; if (ack) { Http2Ping* ping = PopPing(); - if (ping != nullptr) + if (ping != nullptr) { ping->Done(true, frame->ping.opaque_data); + } else { + // PING Ack is unsolicited. Treat as a connection error. The HTTP/2 + // spec does not require this, but there is no legitimate reason to + // receive an unsolicited PING ack on a connection. Either the peer + // is buggy or malicious, and we're not going to tolerate such + // nonsense. + Isolate* isolate = env()->isolate(); + HandleScope scope(isolate); + Local context = env()->context(); + Context::Scope context_scope(context); + + Local argv[1] = { + Integer::New(isolate, NGHTTP2_ERR_PROTO), + }; + MakeCallback(env()->error_string(), arraysize(argv), argv); + } } } @@ -1317,8 +1333,28 @@ inline void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { // If this is an acknowledgement, we should have an Http2Settings // object for it. Http2Settings* settings = PopSettings(); - if (settings != nullptr) + if (settings != nullptr) { settings->Done(true); + } else { + // SETTINGS Ack is unsolicited. Treat as a connection error. The HTTP/2 + // spec does not require this, but there is no legitimate reason to + // receive an unsolicited SETTINGS ack on a connection. Either the peer + // is buggy or malicious, and we're not going to tolerate such + // nonsense. + // Note that nghttp2 currently prevents this from happening for SETTINGS + // frames, so this block is purely defensive just in case that behavior + // changes. Specifically, unlike unsolicited PING acks, unsolicited + // SETTINGS acks should *never* make it this far. + Isolate* isolate = env()->isolate(); + HandleScope scope(isolate); + Local context = env()->context(); + Context::Scope context_scope(context); + + Local argv[1] = { + Integer::New(isolate, NGHTTP2_ERR_PROTO), + }; + MakeCallback(env()->error_string(), arraysize(argv), argv); + } } else { // Otherwise, notify the session about a new settings MakeCallback(env()->onsettings_string(), 0, nullptr); diff --git a/test/common/http2.js b/test/common/http2.js index 44f5f9191e6e33..1d4c269fffd5b5 100644 --- a/test/common/http2.js +++ b/test/common/http2.js @@ -118,11 +118,21 @@ class HeadersFrame extends Frame { } } +class PingFrame extends Frame { + constructor(ack = false) { + const buffers = [Buffer.alloc(8)]; + super(8, 6, ack ? 1 : 0, 0); + buffers.unshift(this[kFrameData]); + this[kFrameData] = Buffer.concat(buffers); + } +} + module.exports = { Frame, DataFrame, HeadersFrame, SettingsFrame, + PingFrame, kFakeRequestHeaders, kFakeResponseHeaders, kClientMagic diff --git a/test/parallel/test-http2-ping-unsolicited-ack.js b/test/parallel/test-http2-ping-unsolicited-ack.js new file mode 100644 index 00000000000000..5a3a261cb098b1 --- /dev/null +++ b/test/parallel/test-http2-ping-unsolicited-ack.js @@ -0,0 +1,43 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); +const net = require('net'); +const http2util = require('../common/http2'); + +// Test that ping flooding causes the session to be torn down + +const kSettings = new http2util.SettingsFrame(); +const kPingAck = new http2util.PingFrame(true); + +const server = http2.createServer(); + +server.on('stream', common.mustNotCall()); +server.on('session', common.mustCall((session) => { + session.on('error', common.expectsError({ + code: 'ERR_HTTP2_ERROR', + message: 'Protocol error' + })); + session.on('close', common.mustCall(() => server.close())); +})); + +server.listen(0, common.mustCall(() => { + const client = net.connect(server.address().port); + + client.on('connect', common.mustCall(() => { + client.write(http2util.kClientMagic, () => { + client.write(kSettings.data); + // Send an unsolicited ping ack + client.write(kPingAck.data); + }); + })); + + // An error event may or may not be emitted, depending on operating system + // and timing. We do not really care if one is emitted here or not, as the + // error on the server side is what we are testing for. Do not make this + // a common.mustCall() and there's no need to check the error details. + client.on('error', () => {}); +})); diff --git a/test/parallel/test-http2-settings-unsolicited-ack.js b/test/parallel/test-http2-settings-unsolicited-ack.js new file mode 100644 index 00000000000000..fa63e9ee3f6425 --- /dev/null +++ b/test/parallel/test-http2-settings-unsolicited-ack.js @@ -0,0 +1,50 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const http2 = require('http2'); +const net = require('net'); +const http2util = require('../common/http2'); +const Countdown = require('../common/countdown'); + +// Test that an unsolicited settings ack is ignored. + +const kSettings = new http2util.SettingsFrame(); +const kSettingsAck = new http2util.SettingsFrame(true); + +const server = http2.createServer(); +let client; + +const countdown = new Countdown(3, () => { + client.destroy(); + server.close(); +}); + +server.on('stream', common.mustNotCall()); +server.on('session', common.mustCall((session) => { + session.on('remoteSettings', common.mustCall(() => countdown.dec())); +})); + +server.listen(0, common.mustCall(() => { + client = net.connect(server.address().port); + + // Ensures that the clients settings frames are not sent until the + // servers are received, so that the first ack is actually expected. + client.once('data', (chunk) => { + // The very first chunk of data we get from the server should + // be a settings frame. + assert.deepStrictEqual(chunk.slice(0, 9), kSettings.data); + // The first ack is expected. + client.write(kSettingsAck.data, () => countdown.dec()); + // The second one is not and will be ignored. + client.write(kSettingsAck.data, () => countdown.dec()); + }); + + client.on('connect', common.mustCall(() => { + client.write(http2util.kClientMagic); + client.write(kSettings.data); + })); +})); diff --git a/test/sequential/sequential.status b/test/sequential/sequential.status index cf4763b00129cc..b95db2a111ea67 100644 --- a/test/sequential/sequential.status +++ b/test/sequential/sequential.status @@ -11,6 +11,8 @@ test-inspector-async-call-stack : PASS, FLAKY test-inspector-bindings : PASS, FLAKY test-inspector-debug-end : PASS, FLAKY test-inspector-async-hook-setup-at-signal: PASS, FLAKY +test-http2-ping-flood : PASS, FLAKY +test-http2-settings-flood : PASS, FLAKY [$system==linux] diff --git a/test/sequential/test-http2-ping-flood.js b/test/sequential/test-http2-ping-flood.js new file mode 100644 index 00000000000000..5b47d51be9c5a8 --- /dev/null +++ b/test/sequential/test-http2-ping-flood.js @@ -0,0 +1,56 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); +const net = require('net'); +const http2util = require('../common/http2'); + +// Test that ping flooding causes the session to be torn down + +const kSettings = new http2util.SettingsFrame(); +const kPing = new http2util.PingFrame(); + +const server = http2.createServer(); + +server.on('stream', common.mustNotCall()); +server.on('session', common.mustCall((session) => { + session.on('error', common.expectsError({ + code: 'ERR_HTTP2_ERROR', + message: + 'Flooding was detected in this HTTP/2 session, and it must be closed' + })); + session.on('close', common.mustCall(() => { + server.close(); + })); +})); + +server.listen(0, common.mustCall(() => { + const client = net.connect(server.address().port); + + // nghttp2 uses a limit of 10000 items in it's outbound queue. + // If this number is exceeded, a flooding error is raised. Set + // this lim higher to account for the ones that nghttp2 is + // successfully able to respond to. + // TODO(jasnell): Unfortunately, this test is inherently flaky because + // it is entirely dependent on how quickly the server is able to handle + // the inbound frames and whether those just happen to overflow nghttp2's + // outbound queue. The threshold at which the flood error occurs can vary + // from one system to another, and from one test run to another. + client.on('connect', common.mustCall(() => { + client.write(http2util.kClientMagic, () => { + client.write(kSettings.data, () => { + for (let n = 0; n < 35000; n++) + client.write(kPing.data); + }); + }); + })); + + // An error event may or may not be emitted, depending on operating system + // and timing. We do not really care if one is emitted here or not, as the + // error on the server side is what we are testing for. Do not make this + // a common.mustCall() and there's no need to check the error details. + client.on('error', () => {}); +})); diff --git a/test/sequential/test-http2-settings-flood.js b/test/sequential/test-http2-settings-flood.js new file mode 100644 index 00000000000000..bad4cec9a8d509 --- /dev/null +++ b/test/sequential/test-http2-settings-flood.js @@ -0,0 +1,53 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); +const net = require('net'); +const http2util = require('../common/http2'); + +// Test that settings flooding causes the session to be torn down + +const kSettings = new http2util.SettingsFrame(); + +const server = http2.createServer(); + +server.on('stream', common.mustNotCall()); +server.on('session', common.mustCall((session) => { + session.on('error', common.expectsError({ + code: 'ERR_HTTP2_ERROR', + message: + 'Flooding was detected in this HTTP/2 session, and it must be closed' + })); + session.on('close', common.mustCall(() => { + server.close(); + })); +})); + +server.listen(0, common.mustCall(() => { + const client = net.connect(server.address().port); + + // nghttp2 uses a limit of 10000 items in it's outbound queue. + // If this number is exceeded, a flooding error is raised. Set + // this lim higher to account for the ones that nghttp2 is + // successfully able to respond to. + // TODO(jasnell): Unfortunately, this test is inherently flaky because + // it is entirely dependent on how quickly the server is able to handle + // the inbound frames and whether those just happen to overflow nghttp2's + // outbound queue. The threshold at which the flood error occurs can vary + // from one system to another, and from one test run to another. + client.on('connect', common.mustCall(() => { + client.write(http2util.kClientMagic, () => { + for (let n = 0; n < 35000; n++) + client.write(kSettings.data); + }); + })); + + // An error event may or may not be emitted, depending on operating system + // and timing. We do not really care if one is emitted here or not, as the + // error on the server side is what we are testing for. Do not make this + // a common.mustCall() and there's no need to check the error details. + client.on('error', () => {}); +})); From 526cf84f2e5b34d7be259128f2453ba7e3f5b067 Mon Sep 17 00:00:00 2001 From: sreepurnajasti Date: Fri, 29 Dec 2017 10:25:31 +0530 Subject: [PATCH 26/27] doc: correct spelling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/17911 Reviewed-By: James M Snell Reviewed-By: Rich Trott Reviewed-By: Daniel Bevenius Reviewed-By: Luigi Pinca Reviewed-By: Gibson Fahnestock Reviewed-By: Tobias Nießen Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater Reviewed-By: Gireesh Punathil Reviewed-By: Weijia Wang --- doc/api/errors.md | 8 ++++---- doc/api/http2.md | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 222a93c1f8d0d8..73fd1d52cbeedc 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -890,7 +890,7 @@ An operation was performed on a stream that had already been destroyed. ### ERR_HTTP2_MAX_PENDING_SETTINGS_ACK Whenever an HTTP/2 `SETTINGS` frame is sent to a connected peer, the peer is -required to send an acknowledgement that it has received and applied the new +required to send an acknowledgment that it has received and applied the new `SETTINGS`. By default, a maximum number of unacknowledged `SETTINGS` frames may be sent at any given time. This error code is used when that limit has been reached. @@ -916,7 +916,7 @@ forbidden. ### ERR_HTTP2_PING_CANCEL -An HTTP/2 ping was cancelled. +An HTTP/2 ping was canceled. ### ERR_HTTP2_PING_LENGTH @@ -1249,11 +1249,11 @@ A failure occurred resolving imports in an [ES6 module][]. ### ERR_MULTIPLE_CALLBACK -A callback was called more then once. +A callback was called more than once. *Note*: A callback is almost always meant to only be called once as the query can either be fulfilled or rejected but not both at the same time. The latter -would be possible by calling a callback more then once. +would be possible by calling a callback more than once. ### ERR_NAPI_CONS_FUNCTION diff --git a/doc/api/http2.md b/doc/api/http2.md index 06734e9b78f7c0..9ef3fe27413ab7 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -191,7 +191,7 @@ the handler function will receive three arguments: added: v8.4.0 --> -The `'localSettings'` event is emitted when an acknowledgement SETTINGS frame +The `'localSettings'` event is emitted when an acknowledgment SETTINGS frame has been received. When invoked, the handler function will receive a copy of the local settings. @@ -339,7 +339,7 @@ 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 +If there are any remaining open `Http2Streams` associated with the `Http2Session`, those will also be destroyed. #### http2session.destroyed @@ -406,7 +406,7 @@ added: v8.4.0 * Value: {boolean} Indicates whether or not the `Http2Session` is currently waiting for an -acknowledgement for a sent SETTINGS frame. Will be `true` after calling the +acknowledgment for a sent SETTINGS frame. Will be `true` after calling the `http2session.settings()` method. Will be `false` once all sent SETTINGS frames have been acknowledged. @@ -428,12 +428,12 @@ The maximum number of outstanding (unacknowledged) pings is determined by the If provided, the `payload` must be a `Buffer`, `TypedArray`, or `DataView` containing 8 bytes of data that will be transmitted with the `PING` and -returned with the ping acknowledgement. +returned with the ping acknowledgment. The callback will be invoked with three arguments: an error argument that will be `null` if the `PING` was successfully acknowledged, a `duration` argument that reports the number of milliseconds elapsed since the ping was sent and the -acknowledgement was received, and a `Buffer` containing the 8-byte `PING` +acknowledgment was received, and a `Buffer` containing the 8-byte `PING` payload. ```js @@ -569,8 +569,8 @@ while the session is waiting for the remote peer to acknowledge the new settings. *Note*: The new settings will not become effective until the SETTINGS -acknowledgement is received and the `'localSettings'` event is emitted. It -is possible to send multiple SETTINGS frames while acknowledgement is still +acknowledgment is received and the `'localSettings'` event is emitted. It +is possible to send multiple SETTINGS frames while acknowledgment is still pending. #### http2session.type @@ -873,7 +873,7 @@ added: v8.4.0 --> The `'timeout'` event is emitted after no activity is received for this -`'Http2Stream'` within the number of millseconds set using +`'Http2Stream'` within the number of milliseconds set using `http2stream.setTimeout()`. #### Event: 'trailers' From d7dd941d93a1e140913e6549bde682b000a6cc07 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 8 Jan 2018 21:05:14 -0500 Subject: [PATCH 27/27] perf_hooks: fix scheduling regression Scheduling a PerformanceGCCallback should not keep the loop alive but due to the recent switch to using the native SetImmediate method, it does. Go back to using uv_async_t and add a regression test. PR-URL: https://github.com/nodejs/node/pull/18051 Fixes: https://github.com/nodejs/node/issues/18047 Refs: https://github.com/nodejs/node/pull/18020 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen --- src/node_perf.cc | 24 +++++++++++++++++------- test/parallel/test-performance-gc.js | 10 ++++++++++ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/node_perf.cc b/src/node_perf.cc index 6a59d50fb64ff3..97d3a2d99522fe 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -182,8 +182,9 @@ void SetupPerformanceObservers(const FunctionCallbackInfo& args) { } // Creates a GC Performance Entry and passes it to observers -void PerformanceGCCallback(Environment* env, void* ptr) { - GCPerformanceEntry* entry = static_cast(ptr); +void PerformanceGCCallback(uv_async_t* handle) { + GCPerformanceEntry* entry = static_cast(handle->data); + Environment* env = entry->env(); HandleScope scope(env->isolate()); Local context = env->context(); @@ -200,6 +201,10 @@ void PerformanceGCCallback(Environment* env, void* ptr) { } delete entry; + auto closeCB = [](uv_handle_t* handle) { + delete reinterpret_cast(handle); + }; + uv_close(reinterpret_cast(handle), closeCB); } // Marks the start of a GC cycle @@ -216,11 +221,16 @@ void MarkGarbageCollectionEnd(Isolate* isolate, v8::GCCallbackFlags flags, void* data) { Environment* env = static_cast(data); - env->SetImmediate(PerformanceGCCallback, - new GCPerformanceEntry(env, - static_cast(type), - performance_last_gc_start_mark_, - PERFORMANCE_NOW())); + uv_async_t* async = new uv_async_t(); + if (uv_async_init(env->event_loop(), async, PerformanceGCCallback)) + return delete async; + uv_unref(reinterpret_cast(async)); + async->data = + new GCPerformanceEntry(env, + static_cast(type), + performance_last_gc_start_mark_, + PERFORMANCE_NOW()); + CHECK_EQ(0, uv_async_send(async)); } diff --git a/test/parallel/test-performance-gc.js b/test/parallel/test-performance-gc.js index 89a9041c1c1159..ec0714ea50034d 100644 --- a/test/parallel/test-performance-gc.js +++ b/test/parallel/test-performance-gc.js @@ -51,3 +51,13 @@ const kinds = [ // Keep the event loop alive to witness the GC async callback happen. setImmediate(() => setImmediate(() => 0)); } + +// GC should not keep the event loop alive +{ + let didCall = false; + process.on('beforeExit', () => { + assert(!didCall); + didCall = true; + global.gc(); + }); +}