Skip to content

http2: tidying up in core.js#17209

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.

Sign up for GitHub

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

Jump to bottom
Closed
jasnell wants to merge 9 commits into from
Closed

http2: tidying up in core.js #17209

jasnell wants to merge 9 commits into from

Conversation

@jasnell
Copy link
Member

@jasnelljasnell commented Nov 22, 2017

A number of cleanups and improvements in core.js

Getting these out of the way before working on a larger cleanup around the lifecycle and error handling in Http2Stream and Http2Session

ping @nodejs/http2

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 dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Nov 22, 2017
@jasnell
Copy link
MemberAuthor

jasnell commented Nov 22, 2017

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

apapirovski
apapirovski approved these changes Nov 22, 2017
View reviewed changes
Copy link
Contributor

@apapirovskiapapirovski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Big fan of kUpdateTimer, should limit the number of accidental timeout bugs. (It's easy to miss calling both _unrefActive as necessary.)

@jasnell
Copy link
MemberAuthor

jasnell commented Nov 22, 2017

btw, In a subsequent PR I'm going to be renaming the streamClosed event to close, per what is standard for streams in general

maclover7
maclover7 reviewed Nov 22, 2017
View reviewed changes
Copy link
Contributor

@maclover7maclover7 left a comment

Choose a reason for hiding this comment

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

two small copy paste errors :)

lib/internal/http2/core.js Outdated
Copy link
Contributor

@maclover7maclover7Nov 22, 2017

Choose a reason for hiding this comment

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

Http2Session

lib/internal/http2/core.js Outdated
Copy link
Contributor

@maclover7maclover7Nov 22, 2017

Choose a reason for hiding this comment

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

Http2Session

@jasnell
Copy link
MemberAuthor

jasnell commented Nov 22, 2017

@maclover7 ... fixed :-)

@jasnell
Copy link
MemberAuthor

jasnell commented Nov 22, 2017

osx and freebsd ci's failed because I pushed a new commit before those could run. Will re-run those soon

@jasnell
Copy link
MemberAuthor

jasnell commented Nov 24, 2017

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

jasnell added a commit that referenced this pull request Nov 24, 2017
@jasnell
http2: general cleanups in core.js
9d87082
* fixup js debug messages * simplify and improve rstStream * improve and simplify _read * simplify and improve priority * simplify on ready a bit * simplify and improve respond/push * reduce duplication with _unrefActive * simplify stream close handling PR-URL: #17209 Reviewed-By: Anatoli Papirovski <[email protected]>
@jasnell
Copy link
MemberAuthor

jasnell commented Nov 24, 2017

Landed in 9d87082

@jasnelljasnell closed this Nov 24, 2017
MylesBorins pushed a commit that referenced this pull request Dec 7, 2017
@jasnell@MylesBorins
http2: general cleanups in core.js
8442b53
* fixup js debug messages * simplify and improve rstStream * improve and simplify _read * simplify and improve priority * simplify on ready a bit * simplify and improve respond/push * reduce duplication with _unrefActive * simplify stream close handling PR-URL: #17209 Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 7, 2017
@jasnell@MylesBorins
http2: general cleanups in core.js
7ceccb1
* fixup js debug messages * simplify and improve rstStream * improve and simplify _read * simplify and improve priority * simplify on ready a bit * simplify and improve respond/push * reduce duplication with _unrefActive * simplify stream close handling PR-URL: #17209 Reviewed-By: Anatoli Papirovski <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 7, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Dec 8, 2017
@jasnell@MylesBorins
http2: general cleanups in core.js
bd035d7
* fixup js debug messages * simplify and improve rstStream * improve and simplify _read * simplify and improve priority * simplify on ready a bit * simplify and improve respond/push * reduce duplication with _unrefActive * simplify stream close handling PR-URL: #17209 Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 8, 2017
@jasnell@MylesBorins
http2: general cleanups in core.js
d3e2bf0
* fixup js debug messages * simplify and improve rstStream * improve and simplify _read * simplify and improve priority * simplify on ready a bit * simplify and improve respond/push * reduce duplication with _unrefActive * simplify stream close handling PR-URL: #17209 Reviewed-By: Anatoli Papirovski <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@maclover7maclover7maclover7 left review comments

@apapirovskiapapirovskiapapirovski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http2Issues or PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jasnell@apapirovski@maclover7@nodejs-github-bot