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.

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

http2: tidying up in core.js #17209

Closed
wants to merge 9 commits into from
Closed

http2: tidying up in core.js #17209

wants to merge 9 commits into from

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
Member

@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
@@ -366,7 +366,8 @@ function onSelectPadding(fn) {
// will be deferred until the socket is ready to go.
function requestOnConnect(headers, options) {
const session = this[kSession];
debug(`[${sessionName(session[kType])}] connected.. initializing request`);
debug(`HttpSession ${sessionName(session[kType])}: connected, ` +
Copy link
Contributor

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
@@ -1004,13 +1011,15 @@ class ClientHttp2Session extends Http2Session {
const state = this[kState];
if (state.destroyed || state.destroying)
throw new errors.Error('ERR_HTTP2_INVALID_SESSION');
debug(`[${sessionName(this[kType])}] initiating request`);
_unrefActive(this);
debug(`HttpSession ${sessionName(this[kType])}: initiating request`);
Copy link
Contributor

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

@maclover7 maclover7 maclover7 left review comments

@apapirovski apapirovski apapirovski approved these changes

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