Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: refactor ping + settings object lifetime management #28150

Closed
wants to merge 2 commits into from

Conversation

@addaleax
Copy link
Member

commented Jun 10, 2019

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
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
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@mcollina
Copy link
Member

left a comment

LGTM

@nodejs-github-bot

This comment was marked as outdated.

@Trott

This comment was marked as outdated.

@Trott

This comment has been minimized.

const http2 = require('http2');
const v8 = require('v8');

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

This comment has been minimized.

Copy link
@gengjiawen

gengjiawen Jun 13, 2019

Member

Looks like the last : is redundant here.

This comment has been minimized.

Copy link
@addaleax

addaleax Jun 17, 2019

Author Member

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

This comment has been minimized.

Copy link
@gengjiawen

gengjiawen Jun 17, 2019

Member

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

@Trott

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

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!)

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Jun 17, 2019
@Trott

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Landed in 2a9f1ad

@Trott Trott closed this Jun 17, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jun 17, 2019
http2: refactor ping + settings object lifetime management
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 added a commit that referenced this pull request Jul 2, 2019
http2: refactor ping + settings object lifetime management
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 targos referenced this pull request Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.