FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: add Http2Stream.bufferSize by oyyd · Pull Request #23711 · nodejs/node · GitHub

/ node Public

http2: add Http2Stream.bufferSize - #23711

Closed
oyyd wants to merge 2 commits into
nodejs:masterfrom
oyyd:http2-stream-buffersize
ConversationCommits (2)ChecksFiles changed
Closed

http2: add Http2Stream.bufferSize#23711
oyyd wants to merge 2 commits into
nodejs:masterfrom
oyyd:http2-stream-buffersize

Conversation

oyyd commented Oct 17, 2018

Copy link
Copy Markdown
Contributor

This commit adds bufferSize for Http2Stream.

Refs: #21631

/cc @jasnell @addaleax @apapirovski

From the code, I believe the writeQueueSize in http2 module could represent the kLastWriteQueueSize in net module:

node/lib/net.js

Line 518 in deaddd2

return this[kLastWriteQueueSize] + this.writableLength;

and the tests following this PR seems okay. Can you help to confirm that?

BTW, we can add Http2Session.bufferSize basing on this which is what #21631 exactly requested.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added dont-land-on-v6.x http2 Issues or PRs related to the http2 subsystem. labels Oct 17, 2018
http2: add Http2Stream.bufferSize
d7d9307 
This commit adds `bufferSize` for `Http2Stream`.

Refs: nodejs#21631
oyyd force-pushed the http2-stream-buffersize branch from f2264e6 to d7d9307 Compare October 17, 2018 12:39
jasnell reviewed Oct 17, 2018
View reviewed changes
Comment thread lib/internal/http2/core.js
get bufferSize() {
// `bufferSize` properties of `net.Socket` are `undefined` when
// their `_handle` are falsy. Here we avoid the behavior.
return this[kState].writeQueueSize + this.writableLength;

jasnell Oct 17, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

At best this is likely an approximation with a high degree of accuracy at any given time. It's likely good enough :-)

What do you think @addaleax?

addaleax Oct 24, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I’d guess it’s good enough, yes :)

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 17, 2018
vsemozhetbyt reviewed Oct 17, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated
Set to `true` if the `Http2Stream` instance was aborted abnormally. When set,
the `'aborted'` event will have been emitted.

### http2stream.bufferSize

vsemozhetbyt Oct 17, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit:

Suggested change
### http2stream.bufferSize
#### http2stream.bufferSize

vsemozhetbyt reviewed Oct 17, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated
[`net.Socket.prototype.ref()`]: net.html#net_socket_ref
[`net.Socket.prototype.unref()`]: net.html#net_socket_unref
[`net.connect()`]: net.html#net_net_connect
[`net.Socket.bufferSize`]: net.html#net_socket_buffersize

vsemozhetbyt Oct 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: should go after [`net.Socket`]: net.html#net_class_net_socket (in ASCII sort order).

oyyd Oct 18, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks! Done.

BridgeAR commented Oct 19, 2018

Copy link
Copy Markdown
Member

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

oyyd commented Oct 20, 2018

Copy link
Copy Markdown
Contributor Author

The test, parallel/test-tls-alert-handling, failed seems unrelated.

lundibundi commented Oct 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18008/ ✅

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 6, 2018
http2: add Http2Stream.bufferSize
33fbb93 
This commit adds `bufferSize` for `Http2Stream`.

Refs: nodejs#21631

PR-URL: nodejs#23711
Reviewed-By: James M Snell <jasnell@gmail.com>

Trott commented Nov 6, 2018

Copy link
Copy Markdown
Member

Landed in 33fbb93

Trott closed this Nov 6, 2018
targos pushed a commit that referenced this pull request Nov 6, 2018
http2: add Http2Stream.bufferSize
4a0f276 
This commit adds `bufferSize` for `Http2Stream`.

Refs: #21631

PR-URL: #23711
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR mentioned this pull request Nov 14, 2018
Merged
This was referenced Nov 15, 2018
Closed
Closed
Closed
Closed
Closed
Closed
BethGriggs pushed a commit that referenced this pull request Apr 8, 2019
http2: add Http2Stream.bufferSize
4f60364 
This commit adds `bufferSize` for `Http2Stream`.

Refs: #21631

PR-URL: #23711
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs mentioned this pull request May 1, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
beevelop mentioned this pull request May 29, 2019
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax left review comments
jasnell jasnell approved these changes
+1 more reviewer
vsemozhetbyt vsemozhetbyt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http2 Issues or PRs related to the http2 subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL