-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
HTTP/2 POST loop fails with ENHANCE_YOUR_CALM after 40701 iterations #23116
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Comments
The relevant code in nghttp2 is here: https://github.com/nodejs/node/blob/v10.10.0/deps/nghttp2/lib/nghttp2_session.c#L1211 if ((session->opt_flags & NGHTTP2_OPTMASK_NO_CLOSED_STREAMS) == 0 &&
session->server && !is_my_stream_id &&
nghttp2_stream_in_dep_tree(stream)) {
/* On server side, retain stream at most MAX_CONCURRENT_STREAMS
combined with the current active incoming streams to make
dependency tree work better. */
nghttp2_session_keep_closed_stream(session, stream);
} else {
rv = nghttp2_session_destroy_stream(session, stream);
if (rv != 0) {
return rv;
}
} I think this is going to cause a failure for any Node.js HTTP/2 app which processes many requests in the same session, due to tracking and limiting |
cc: @nodejs/http2 |
Note in the conditional above, I'm seeing both true in the logs. The requests are inserted as dependencies of the root, e.g.:
|
This is specifically there to prevent denial of service attacks. It is possible to change the default maximum session memory limit using the Try setting a significantly higher |
To clarify, a single session would keep around 232 bytes for every stream that was closed?
From the nghttp2 docs, it seems
what other checks it is doing? |
Ah, right, given that we're not really make use of priority, this likely can be switched off. Still, in this particular use case, I would still set a significantly higher |
If I increase The Web page has no way of resetting its session with the server - the So this means long running sessions to Node over HTTP/2 don't work reliably. |
Will investigate further! :) |
Here's a simple server app which reads HTTP/2 POST request bodies to their end and then responds:
For this test, the cert and key are self-issued and my browser trusts them.
Here's a Web page which makes requests in series to the server:
I expect this to continue indefinitely.
However, what happens is I get the following error after 40701 iterations:
I did some debugging using Wireshark and found the error was
ENHANCE_YOUR_CALM
.After adding some tracing to
node_http2.cc
, I found the error being produced here: https://github.com/nodejs/node/blob/v10.10.0/src/node_http2.cc#L863I've tracked this down to
current_nghttp2_memory_
continually growing.The cause for this is nghttp2 allocation of 232 bytes for each stream (https://github.com/nodejs/node/blob/v10.10.0/deps/nghttp2/lib/nghttp2_session.c#L1029) is never being released.
This is because nghttp2 keeps closed streams around (up to the concurrent connection limit).
If I add the following line to
Http2Options::Http2Options
insrc/node_http2.cc
:then the test works as expected and doesn't fail at 40701 iterations.
The text was updated successfully, but these errors were encountered: