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.

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

http2: window size connection control #26962

Closed
wants to merge 2 commits into from

Conversation

migounette
Copy link

Allow to control the connection window size by setting local window
size (local endpoints's window size) to the given window_size.

To increase window size, this function may submit WINDOW_UPDATE frame
to transmission queue.

Pay attention, this function takes the absolute value of window size to set,
rather than the delta, the delta is computed by the update operation.

Allow to control the connection window size by setting local window
size (local endpoints's window size) to the given window_size.

To increase window size, this function may submit WINDOW_UPDATE frame
to transmission queue.

Pay attention, this function takes the absolute value of window size to set,
rather than the delta, the delta is computed by the update operation.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 28, 2019
@vsemozhetbyt vsemozhetbyt added the http2 Issues or PRs related to the http2 subsystem. label Mar 28, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Apr 3, 2019

@nodejs/http2 PTAL

@nodejs-github-bot
Copy link
Collaborator

@@ -2222,6 +2249,7 @@ added: v8.4.0
| `0x0b` | Enhance Your Calm | `http2.constants.NGHTTP2_ENHANCE_YOUR_CALM` |
| `0x0c` | Inadequate Security | `http2.constants.NGHTTP2_INADEQUATE_SECURITY` |
| `0x0d` | HTTP/1.1 Required | `http2.constants.NGHTTP2_HTTP_1_1_REQUIRED` |
| `0x0d` | HTTP/1.1 Required | `http2.constants.NGHTTP2_HTTP_1_1_REQUIRED` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate line?

than the delta.

```js
clientSession.on('connect', (session) => sendSettings(session, (s) => cb(s)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps (s) => cb(s) would be more clear, as documentation, if it were written with meaningful variable names (settings?). No need to squeeze it on 80ch; just go multi-line.

doc/api/http2.md Outdated Show resolved Hide resolved
doc/api/http2.md Outdated
-->

* `windowSize` {number}
* Returns: 0
Copy link
Member

Choose a reason for hiding this comment

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

Why return anything?

Copy link
Author

Choose a reason for hiding this comment

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

In fact it's not exaclty the case, at this stage it returns only zero or throws ERR_OUT_OF_RANGE, the function is mapped to nghttp2_session_set_local_window_size.
If the library changes its internal, the value will be returned.
If it does not fit the philosophy of node, we can return a generic errror that maps all errors at the native code level.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Would you mind adding a unit test that verifies this behavior?

@@ -524,6 +524,33 @@ Used to set a callback function that is called when there is no activity on
the `Http2Session` after `msecs` milliseconds. The given `callback` is
registered as a listener on the `'timeout'` event.

#### http2session.setConnectionWindowSize(windowSize)
Copy link
Member

Choose a reason for hiding this comment

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

Would a getter and setter be better for an API?

Copy link
Author

Choose a reason for hiding this comment

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

It's a good point, I am not sure to be able to detect it. There is not acknoledge on a SETTINGS_INITIAL_WINDOW_SIZE
But, we can add a test for testing the limit of the buffer allocation for instance and insure that code results are covered.

Copy link
Author

Choose a reason for hiding this comment

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

I will add the getter

@ZYSzys
Copy link
Member

ZYSzys commented Jan 4, 2020

Ping @migounette , can you rebase and add some tests so that we can move forward ?

@migounette
Copy link
Author

Back on tracks for this activity, I will adresse it as soon as possible

@HarshithaKP
Copy link
Member

@migounette, this needs a rebase.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

Unfortunately this work appears to have stalled out. Closing but can reopen if it is picked back up again and rebased.

@jasnell jasnell closed this Jun 25, 2020
ZYSzys pushed a commit that referenced this pull request Nov 11, 2020
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
codebytere pushed a commit that referenced this pull request Nov 22, 2020
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
targos pushed a commit that referenced this pull request Aug 4, 2021
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
targos pushed a commit that referenced this pull request Aug 4, 2021
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 12, 2021
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 31, 2021
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
PR-URL: nodejs#35978
Fixes: nodejs#31084
Refs: nodejs#26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants