Skip to content

http2: tidying up in core.js#17209

Closed
jasnell wants to merge 9 commits intonodejs:masterfrom
jasnell:http2-refactor3
Closed

http2: tidying up in core.js#17209
jasnell wants to merge 9 commits intonodejs:masterfrom
jasnell:http2-refactor3

Conversation

@jasnell
Copy link
Member

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

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

@apapirovski apapirovski 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
Member Author

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

@maclover7 maclover7 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

@maclover7 maclover7 Nov 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

@maclover7 maclover7 Nov 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
Member Author

jasnell commented Nov 22, 2017

@maclover7 ... fixed :-)

@jasnell
Copy link
Member Author

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
Member Author

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 <apapirovski@mac.com>
@jasnell
Copy link
Member Author

jasnell commented Nov 24, 2017

Landed in 9d87082

@jasnell jasnell 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 <apapirovski@mac.com>
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 <apapirovski@mac.com>
@MylesBorins MylesBorins 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 <apapirovski@mac.com>
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 <apapirovski@mac.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@maclover7 maclover7 maclover7 left review comments

@apapirovski apapirovski apapirovski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

4 participants

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