Skip to content

Conversation

@exoego
Copy link
Contributor

@exoego exoego commented Oct 3, 2019

assert.strictEqual(typeof state.localClose, 'number');
assert.strictEqual(typeof state.remoteClose, 'number');

Type of localClose and remoteClose are number, so I think 1 is valid instead of true.

Checklist

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Oct 3, 2019
* `localClose` {number} `true` if this `Http2Stream` has been closed locally.
* `remoteClose` {number} `true` if this `Http2Stream` has been closed
* `localClose` {number} `1` if this `Http2Stream` has been closed locally.
* `remoteClose` {number} `1` if this `Http2Stream` has been closed
Copy link
Member

@Trott Trott Oct 3, 2019

Choose a reason for hiding this comment

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

If I'm reading the nghttp2 source correctly, it's 1 if remote peer half closed the given stream, 0 if it did not, and -1 if no such stream exists. Same for localClose. Is it possible to get -1 for these properties in Node.js?

Once the possible values are determined, tests should be augmented to check those values are returned in the expected situations. That doesn't have to happen in this PR but it would be a nice addition for sure.

Copy link
Member

Choose a reason for hiding this comment

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

@nodejs/http2

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Despite my suggested improvement, this is an improvement as it is already so I'm going to land it. My additional suggestions can happen some other time (or not).

@nodejs-github-bot

This comment has been minimized.

@Trott
Copy link
Member

Trott commented Oct 11, 2019

@Trott
Copy link
Member

Trott commented Oct 11, 2019

Landed in 3aeae8d

@Trott Trott closed this Oct 11, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 11, 2019
targos pushed a commit that referenced this pull request Oct 14, 2019
PR-URL: #29828
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants