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

Post large file failed when the initialWindowSize of HTTP2 Settings Object set to 6553500 #19141

Closed
SeaOceanLiu opened this issue Mar 5, 2018 · 6 comments
Assignees
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@SeaOceanLiu
Copy link

Version: 9.5.0
Platform: Windows7 x64 / Windows10 x64
Subsystem: node_http2

It is required to post large file (~100MB) to a HTTP/2 server in a project, we implement the HTTP/2 server using node.js v9.5.0.

The speed is too low when comparing to HTTPS when post via HTTP/2 with the default settings. It was the reason the TCP package sending speed is too low after we checked a lot of captured package. So, we had to adjust the initialWindowSize of HTTP2 Settings Object quite larger than the default value (65535), to 6553500.

After that, we got an error from the client saying the connection timeout during the file transition. And more we found is the WINDOW_SIZE is not updated by the server anymore. We had to analysis the node.js source code without helpful searching result from the internet. In the node_http2.c, we found it only set the stream level WINDOW_SIZE when we adjust initialWindowSize of HTTP2 Settings Object, left the session level one. That is the root reason cause the HTTP/2 server stopping update the WINDOW_SIZE.

NGHTTP2 lib has provided the API interface to modify both the stream and session level WINDOW_SIZE, so we temporarily modify Http2Session::Http2Settings::Send() to call the API (nghttp2_session_set_local_window_size()) with the stream_id = 0 when the stream level WINDOW_SIZE > session level WINDOWS_SIZE.

We are still looking for the best solution...
Thanks for any help!

@bzoz
Copy link
Contributor

bzoz commented Mar 8, 2018

/cc @nodejs/http2

@mcollina
Copy link
Member

mcollina commented Mar 8, 2018

@jasnell what do you think?

@trivikr trivikr added the http2 Issues or PRs related to the http2 subsystem. label Mar 25, 2018
@apapirovski apapirovski self-assigned this Jun 24, 2018
@apapirovski
Copy link
Member

I still need to actually test this but two things:

  1. The fact that a window update is not being propagated seems to either be an nghttp2 bug or with whatever client is being used. The C++ code consumes data as it comes in, so the blame can't be there.

  2. I'm not really certain what the correct behaviour here is. I'm leaning towards exposing an additional initialConnectionWindowSize option because I'm not convinced that changing initialWindowSize should affect the connection level flow-control window.

@jasnell thoughts?

@jasnell
Copy link
Member

jasnell commented Aug 10, 2018

Oy, just spotted this. It ended up buried in a backlog. Will investigate.

@jasnell
Copy link
Member

jasnell commented Aug 10, 2018

I am unable to replicate any issues here with the current code in master. It is possible that the 9.x branch was simply out of date. I will be opening a PR with a test that transfers 100 MB of data from client to server with a large initial window size that confirms that it should be working end to end.

jasnell added a commit to jasnell/node that referenced this issue Aug 10, 2018
gdams pushed a commit that referenced this issue Aug 13, 2018
Refs: #19141

PR-URL: #22254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rvagg pushed a commit that referenced this issue Aug 15, 2018
Refs: #19141

PR-URL: #22254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@TimothyGu
Copy link
Member

Now that #22254 is landed, let's call this resolved.

kjin pushed a commit to kjin/node that referenced this issue Sep 25, 2018
Refs: nodejs#19141

PR-URL: nodejs#22254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Oct 16, 2018
Refs: nodejs#19141

PR-URL: nodejs#22254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this issue Oct 17, 2018
Refs: #19141

Backport-PR-URL: #22850
PR-URL: #22254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants