Skip to content

http2: convert Http2Settings to an AsyncWrap - #17763

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:http2-settings-async-req
Closed

http2: convert Http2Settings to an AsyncWrap#17763
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:http2-settings-async-req

Conversation

@jasnell

@jasnelljasnell commented Dec 19, 2017

Copy link
Copy Markdown
Member

Make Http2Settings an AsyncWrap.

Refs: #17746

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)

http2

@nodejs-github-botnodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 19, 2017
@jasnelljasnell mentioned this pull request Dec 19, 2017
Closed
11 tasks
@jasnell

jasnell commented Dec 21, 2017

Copy link
Copy Markdown
MemberAuthor

ping @mcollina @nodejs/http2

@jasnell
jasnellforce-pushed the http2-settings-async-req branch from 6596369 to 2ca60acCompareDecember 21, 2017 20:11
@jasnell

jasnell commented Dec 21, 2017

Copy link
Copy Markdown
MemberAuthor

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

mcollina
mcollina approved these changes Dec 21, 2017
View reviewed changes

@mcollinamcollina 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.

LGTM

mcollina
mcollina reviewed Dec 21, 2017
View reviewed changes
Comment threadlib/internal/http2/core.js Outdated

@mcollinamcollinaDec 21, 2017

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.

I would at least so a debug if ack is null.

@jasnelljasnellDec 21, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Just reworked the entire function a bit to clean it up more and add the debug output.

@jasnell

jasnell commented Dec 22, 2017

Copy link
Copy Markdown
MemberAuthor

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

@jasnell
jasnellforce-pushed the http2-settings-async-req branch from 2f4808b to 880bbeeCompareDecember 22, 2017 18:01
@jasnell

jasnell commented Dec 22, 2017

Copy link
Copy Markdown
MemberAuthor

Try again: https://ci.nodejs.org/job/node-test-pull-request/12264/

@jasnell
jasnellforce-pushed the http2-settings-async-req branch from bc4a14a to 788c847CompareDecember 22, 2017 20:10
@jasnell

jasnell commented Dec 22, 2017

Copy link
Copy Markdown
MemberAuthor

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

@jasnell

jasnell commented Dec 22, 2017

Copy link
Copy Markdown
MemberAuthor

CI is good

jasnell added a commit that referenced this pull request Dec 22, 2017
@jasnell
http2: convert Http2Settings to an AsyncWrap
bbaea12
PR-URL: #17763
Refs: #17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell

jasnell commented Dec 22, 2017

Copy link
Copy Markdown
MemberAuthor

Landed in bbaea12

@jasnelljasnell closed this Dec 22, 2017
MylesBorins pushed a commit to jasnell/node that referenced this pull request Jan 9, 2018
@jasnell@MylesBorins
http2: convert Http2Settings to an AsyncWrap
293c2f9
PR-URL: nodejs#17763
Refs: nodejs#17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@jasnell@MylesBorins
http2: convert Http2Settings to an AsyncWrap
098edcb
Backport-PR-URL: #18050
PR-URL: #17763
Refs: #17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Jan 10, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
@jasnell@MylesBorins
http2: convert Http2Settings to an AsyncWrap
3a6b2ad
Backport-PR-URL: #18050
PR-URL: #17763
Refs: #17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
@jasnell@kjin
http2: convert Http2Settings to an AsyncWrap
f68325c
Backport-PR-URL: nodejs#18050
PR-URL: nodejs#17763
Refs: nodejs#17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
@jasnell@MylesBorins
http2: convert Http2Settings to an AsyncWrap
0463221
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17763
Refs: #17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request May 2, 2018
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

Assignees

No one assigned

Labels

lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jasnell@mcollina@MylesBorins@nodejs-github-bot