http2: implement maxSessionMemory #17967

jasnell · 2018-01-03T19:28:42Z

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.

@nodejs/http2 (@mcollina in particular)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http2

jasnell · 2018-01-03T19:32:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12398/

mcollina

LGTM

addaleax · 2018-01-03T19:37:11Z

src/node_http2.cc

+  // 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);


In that case you really should be specifying that you 1 MB is one million bytes, not 2^10 bytes ;)

@addaleax ... sorry, I'm being a bit thick... can you clarify the specific change? comment or code?

@jasnell Sorry, missed the notification – I meant, for some people 1 MB means 1.000.000 bytes, and for some it means 1.048.576 bytes, so it would be good to have disambiguated this in the documentation :)

addaleax · 2018-01-03T19:43:26Z

src/node_http2.h

@@ -1121,6 +1164,10 @@ class Http2Session::Http2Ping : public AsyncWrap {

  size_t self_size() const override { return sizeof(*this); }

+  uint64_t cost() const {
+    return sizeof(Http2Session::Http2Ping);
+  }


I think these cost() methods are essentially exactly the same as self_size(), so it might be nicer to keep using the existing methods…?

was considering that, kept them separate for now in case there were other bits that needed to be accounted for at some point. For instance, the cost() of a settings frame is the size of the object + the number of settings included.

For instance, the cost() of a settings frame is the size of the object + the number of settings included.

I think that’s a bug in the self_size() methods for settings then – we’re generally not very strict about self_size() being accurate, but as long as the semantics are “the size of the memory owned by this object”, they seem pretty much identical for both methods?

good point... hmm ok

addaleax · 2018-01-03T19:49:21Z

src/node_http2.h

+  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_);


I’m wondering what the cost of these calls is, since they can’t be inlined & this function is called quite a few times as far as I can tell? Did you try to measure that? (Not a blocker, I think this feature is worth it either way)

No, haven't benchmarked it yet.

One way we can limit the impact here is to capture the inbound dynamic table size at the completion of a headers frame, and the outbound dynamic size after sending a headers frame, then just add the static values.

(note that doing so relaxes one of the protections included here that keeps headers within a single HEADERS frame from blowing the memory limit)

jasnell · 2018-01-05T19:53:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12425/

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.

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: #17967 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

jasnell · 2018-01-05T20:38:05Z

Landed in 882e7ef.

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: nodejs#17967 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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. Backport-PR-URL: #18050 PR-URL: #17967 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

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. Backport-PR-URL: #18050 PR-URL: #17967 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 - Refactoring and cleanup of Http2Session and Http2Stream destroy (James M Snell) #17406 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

MylesBorins · 2018-02-27T04:43:38Z

This does not land cleanly on v8.x, could we get a backport?

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. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17967 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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. Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17967 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

jasnell added the http2 Issues or PRs related to the http2 subsystem. label Jan 3, 2018

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 3, 2018

mcollina approved these changes Jan 3, 2018

View reviewed changes

addaleax approved these changes Jan 3, 2018

View reviewed changes

jasnell force-pushed the http2-maxsessionmemory branch from 833c97f to ae80dc8 Compare January 5, 2018 20:34

jasnell closed this Jan 5, 2018

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added dont-land-on-v6.x labels Feb 27, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: implement maxSessionMemory #17967

http2: implement maxSessionMemory #17967

jasnell commented Jan 3, 2018

jasnell commented Jan 3, 2018

mcollina left a comment

addaleax Jan 3, 2018

jasnell Jan 4, 2018

addaleax Jan 7, 2018

addaleax Jan 3, 2018

jasnell Jan 3, 2018

addaleax Jan 3, 2018

jasnell Jan 3, 2018

addaleax Jan 3, 2018

jasnell Jan 3, 2018

jasnell Jan 3, 2018

jasnell Jan 3, 2018

jasnell commented Jan 5, 2018

jasnell commented Jan 5, 2018

MylesBorins commented Feb 27, 2018

http2: implement maxSessionMemory #17967

http2: implement maxSessionMemory #17967

Conversation

jasnell commented Jan 3, 2018

Checklist

Affected core subsystem(s)

jasnell commented Jan 3, 2018

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jan 5, 2018

jasnell commented Jan 5, 2018

MylesBorins commented Feb 27, 2018