http2: keep session objects alive during Http2Scope #17863

addaleax · 2017-12-25T19:22:48Z

Keep a local handle as a reference to the JS Http2Session
object so that it will not be garbage collected
when inside an Http2Scope, because the presence of the
latter usually indicates that further actions on
the session object are expected.

Strictly speaking, storing the session_handle_ as a
property on the scope object is not necessary, but
this is not very costly and makes the code more
obviously correct.

Fixes: #17840

Checklist

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

Affected core subsystem(s)

@nodejs/http2

Keep a local handle as a reference to the JS `Http2Session` object so that it will not be garbage collected when inside an `Http2Scope`, because the presence of the latter usually indicates that further actions on the session object are expected. Strictly speaking, storing the `session_handle_` as a property on the scope object is not necessary, but this is not very costly and makes the code more obviously correct. Fixes: nodejs#17840

apapirovski · 2017-12-25T19:42:00Z

src/node_http2.cc

@@ -71,6 +71,11 @@ Http2Scope::Http2Scope(Http2Session* session) {
  }
  session->flags_ |= SESSION_STATE_HAS_SCOPE;
  session_ = session;
+
+  // Always keep the session object alive for at least as long as the
+  // this scope is active.


extra "the" or "this".

@apapirovski yup, thanks for catching! 😄

addaleax · 2017-12-27T15:34:27Z

CI: https://ci.nodejs.org/job/node-test-commit/15089/

jasnell · 2017-12-28T00:03:21Z

Failure in CI is unrelated

PR-URL: #17863 Fixes: #17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Keep a local handle as a reference to the JS `Http2Session` object so that it will not be garbage collected when inside an `Http2Scope`, because the presence of the latter usually indicates that further actions on the session object are expected. Strictly speaking, storing the `session_handle_` as a property on the scope object is not necessary, but this is not very costly and makes the code more obviously correct. Fixes: #17840 PR-URL: #17863 Fixes: #17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

jasnell · 2017-12-28T00:18:46Z

Landed in 57594d2 and 0666630

PR-URL: nodejs#17863 Fixes: nodejs#17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Keep a local handle as a reference to the JS `Http2Session` object so that it will not be garbage collected when inside an `Http2Scope`, because the presence of the latter usually indicates that further actions on the session object are expected. Strictly speaking, storing the `session_handle_` as a property on the scope object is not necessary, but this is not very costly and makes the code more obviously correct. Fixes: nodejs#17840 PR-URL: nodejs#17863 Fixes: nodejs#17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Backport-PR-URL: #18050 PR-URL: #17863 Fixes: #17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Keep a local handle as a reference to the JS `Http2Session` object so that it will not be garbage collected when inside an `Http2Scope`, because the presence of the latter usually indicates that further actions on the session object are expected. Strictly speaking, storing the `session_handle_` as a property on the scope object is not necessary, but this is not very costly and makes the code more obviously correct. Fixes: #17840 Backport-PR-URL: #18050 PR-URL: #17863 Fixes: #17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Backport-PR-URL: #18050 PR-URL: #17863 Fixes: #17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Keep a local handle as a reference to the JS `Http2Session` object so that it will not be garbage collected when inside an `Http2Scope`, because the presence of the latter usually indicates that further actions on the session object are expected. Strictly speaking, storing the `session_handle_` as a property on the scope object is not necessary, but this is not very costly and makes the code more obviously correct. Fixes: #17840 Backport-PR-URL: #18050 PR-URL: #17863 Fixes: #17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17863 Fixes: nodejs#17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Keep a local handle as a reference to the JS `Http2Session` object so that it will not be garbage collected when inside an `Http2Scope`, because the presence of the latter usually indicates that further actions on the session object are expected. Strictly speaking, storing the `session_handle_` as a property on the scope object is not necessary, but this is not very costly and makes the code more obviously correct. Fixes: nodejs#17840 Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17863 Fixes: nodejs#17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17863 Fixes: #17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Keep a local handle as a reference to the JS `Http2Session` object so that it will not be garbage collected when inside an `Http2Scope`, because the presence of the latter usually indicates that further actions on the session object are expected. Strictly speaking, storing the `session_handle_` as a property on the scope object is not necessary, but this is not very costly and makes the code more obviously correct. Fixes: #17840 Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17863 Fixes: #17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

addaleax added 2 commits December 25, 2017 12:21

http2: fix compiling with --debug-http2

fab3773

addaleax requested review from apapirovski and jasnell December 25, 2017 19:22

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 Dec 25, 2017

addaleax mentioned this pull request Dec 25, 2017

Investigate test-http2-createwritereq failure on Linux and AIX #17840

Closed

addaleax added http2 Issues or PRs related to the http2 subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 25, 2017

apapirovski approved these changes Dec 25, 2017

View reviewed changes

[squash] apapirovski nit

fcd50be

jasnell approved these changes Dec 27, 2017

View reviewed changes

cjihrig approved these changes Dec 27, 2017

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 Dec 27, 2017

apapirovski mentioned this pull request Dec 27, 2017

tls: simplify write mechanism #17883

Closed

2 tasks

jasnell pushed a commit that referenced this pull request Dec 28, 2017

http2: fix compiling with --debug-http2

57594d2

PR-URL: #17863 Fixes: #17840 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

jasnell closed this Dec 28, 2017

joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 28, 2017

addaleax mentioned this pull request Dec 30, 2017

Investigate flaky test-http2-https-fallback #17888

Closed

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added the dont-land-on-v6.x label Jan 19, 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: keep session objects alive during Http2Scope #17863

http2: keep session objects alive during Http2Scope #17863

addaleax commented Dec 25, 2017

apapirovski Dec 25, 2017

addaleax Dec 25, 2017

addaleax commented Dec 27, 2017

jasnell commented Dec 28, 2017

jasnell commented Dec 28, 2017

http2: keep session objects alive during Http2Scope #17863

http2: keep session objects alive during Http2Scope #17863

Conversation

addaleax commented Dec 25, 2017

Checklist

Affected core subsystem(s)

apapirovski Dec 25, 2017

Choose a reason for hiding this comment

addaleax Dec 25, 2017

Choose a reason for hiding this comment

addaleax commented Dec 27, 2017

jasnell commented Dec 28, 2017

jasnell commented Dec 28, 2017