http2: fix session memory accounting after pausing by mikelehen · Pull Request #30684 · nodejs/node

mikelehen · 2019-11-27T17:23:06Z

The ability to pause input processing was added in 8a4a193 but
introduced a session memory accounting mismatch leading to potential
NGHTTP2_ENHANCE_YOUR_CALM errors.

After pausing
(

node/src/node_http2.cc

Line 871 in f36331c

// Mark the remainder of the data as available for later consumption.

),
the early return on line 873 skips the
DecrementCurrentSessionMemory(stream_buf_.len) call below (line 878).

When we later finished processing the input chunk
(

node/src/node_http2.cc

Line 1858 in f36331c

if (UNLIKELY(stream_buf_offset_ > 0)) {

),
we were calling DecrementCurrentSessionMemory(stream_buf_offset_) [line
1875] which was a no-op since we just set stream_buf_offset_ to 0 [line
1873].

The correct amount to decrement by is still stream_buf_.len, since
that's the amount we skipped previously (line 878).

Fixes: #29223
Refs: 164ac5b

Checklist

make -j4 test (UNIX)
tests and/or benchmarks are included
commit message follows commit guidelines

The ability to pause input processing was added in 8a4a193 but introduced a session memory accounting mismatch leading to potential NGHTTP2_ENHANCE_YOUR_CALM errors. After pausing (https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L871), the early return on line 873 skips the DecrementCurrentSessionMemory(stream_buf_.len) call below (line 878). When we later finished processing the input chunk (https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L1858), we were calling DecrementCurrentSessionMemory(stream_buf_offset_) [line 1875] which was a no-op since we just set stream_buf_offset_ to 0 [line 1873]. The correct amount to decrement by is still stream_buf_.len, since that's the amount we skipped previously (line 878). Fixes: nodejs#29223 Refs: nodejs@164ac5b

src/node_http2.cc

addaleax

LGTM

I’m good with landing this and then we can rebase #30351 against if if we want to?

lundibundi · 2019-11-29T16:01:47Z

@addaleax yeah, let's land this first, I'll then update mine.

addaleax · 2019-11-29T16:04:10Z

@lundibundi Just to be clear, this PR LGTY? It could land pretty soon then :)

nodejs-github-bot · 2019-11-29T16:04:45Z

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

nodejs-github-bot · 2019-11-30T08:13:38Z

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

The ability to pause input processing was added in 8a4a193 but introduced a session memory accounting mismatch leading to potential NGHTTP2_ENHANCE_YOUR_CALM errors. After pausing (https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L871), the early return on line 873 skips the DecrementCurrentSessionMemory(stream_buf_.len) call below (line 878). When we later finished processing the input chunk (https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L1858), we were calling DecrementCurrentSessionMemory(stream_buf_offset_) [line 1875] which was a no-op since we just set stream_buf_offset_ to 0 [line 1873]. The correct amount to decrement by is still stream_buf_.len, since that's the amount we skipped previously (line 878). Fixes: #29223 Refs: 164ac5b PR-URL: #30684 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

addaleax · 2019-11-30T14:51:17Z

Landed in 18a1796, thanks for the PR!

The ability to pause input processing was added in 8a4a193 but introduced a session memory accounting mismatch leading to potential NGHTTP2_ENHANCE_YOUR_CALM errors. After pausing (https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L871), the early return on line 873 skips the DecrementCurrentSessionMemory(stream_buf_.len) call below (line 878). When we later finished processing the input chunk (https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L1858), we were calling DecrementCurrentSessionMemory(stream_buf_offset_) [line 1875] which was a no-op since we just set stream_buf_offset_ to 0 [line 1873]. The correct amount to decrement by is still stream_buf_.len, since that's the amount we skipped previously (line 878). Fixes: #29223 Refs: 164ac5b PR-URL: #30684 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

mikelehen · 2019-11-30T19:34:45Z

Awesome! Thanks for the quick reviewing and merging!

mikelehen · 2019-12-02T18:10:01Z

As a minor follow-up, since the original bug affects Node.js 10.x (c152449), I think this fix needs to be back-ported as well. Is that an automatic thing? Or is there some way to "request" the back-port (or submit a PR that will help get it into 10.x)? Thanks!

MylesBorins · 2019-12-05T00:49:32Z

/cc @nodejs/backporters

MylesBorins · 2019-12-05T00:52:03Z

FWIW this lands cleanly on both v10.x and v12.x

@BethGriggs could we get this in the next 10.x and 12.x? Considering this is a pretty nasty bug it might make sense to delay both those releases by a week

thoughts?

/cc @nodejs/lts

addaleax · 2019-12-05T01:01:17Z

I don’t think we need to delay releases for a low-risk patch like this.

The ability to pause input processing was added in 8a4a193 but introduced a session memory accounting mismatch leading to potential NGHTTP2_ENHANCE_YOUR_CALM errors. After pausing (https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L871), the early return on line 873 skips the DecrementCurrentSessionMemory(stream_buf_.len) call below (line 878). When we later finished processing the input chunk (https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L1858), we were calling DecrementCurrentSessionMemory(stream_buf_offset_) [line 1875] which was a no-op since we just set stream_buf_offset_ to 0 [line 1873]. The correct amount to decrement by is still stream_buf_.len, since that's the amount we skipped previously (line 878). Fixes: #29223 Refs: 164ac5b PR-URL: #30684 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

MylesBorins · 2019-12-05T17:37:30Z

@addaleax are you suggesting we get this in the next release or that we should land without waiting 2 weeks?

addaleax · 2019-12-05T23:13:05Z

@MylesBorins I’m suggesting including it in the upcoming releases, without any kind of waiting time; this is a very straightforward bug fix, and I’d treat it like that.

The ability to pause input processing was added in 8a4a193 but introduced a session memory accounting mismatch leading to potential NGHTTP2_ENHANCE_YOUR_CALM errors. After pausing (https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L871), the early return on line 873 skips the DecrementCurrentSessionMemory(stream_buf_.len) call below (line 878). When we later finished processing the input chunk (https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L1858), we were calling DecrementCurrentSessionMemory(stream_buf_offset_) [line 1875] which was a no-op since we just set stream_buf_offset_ to 0 [line 1873]. The correct amount to decrement by is still stream_buf_.len, since that's the amount we skipped previously (line 878). Fixes: #29223 Refs: 164ac5b PR-URL: #30684 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

- http2: fix session memory accounting after pausing (Michael Lehenbauer) [#30684](#30684) - n-api: correct bug in napi_get_last_error (Octavian Soldea) [#28702](#28702) PR-URL: #31248

- http2: fix session memory accounting after pausing (Michael Lehenbauer) [#30684](#30684) - n-api: correct bug in napi_get_last_error (Octavian Soldea) [#28702](#28702) - tools: update tzdata to 2019c (Myles Borins) [#30479](#30479) PR-URL: #31248

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. labels Nov 27, 2019

mikelehen mentioned this pull request Nov 27, 2019

NGHTTP2_ENHANCE_YOUR_CALM when starting with version 10.16.3 #29223

Closed

lundibundi reviewed Nov 27, 2019

View reviewed changes

src/node_http2.cc Show resolved Hide resolved

jasnell requested a review from addaleax November 27, 2019 18:05

addaleax approved these changes Nov 29, 2019

View reviewed changes

lundibundi mentioned this pull request Nov 29, 2019

http2: fix session memory tracking and small clean up #30351

Closed

3 tasks

lundibundi approved these changes Nov 29, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 29, 2019

jasnell approved these changes Nov 29, 2019

View reviewed changes

devnexen approved these changes Nov 30, 2019

View reviewed changes

addaleax closed this Nov 30, 2019

BridgeAR mentioned this pull request Dec 3, 2019

v13.3.0 proposal #30774

Merged

MylesBorins added lts-watch-v10.x labels Dec 5, 2019

MylesBorins mentioned this pull request Dec 5, 2019

v10.18.1 proposal #30796

Closed

BethGriggs mentioned this pull request Dec 9, 2019

v12.13.2 proposal #30865

Closed

MiLk mentioned this pull request Dec 13, 2019

Error: 8 RESOURCE_EXHAUSTED: Bandwidth exhausted grpc/grpc-node#1158

Closed

BethGriggs mentioned this pull request Dec 23, 2019

v12.14.1 proposal #31069

Merged

BethGriggs mentioned this pull request Jan 7, 2020

v10.18.1 proposal #31248

Merged

This was referenced Jan 30, 2020

Nodejs v13.0.1 HTTP2 NGHTTP2_ENHANCE_YOUR_CALM & NGHTTP2_INTERNAL_ERROR #30193

Closed

NodeJs 12.11.1 NGHTTP2_ENHANCE_YOUR_CALM #29907

Closed

BethGriggs added land-on-v10.x and removed lts-watch-v10.x labels Feb 27, 2020

lundibundi mentioned this pull request Jul 12, 2020

Revert "http2: streamline OnStreamRead streamline memory accounting" #34315

Closed

2 tasks

Uh oh!

Conversation

mikelehen commented Nov 27, 2019

Checklist

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

lundibundi commented Nov 29, 2019

Uh oh!

addaleax commented Nov 29, 2019

Uh oh!

nodejs-github-bot commented Nov 29, 2019

Uh oh!

nodejs-github-bot commented Nov 30, 2019

Uh oh!

addaleax commented Nov 30, 2019

Uh oh!

mikelehen commented Nov 30, 2019

Uh oh!

mikelehen commented Dec 2, 2019

Uh oh!

MylesBorins commented Dec 5, 2019

Uh oh!

MylesBorins commented Dec 5, 2019

Uh oh!

addaleax commented Dec 5, 2019

Uh oh!

MylesBorins commented Dec 5, 2019

Uh oh!

addaleax commented Dec 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants