Prevent file descriptor exhaustion from too many RPC calls

fjahr commented at 11:20 pm on May 23, 2023: contributor

Fixes #11368

This requires libevent 2.2.1 which so far was only released in alpha.

For more context see specifically #11368 (comment), this uses the feature mentioned there as libevent#592.

DrahtBot commented at 11:20 pm on May 23, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

in src/httpserver.cpp:473 in a0eed5d459 outdated

418@@ -419,6 +419,15 @@ bool InitHTTPServer()
419     int workQueueDepth = std::max((long)gArgs.GetIntArg("-rpcworkqueue", DEFAULT_HTTP_WORKQUEUE), 1L);
420     LogPrintfCategory(BCLog::HTTP, "creating work queue of depth %d\n", workQueueDepth);
421 
422+#if LIBEVENT_VERSION_NUMBER >= 0x02020001
423+    if (event_get_version_number() >= 0x02020001) {
424+        // Limit the maximum number of open connections to prevent exhausting
425+        // the file descriptor limit. When the http server gets overwhelmed it
426+        // will respond with 503 Service Unavailable.
427+        evhttp_set_max_connections(http, workQueueDepth * 2);

willcl-ark commented at 10:09 am on May 24, 2023:

I am not sure this actually limits us within the file descriptor limit. For example if you run:

0$ ulimit -n 175
1$ src/bitcoind
2Warning: Reducing -maxconnections from 125 to 15, because of system limitations.
3Bitcoin Core starting

I think (but have not tested) that if I then recieve 15 inbound connections I would could still run out of file descriptors for the http server?

Perhaps if we also modify this line:

https://github.com/bitcoin/bitcoin/blob/51c050787fd6bcd016969dd7e245818ebd110b67/src/init.cpp#L943

…to consider the workqueue depth too, then it could work?

fjahr commented at 12:06 pm on May 24, 2023:

Yeah, I think it will be hard to find a number where there isn’t a specific limit and specific circumstances that would still cause problems. If we set the limit dynamically based on what’s really available there would be a range where we would effectively set evhttp_set_max_connections(http, 0); which would probably mean that we couldn’t even shut down the server via the RPC interface.

When the maxconnections are reduced to 15 that doesn’t mean that there are only 15 file descriptors left. There are more reserved than are not always needed. But that part of the code certainly could use improvement, see #27539 and #27730. I think making changes to the maxconnections calculation is not necessarily linked to this change here because it could reduce the risk of running out of FDs regardless. Also, note that the node will even start if you set ulimit -n 150 and it have maxconnection at 0. So that is another question to discuss if that even makes sense. Because if we alter the calculation of maxconnections but still let the node run regardless of what the result is, we haven’t really prevented the file descriptor exhaustion completely. And it may be too limiting on the other hand if we set the requirements of available file descriptors so high that we can absolutely be sure the node will not encounter an exhaustion under any circumstances.

So, I am not sure what would be the optimal number to use. I think we can still tweak this later so it covers the most common use cases best. What I am currently concerned about is if this actually improves the situation in #11368 for the default file descriptor limits in the most commonly used OS. macOS was among the lowest as far as I remember and it was 256 usually I think. So I am currently aiming at showing that it’s an improvement for that. But I do struggle to reproduce the original issue on my new machine right now so I am not 100% sure anymore if this actually needs fixing. Though I am on a different machine now than I was back then.

in src/httpserver.cpp:469 in 8b0d64d831 outdated

418@@ -419,6 +419,15 @@ bool InitHTTPServer()
419     int workQueueDepth = std::max((long)gArgs.GetIntArg("-rpcworkqueue", DEFAULT_HTTP_WORKQUEUE), 1L);
420     LogPrintfCategory(BCLog::HTTP, "creating work queue of depth %d\n", workQueueDepth);
421 
422+#if LIBEVENT_VERSION_NUMBER >= 0x02020001
423+    if (event_get_version_number() >= 0x02020001) {

luke-jr commented at 1:06 am on June 24, 2023:

Won’t runtime linking fail to find the evhttp_set_max_connections symbol?

fjahr commented at 3:51 pm on June 28, 2023:

Hm, how would that happen? I didn’t have any issues when testing this code with 2.2.1-alpha and 2.1.12-stable.

luke-jr commented at 10:26 pm on July 2, 2023:

I’m thinking of the case where you build with 2.2.1, then try to run it with 2.1.12. (If that’s not supported, the runtime check is unnecessary)

in doc/JSON-RPC-interface.md:191 in 8b0d64d831 outdated

159-want to increase the number of maximum allowed file descriptors in your system
160-and try to prevent opening too many connections to your JSON-RPC interface at the
161-same time if this is under your control. It is hard to give general advice
162-since this depends on your system but if you make several hundred requests at
163-once you are definitely at risk of encountering this issue.
164+When too many connections are opened quickly the interface will start to

luke-jr commented at 6:08 pm on September 15, 2023:

This loses the correct docs for built-against-older-libevent

DrahtBot added the label CI failed on Oct 25, 2023

DrahtBot commented at 2:34 pm on February 5, 2024: contributor

🤔 There hasn’t been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

DrahtBot commented at 0:14 am on May 5, 2024: contributor

🤔 There hasn’t been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

http: Use new max connection limit from libevent 2.2 5e8eccceac

doc: Update limitations sections in RPC and REST interface docs 3a66391bdb

fjahr force-pushed on May 5, 2024

fjahr commented at 3:40 am on May 5, 2024: contributor

Pushed a rebase though currently I am working on removing libevent as a dependency which should be a quicker solution than waiting for 2.2.1 :) I will address the feedback if 2.2.1 lands anytime soon though!

DrahtBot removed the label CI failed on May 5, 2024

maflcko commented at 8:35 am on May 6, 2024: member

removing libevent

Nice. Is there a tracking issue?

fjahr commented at 2:33 pm on May 29, 2024: contributor

removing libevent

Nice. Is there a tracking issue?

Not yet, but should be open soon (TM).

DrahtBot added the label CI failed on Sep 12, 2024

DrahtBot removed the label CI failed on Sep 16, 2024

Prevent file descriptor exhaustion from too many RPC calls #27731

Code Coverage

Reviews

Conflicts