FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: refactor ping + settings object lifetime management by addaleax · Pull Request #28150 · nodejs/node · GitHub

/ node Public

http2: refactor ping + settings object lifetime management - #28150

Closed
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:http2-ping-settings
ConversationCommits (2)ChecksFiles changed
Closed

http2: refactor ping + settings object lifetime management#28150
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:http2-ping-settings

Conversation

addaleax commented Jun 10, 2019

Copy link
Copy Markdown
Member

Have clearer ownership relations between the Http2Ping,
Http2Settings and Http2Session objects.

Ping and Settings objects are now owned by the Http2Session
instance, and deleted along with it, so neither type of object
refers to the session after it is gone.
In the case of Http2Pings, that deletion is slightly delayed,
so we explicitly reset its session_ property.

Fixes: #28088

@nodejs/http2

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

http2: refactor ping + settings object lifetime management
d85020b 
Have clearer ownership relations between the `Http2Ping`,
`Http2Settings` and `Http2Session` objects.

Ping and Settings objects are now owned by the `Http2Session`
instance, and deleted along with it, so neither type of object
refers to the session after it is gone.
In the case of `Http2Ping`s, that deletion is slightly delayed,
so we explicitly reset its `session_` property.

Fixes: nodejs#28088

nodejs-github-bot commented Jun 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Sadly, an error occurred when I tried to trigger a build. :(
CI: https://ci.nodejs.org/job/node-test-pull-request/23827/
CI: https://ci.nodejs.org/job/node-test-pull-request/23840/
CI: https://ci.nodejs.org/job/node-test-pull-request/23860/

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 Jun 10, 2019
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 Jun 10, 2019

nodejs-github-bot commented Jun 10, 2019

Copy link
Copy Markdown
Collaborator

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

mcollina approved these changes Jun 10, 2019
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 10, 2019

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

Trott commented Jun 13, 2019

Copy link
Copy Markdown
Member

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

gengjiawen reviewed Jun 13, 2019
View reviewed changes
Comment thread test/parallel/test-http2-ping-settings-heapdump.js
const http2 = require('http2');
const v8 = require('v8');

// Regression test for https://github.com/nodejs/node/issues/28088:

gengjiawen Jun 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks like the last : is redundant here.

addaleax Jun 17, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it makes sense, given that a short explanation of the issue follows?

gengjiawen Jun 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe add a space. I think the link is necessary too. but : will make IDEs link the wrong url (Include github code review).

Trott commented Jun 13, 2019

Copy link
Copy Markdown
Member

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

(Failure was one of the test-cpu-prof* tests. Second one I've seen in the last 10 minutes. I'm still on Team Move-CPU-Profiling-Tests-To-Sequential!)

joyeecheung mentioned this pull request Jun 16, 2019
Closed

nodejs-github-bot commented Jun 17, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jun 17, 2019

Copy link
Copy Markdown
Collaborator

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

Trott commented Jun 17, 2019

Copy link
Copy Markdown
Member

Landed in 2a9f1ad

Trott closed this Jun 17, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 17, 2019
http2: refactor ping + settings object lifetime management
2a9f1ad 
Have clearer ownership relations between the `Http2Ping`,
`Http2Settings` and `Http2Session` objects.

Ping and Settings objects are now owned by the `Http2Session`
instance, and deleted along with it, so neither type of object
refers to the session after it is gone.
In the case of `Http2Ping`s, that deletion is slightly delayed,
so we explicitly reset its `session_` property.

Fixes: nodejs#28088

PR-URL: nodejs#28150
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jul 2, 2019
http2: refactor ping + settings object lifetime management
d0de204 
Have clearer ownership relations between the `Http2Ping`,
`Http2Settings` and `Http2Session` objects.

Ping and Settings objects are now owned by the `Http2Session`
instance, and deleted along with it, so neither type of object
refers to the session after it is gone.
In the case of `Http2Ping`s, that deletion is slightly delayed,
so we explicitly reset its `session_` property.

Fixes: #28088

PR-URL: #28150
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos mentioned this pull request Jul 2, 2019
Merged
This was referenced Jul 3, 2019
Closed
Closed
Closed
kissofire mentioned this pull request Jul 22, 2022
Open
wedataintelligence mentioned this pull request Jul 22, 2022
Open
ron-a20 mentioned this pull request Jul 22, 2022
Open
Dmarch28 mentioned this pull request Jul 22, 2022
Open
aliceUnhinged613 mentioned this pull request Jul 22, 2022
Open
137717unity mentioned this pull request Jul 22, 2022
Open
baby636 mentioned this pull request Jul 22, 2022
Open
molllyn1 mentioned this pull request Jul 23, 2022
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gengjiawen gengjiawen left review comments
mcollina mcollina approved these changes
jasnell jasnell approved these changes
Trott Trott approved these changes
+1 more reviewer
ryzokuken ryzokuken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

core dumped when takeHeapSnapshot for http2 server and memory leak

7 participants

Back | FazBrowse Home | New Git URL