http: fix submission during shutdown race

[furszy]

Fixes #34573.

As mentioned in #34573 (comment), the ThreadPool PR (#33689) revealed an existing issue.

Before that PR, we were returning an incorrect error "Request rejected because http work queue depth exceeded" during shutdown for unhandled requests (we were not differentiating between "queue depth exceeded" and "server interrupted" errors). Now, with the ThreadPool inclusion, we return the proper error but we don't handle it properly.

This PR improves exactly that. Handling the missing error and properly returning it to the user.

The race can be reproduced as follows:

1. The server receives an http request.
2. Processing of the request is delayed, and shutdown is triggered in the meantime.
3. During shutdown, the libevent callback is unregistered and the threadpool interrupted.
4. The delayed request (step 2) resumes and tries to submit a task to the now-interrupted server.

Reproduction test can be found #34577 (comment).

Also, to prevent this kind of issue from happening again, this PR changes task submission
to return the error as part of the function's return value using util::Expected instead of
throwing the exception. Unlike exceptions, which require extra try-catch blocks and can be
ignored, returning Expected forces callers to explicitly handle failures, and attributes
like [[nodiscard]] allow us catch unhandled ones at compile time.

[DrahtBot]

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sedited, pinheadmz, andrewtoth, achow101, hodlinator
Concept ACK	ismaelsadeeq
Stale ACK	Eunovo

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34562 (ThreadPool follow-ups, proactive shutdown and HasReason dependency cleanup by furszy)
• #32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[DrahtBot]

🚧 At least one of the CI tasks failed.
_{Task No wallet: https://github.com/bitcoin/bitcoin/actions/runs/21961944156/job/63441463470
LLM reason (✨ experimental): Compilation failed in fuzz/threadpool.cpp: Submit returns util::Expected<future, string> which cannot be assigned to std::future, causing build errors.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being
  incompatible with the current code in the target branch). If so, make sure to rebase on the latest
  commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the
  affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

[andrewtoth]

The race can be reproduced as follows:

Can this be made into a functional regression test?

[furszy]

Can this be made into a functional regression test?

The "processing request is delayed" step means manually placing a sleep() inside the libevent callback http_request_cb() to progress on the server shutdown concurrently. Then we need to sleep() during shutdown, just after InterruptHTTPServer(), to resume the request processing before shutdown occurs.
So, the short answer is no. Sadly races aren't easy to replicate deterministically in functional tests.
We will surely be able to exercise more scenarios like this one, in unit tests, after the new http server inclusion (libevent removal path), which will bring better encapsulation.

That being said, could provide a sleep()-based patch to replicate it locally.

[hodlinator]

Concept ACK 5326821

[hodlinator]

Nice helper! Was actually experimenting with changing ThreadPool::Submit() to return util::Expected yesterday (suffering from lack of knowledge of std::invoke_result_t<F> though). It was so cumbersome to adapt these test without realizing a helper would be useful. :)

nit: The going style seems to be [[nodiscard]] on the same line as the return type:

template <typename F>
[[nodiscard]] auto Submit(ThreadPool& pool, F&& fn)

Examples from master:

bitcoin/src/util/overflow.h

Lines 14 to 15 in 07b9247

	template <class T>
	[[nodiscard]] bool AdditionOverflow(const T i, const T j) noexcept

bitcoin/src/test/fuzz/util.h

Lines 84 to 85 in 07b9247

	template <typename T>
	[[nodiscard]] inline std::vector<T> ConsumeRandomLengthIntegralVector(FuzzedDataProvider& fuzzed_data_provider, const size_t max_vector_size = 16) noexcept

Same style nit goes for ThreadPool::Submit().

[furszy]

Done as suggested. [[nodiscard]] inlined.
Also, credits to ryanofsky for the helper function idea <3. I was about to use a macro.

[hodlinator]

remark: Was experimenting with having ThreadPool::Submit() take a "task_creator" lambda that only gets called when no error condition in Submit occurs, it would then return the inner lambda which would take ownership of the unique_ptr as on master. Even though I have a distaste for shared_ptr, your version is more elegant.

[hodlinator]

Commit message suggestion for 5326821:

Given that I think the unclean shutdown is already fixed in the first commit by using util::Expected instead of throwing exceptions, I think this is somewhat more correct for the second:

http: properly respond to HTTP request during shutdown

Makes sure we respond to the client as the HTTP request attempts to submit a task to
the threadpool during server shutdown.

Roughly what happens:

1) The server receives an HTTP request and starts calling http_request_cb().
2) Meanwhile on another thread, shutdown is triggered which calls InterruptHTTPServer()
   and unregisters http_request_cb() and interrupts the thread pool.
3) The request (step 1) resumes and tries to submit a task
   to the now-interrupted server.

This fix detects failed submissions immediately, and the server
responds with HTTP_SERVICE_UNAVAILABLE.

I don't think it's necessary to classify this as a race condition. I guess it depends on how you define it.

[furszy]

Taken, thanks!

I don't think it's necessary to classify this as a race condition. I guess it depends on how you define it.

There is a thread (shutdown - main thread) modifying a resource (thread pool) just before another thread (libevent http handler) accesses it. It is pretty much a race condition to me.

[hodlinator]

There is a thread (shutdown - main thread) modifying a resource (thread pool) just before another thread (libevent http handler) accesses it. It is pretty much a race condition to me.

Fair.

[furszy]

Updated per feedback, thanks @hodlinator!

[hodlinator]

There is a thread (shutdown - main thread) modifying a resource (thread pool) just before another thread (libevent http handler) accesses it. It is pretty much a race condition to me.

Fair.

[hodlinator]

Thanks for incorporating my earlier feedback btw!

[andrewtoth]

That being said, could provide a sleep()-based patch to replicate it locally.

Here is a diff, along with a functional test rpc_http_shutdown_race.py. Applying this to the base commit 07b9247 will fail with a remote disconnected error, and applying to this patch 542879d will pass.

diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 671e119642..d2de754183 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -18,6 +18,7 @@
 #include <util/strencodings.h>
 #include <util/threadnames.h>
 #include <util/threadpool.h>
+#include <util/time.h>
 #include <util/translation.h>
 
 #include <condition_variable>
@@ -257,6 +258,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
             hreq->WriteReply(HTTP_SERVICE_UNAVAILABLE, "Work queue depth exceeded");
             return;
         }
+        UninterruptibleSleep(2s);
 
         auto item = [req = std::move(hreq), in_path = std::move(path), fn = i->handler]() {
             std::string err_msg;
@@ -446,6 +448,7 @@ void InterruptHTTPServer()
     }
     // Interrupt pool after disabling requests
     g_threadpool_http.Interrupt();
+    UninterruptibleSleep(2s);
 }
 
 void StopHTTPServer()
diff --git a/test/functional/rpc_http_shutdown_race.py b/test/functional/rpc_http_shutdown_race.py
new file mode 100755
index 0000000000..f91cdee9f3
--- /dev/null
+++ b/test/functional/rpc_http_shutdown_race.py
@@ -0,0 +1,42 @@
+#!/usr/bin/env python3
+# Copyright (c) The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test that an in-flight RPC during shutdown receives 503."""
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal, str_to_b64str
+
+import http.client
+import urllib.parse
+
+class RpcHttpShutdownRaceTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+        self.setup_clean_chain = True
+
+    def run_test(self):
+        url = urllib.parse.urlparse(self.nodes[0].url)
+        authpair = f"{url.username}:{url.password}"
+        headers = {"Authorization": f"Basic {str_to_b64str(authpair)}"}
+
+        conn = http.client.HTTPConnection(url.hostname, url.port, timeout=10)
+        conn.connect()
+        conn.request("POST", "/", '{"method":"getblockchaininfo","id":1}', headers)
+
+        # Trigger shutdown outside RPC callback path so the in-flight request
+        # is interrupted before submitting to the pool.
+        self.nodes[0].process.terminate()
+
+        resp = conn.getresponse()
+        body = resp.read()
+        assert_equal(resp.status, http.client.SERVICE_UNAVAILABLE)
+        assert b"Request rejected during server shutdown" in body
+        node = self.nodes[0]
+        self.wait_until(lambda: node.process.poll() is not None, timeout=20)
+        stderr = node.stderr.read().decode("utf-8", errors="replace").strip()
+        node.is_node_stopped(expected_stderr=stderr, expected_ret_code=node.process.returncode)
+
+
+if __name__ == '__main__':
+    RpcHttpShutdownRaceTest(__file__).main()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index cdfa8c1022..0d21907677 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -201,6 +201,7 @@ BASE_SCRIPTS = [
     'wallet_reindex.py',
     'wallet_reorgsrestore.py',
     'interface_http.py',
+    'rpc_http_shutdown_race.py',
     'interface_rpc.py',
     'interface_usdt_coinselection.py',
     'interface_usdt_mempool.py',

[andrewtoth]

ACK 542879d

This fixes the unclean shutdown race condition. It's too bad we can't have a regression test without adding sleeps.

[furszy]

Thanks for the reproduction patch @andrewtoth! Added link to the PR description.

This fixes the unclean shutdown race condition. It's too bad we can't have a regression test without adding sleeps.

A small reflection about this.

We can't expect to be able to cover everything in functional tests. Simply because they live on a higher level and have no access to internals to exercise race conditions.

These types of scenarios belong to the unit tests land. But for that to happen, the surrounding code has to be properly encapsulated. Which is not the case for the http server, as there is currently no class for it, we basically import an entire file with global variables and static functions (and up to a few days ago, we had no tests for the work queue at all and this race condition was already there, just hidden returning an incorrect error message instead).

The only way to replicate scenarios like this one atm, on a unit test, would be to manually copy paste startup and shutdown. Which is not really scalable nor maintainable.

Still, we will get better soon, the libevent removal path @pinheadmz is working on also brings better encapsulation that we can use to deeply cover the area.

[ismaelsadeeq]

Concept ACK

@furszy, it would be nice if you share a branch with a reproducer.

edit: see #34577 (comment) for reproducer

[hodlinator]

The CI MSan failure in p2p_private_broadcast.py¹ possibly stems from the I2P network being unstable².

Footnotes

1. https://github.com/bitcoin/bitcoin/actions/runs/22083305828/job/63812732359?pr=34577#step:11:3388 ↩

2. https://stacker.news/items/1434628 ↩

[hodlinator]

IMO stopping the ThreadPool before we stop the libevent dispatch loop is the wrong order. Would ideally do something like the following patch:

Patch for HTTP shutdown order

http: Correct shutdown order of libevent vs ThreadPool

g_threadpool_http is initialized before the libevent dispatch loop in g_thread_http,
so it should be deinitialized in the reverse order. Exit the event loop first so
that we stop trying to Submit() new tasks to the pool before stopping it. (We will
still interrupt the pool earlier on, so submission-attempts can still fail).

Also:
* Delete the accept sockets before exiting the loop.
* Free eventHTTP from our own thread as events are no longer processed (this also fixes a frequent memory leak where eventHTTP would not be freed).

diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 578c4ec693..ec821dd0b0 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -457,15 +457,20 @@ void StopHTTPServer()
 {
     LogDebug(BCLog::HTTP, "Stopping HTTP server\n");
 
-    LogDebug(BCLog::HTTP, "Waiting for HTTP worker threads to exit\n");
-    g_threadpool_http.Stop();
-
-    // Unlisten sockets, these are what make the event loop running, which means
-    // that after this and all connections are closed the event loop will quit.
+    // Unlisten sockets
     for (evhttp_bound_socket *socket : boundSockets) {
         evhttp_del_accept_socket(eventHTTP, socket);
     }
     boundSockets.clear();
+
+    if (eventBase) {
+        LogDebug(BCLog::HTTP, "Finishing processing of event loop callbacks\n");
+        event_base_loopexit(eventBase, nullptr);
+    }
+
+    LogDebug(BCLog::HTTP, "Waiting for HTTP worker threads to exit\n");
+    g_threadpool_http.Stop();
+
     {
         if (const auto n_connections{g_requests.CountActiveConnections()}; n_connections != 0) {
             LogDebug(BCLog::HTTP, "Waiting for %d connections to stop HTTP server\n", n_connections);
@@ -473,13 +478,8 @@ void StopHTTPServer()
         g_requests.WaitUntilEmpty();
     }
     if (eventHTTP) {
-        // Schedule a callback to call evhttp_free in the event base thread, so
-        // that evhttp_free does not need to be called again after the handling
-        // of unfinished request connections that follows.
-        event_base_once(eventBase, -1, EV_TIMEOUT, [](evutil_socket_t, short, void*) {
-            evhttp_free(eventHTTP);
-            eventHTTP = nullptr;
-        }, nullptr, nullptr);
+        evhttp_free(eventHTTP);
+        eventHTTP = nullptr;
     }
     if (eventBase) {
         LogDebug(BCLog::HTTP, "Waiting for HTTP event thread to exit\n");

Doing the above allows for merging the ThreadPool's constructor with Start() and merging the destructor with Stop().

• This follows from not allowing calling Start() twice, which is implicit in a constructor. The new assert in Start() becomes unnecessary.
• Having Stop() logic only happen during destruction also implies that only a single thread should have access to the ThreadPool instance at that time. This would prevent the potential race condition I found here: ThreadPool follow-ups, proactive shutdown and HasReason dependency cleanup #34562 (comment)

Patch to merge ThreadPool ctor with Start() and dtor with Stop()

util: Merge ThreadPool ctor with Start(), and ThreadPool dtor with Stop().

HTTP now wraps the pool in std::optional.

diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index ec821dd0b0..8ca5b7771e 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -75,7 +75,7 @@ static std::vector<HTTPPathHandler> pathHandlers GUARDED_BY(g_httppathhandlers_m
 //! Bound listening sockets
 static std::vector<evhttp_bound_socket *> boundSockets;
 //! Http thread pool - future: encapsulate in HttpContext
-static ThreadPool g_threadpool_http("http");
+static std::optional<ThreadPool> g_threadpool_http;
 static int g_max_queue_depth{100};
 
 /**
@@ -252,7 +252,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
 
     // Dispatch to worker thread
     if (i != iend) {
-        if (static_cast<int>(g_threadpool_http.WorkQueueSize()) >= g_max_queue_depth) {
+        if (static_cast<int>(g_threadpool_http->WorkQueueSize()) >= g_max_queue_depth) {
             LogWarning("Request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting");
             hreq->WriteReply(HTTP_SERVICE_UNAVAILABLE, "Work queue depth exceeded");
             return;
@@ -276,7 +276,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
             req->WriteReply(HTTP_INTERNAL_SERVER_ERROR, err_msg);
         };
 
-        if (auto res = g_threadpool_http.Submit(std::move(item)); !res.has_value()) {
+        if (auto res = g_threadpool_http->Submit(std::move(item)); !res.has_value()) {
             // Both SubmitError::Inactive and SubmitError::Interrupted mean shutdown
             LogWarning("HTTP request rejected during server shutdown: '%s'", SubmitErrorString(res.error()));
             hreq->WriteReply(HTTP_SERVICE_UNAVAILABLE, "Request rejected during server shutdown");
@@ -438,7 +438,7 @@ void StartHTTPServer()
 {
     int rpcThreads = std::max((long)gArgs.GetIntArg("-rpcthreads", DEFAULT_HTTP_THREADS), 1L);
     LogInfo("Starting HTTP server with %d worker threads\n", rpcThreads);
-    g_threadpool_http.Start(rpcThreads);
+    g_threadpool_http.emplace("http", rpcThreads);
     g_thread_http = std::thread(ThreadHTTP, eventBase);
 }
 
@@ -450,7 +450,7 @@ void InterruptHTTPServer()
         evhttp_set_gencb(eventHTTP, http_reject_request_cb, nullptr);
     }
     // Interrupt pool after disabling requests
-    g_threadpool_http.Interrupt();
+    g_threadpool_http->Interrupt();
 }
 
 void StopHTTPServer()
@@ -469,7 +469,7 @@ void StopHTTPServer()
     }
 
     LogDebug(BCLog::HTTP, "Waiting for HTTP worker threads to exit\n");
-    g_threadpool_http.Stop();
+    g_threadpool_http.reset();
 
     {
         if (const auto n_connections{g_requests.CountActiveConnections()}; n_connections != 0) {
diff --git a/src/util/threadpool.h b/src/util/threadpool.h
index e1f2d86ed0..7a3d77e453 100644
--- a/src/util/threadpool.h
+++ b/src/util/threadpool.h
@@ -87,13 +87,6 @@ private:
     }
 
 public:
-    explicit ThreadPool(const std::string& name) : m_name(name) {}
-
-    ~ThreadPool()
-    {
-        Stop(); // In case it hasn't been stopped.
-    }
-
     /**
      * @brief Start worker threads.
      *
@@ -104,12 +97,11 @@ public:
      * initialization and idle shutdown patterns, callers must provide their
      * own synchronization.
      */
-    void Start(const int num_workers) noexcept EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    explicit ThreadPool(const std::string& name, const int num_workers) noexcept EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+        : m_name(name)
     {
         assert(num_workers > 0);
         LOCK(m_mutex);
-        assert(m_workers.empty());
-        m_interrupt = false; // Reset
 
         // Create workers
         m_workers.reserve(num_workers);
@@ -126,7 +118,7 @@ public:
      *
      * Must be called from a controller (non-worker) thread.
      */
-    void Stop() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    ~ThreadPool() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         // Notify workers and join them
         std::vector<std::thread> threads_to_join;

Curious to hear what others think. This is not blocking for me within this PR. I haven't thoroughly tested the libevent change beyond running functional tests and I can understand not wanting to touch libevent behavior. Maybe the change to ThreadPool should wait until #32061 lands.

[furszy]

IMO stopping the ThreadPool before we stop the libevent dispatch loop is the wrong order

Just to be precise with this, the ordering is wrong but it is not really an issue, because (1) InterruptHTTPServer replaces the handler for one that rejects all incoming requests. And (2) even if we would be handling requests, the pool would just reject the work and we would return a clear error message now.

I agree with the first patch, but I think we should limit ourselves to refactoring libevent code while trying to avoid behavior changes at this point (unless we see a very clear benefit behind them). Modifications there require additional research time for an unmaintained library.

Moving the pool shutdown to the end of the function should give us the same behavior without touching the libevent code.

Doing the above allows for merging the ThreadPool's constructor with Start() and merging the destructor with Stop().

RAII the pool was brought up a few times before; I implemented it too. It’s not a bad idea, but I’ve been trying not to introduce it too early and first gather more use cases. Mainly because it would make lazy startup and idle shutdown patterns harder to implement. It would also require pool configurations to be stored in a separate structure and dup them when the pool gets constructed.

[hodlinator]

Mainly because it would make lazy startup and idle shutdown patterns harder to implement.

Yup, it took me a little while to figure out I needed to use optional::emplace() to in-place construct the pool in my suggested patch. Probably first time I used that in code I wrote.

It would also require pool configurations to be stored in a separate structure and dup them when the pool gets constructed.

Didn't run into such issues in this instance FWIW.

---

Interesting that more RAII style was brought up earlier, it does feel like a natural fit if we didn't have libevent to navigate. Anyway, I'm happy we have this primitive now, sorry I didn't have bandwidth to review it earlier.

[furszy]

Interesting that more RAII style was brought up earlier, it does feel like a natural fit if we didn't have libevent to navigate

we have some enthusiastic RAII fans around here.

[andrewtoth]

ACK 9d45a1b

[furszy]

Updated per @pinheadmz feedback, thanks.
Only added a shared_pointer.use_count() == 1 check to be completely sure the request is destroyed upon submission failure.

[Eunovo]

Tested ACK a6fc8a4

Confirmed using diff from #34577 (comment)

[hodlinator]

re-ACK a6fc8a4

[fanquake]

@dergoegge does this resolve #34573 for you?

[pinheadmz]

ACK a6fc8a4

Built and tested on arm64/macos. Reviewed each commit, gave feedback which was addressed. I compared the results of the suggested functional test against modified code between master and branch, ran the test without the modified code between master and branch, and observed all those trials with Wireshark to confirm that the PR improves HTTP server behavior during shutdown.

Show Signature

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK a6fc8a43f3f5b7605ebcd24212e01524c86a9189
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmmW/0sACgkQ5+KYS2KJ
yTomKxAAhc0LeceulNUcjufbNhrpCSGYCK3n+5BrLLKObfQbngO1OnOW3wOEVBjt
RwwpJpwa7nIX2U9YoLWDEJKb5HmdozJZAIWHRuuX2qALOkEr/oguBYZEtan72DDq
cvtm+d6jncur1FnieRTDIdj8/YFkJPIv4dtGyxuy7f3VF0cODuk2fE7oZ+NIQdzN
rMcgI4757HIltYxJRuTVAt7KtvfZuMye/6UufBOcjz159vM+uUi6ORGORzf6EUk9
N3ZF+6DWrQrRjWi0k195t9i0TVDlF/eND2DZmEkK5Jrt+ZjcTMoUaQ3Wj2WoRRfd
XQ5CL9jzw0g8v6LMdYaVmAOfJbIzoF1H9NMWx1a7dwTkaABClXvm0QjB8N14kbhZ
S3inDNJ0hTfyoEeXyHk+Ui+TMj1TutFq1kLvN5TUT+lDE/e/rybL9cKNQvl8NrEk
CtD2mIXLf0vJynpo/hOEO4IP6WzUJjl/6gyrhJ3Dli6xLUznNsPnFB0YgNd2+tPk
0xr/SQ0DTn5GVzOkw9E5jkvDjoVKRbV1sZaw039cBL3ika7ckvt3pEumKj+zZ+IK
IVZ/mXSrfCVKxy3WfY9eQjgjp0A5gogpTQR/MHq1TQ3Pa0v0Qn18bdw0mvm32R8f
DXw8cVQj0JZEP2XiOxD3G2v6rtYK+vnnFQUpxhgcbNSND/8Fpg8=
=Q/ll
-----END PGP SIGNATURE-----

pinheadmz's public key is on openpgp.org

[sedited]

Not sure about marking this as noexcept. Exceptions here would be caught by AppInit and we'd shutdown gracefully. There a bunch of functions that do still throw here, e.g. std::system_error when spawning the thread fails. Do we really gain anything by terminating immediately?

[furszy]

Agree on removing noexcept.

And this is probably a good moment to align on the broader design philosophy so we have some common foundations going forward.

The way I'm seeing it after some back and forth: there are two fundamentally different kinds of failures, and they deserve different treatment. Plus we have a special third rule that is even more important to us than any design decision.

The first is environment related failures: things like an allocation failure, IO error, thread spawning failing due to the OS is out of resources, etc. These can happen even in perfectly correct code. For these, throwing makes sense. Even if the caller can't fully recover, it at least gets the chance to shut down gracefully, flush state, log something useful. Hard termination throws all of that away.

The second kind is logic errors: things that cannot happen if the code is correct. A double Start() call is a good example of this. That's not the OS misbehaving, that's a bug in the caller. Throwing a std::runtime_error implies the caller might catch it and carry on, when really we want to say "this shouldn't happen, and if it does, something is just wrong". An assertion is more direct here. It's not an error to be handled, it's a violation of the API contract.

The third rule that applies only to us (Bitcoin-Core) is that we want to minimize assertions in code that can be accessed through the P2P layer. We don't want to be logically honest here, we want to guard against someone triggering a crash remotely.

---

Maybe if we agree on this, we could add it to one of our .md guidelines. So we don't have to revive this type of discussions every time someone suggests to add a noexcept or to change an exception for an assertion in risky places.

Thoughts?

[hodlinator]

The first is environment related failures: things like an allocation failure, IO error, thread spawning failing due to the OS is out of resources, etc. These can happen even in perfectly correct code. For these, throwing makes sense. Even if the caller can't fully recover, it at least gets the chance to shut down gracefully, flush state, log something useful. Hard termination throws all of that away.

I think it's fairly hard to recover from OOM, IO errors or failing to spawn threads, so wouldn't aim to use exceptions in this case. Exiting with a clear error in the log/stderr and/or crash dump seems realistic and sufficient.

Agree it would be good to document, if we can agree. ;)

Some compilers/stdlibs allow for disabling exceptions btw. Not advocating for it in this project, just FYI. I guess one gets a hard crash instead of stack unwinding when throwing from constructors etc.

[furszy]

Due to the upcoming release freeze, decided to drop the last commit a6fc8a4 to keep this PR focused strictly on fixing the race.

That commit was a design improvement rather than a direct fix, so it makes sense to handle it separately. We can reintroduce it in a follow-up once we align on the overall design direction #34577 (comment) (if possible).

PR ready to go.

[sedited]

ACK 726b366

[pinheadmz]

re-ACK 726b366

Change since last review, dropped tip commit which doesn't affect the bug fix targeting by the rest of the branch.

Show Signature

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmmXTMUACgkQ5+KYS2KJ
yTrnag/+IVOKcTiwVPKY0LS+i96t8qwCDeBCYH22b7pzwdDbQ8Bmnhg42Bz5kSQq
XuGPXt4sAn0MjGiC7EpAIErA+yhT/nrf1QuOyJslSUz+6BnO491trzZAdPJ9cnyF
13x4AY2mYs/fLFCfKVk7QhW5U+0XfgqcVD/BNsPtmBfcA6ClXUmFatBL3Mb+P1Y8
H26shtJUz3If+HAM6d5Iey/IoMgS6ENgaLr+4weMRej6JYpNfU2cC4+G9vs/WgRL
+W2K+Fs+Ose2lMLwg+Py6m7vmtrrb1PzuYYGw2B07qsHOP9aGGN+SLDEVs/tEMeY
YUVY0vWnplCG0NMo+VQoMa4xchaMNVpLN1Haa4nwBDWOxSn64t3a9E/EFh+9X3Z/
fLppj+2KrUqCeJ6eN/bJB2sAutq+oZSMFwgybqArPQ9KSMSoNAwoUYySphMmeEkW
JC6NcCTNkI1ZzVrZChizJPYLoGVF7YKWnlo2lLR74/l99+auKQAmTxBIl5hlctoB
4Fv3tgqS+iKrPe9g1mrgGRInhCwsvXVU9tZtgXChzQGuecfQYaESmWVJ1RvqzbQx
fUSbShCOT7iYs7OK+7j5BwPyIBgRoDEf9A9fJ8+/QlYSkQRnTMqux90AtdWXKm/V
Pde7ojKRmACJsuC2hLnE/Ympa63+EwSBf1xZXiLKNtCauVlQCpo=
=TWiX
-----END PGP SIGNATURE-----

pinheadmz's public key is on openpgp.org

[andrewtoth]

ACK 726b366

[achow101]

ACK 726b366

[hodlinator]

re-ACK 726b366

[dergoegge]

does this resolve #34573 for you?

I did re-run the test that found this new crash issue yesterday and it looks like it's fixed.

Fuzzing/property-based tests take some time to run. If I get pinged to re-test something it'd make sense to wait for me to follow up before merge. Otherwise I'll likely stop responding to these pings and will open a new issue if something wasn't fixed.

[ismaelsadeeq]

Fuzzing/property-based tests take some time to run. If I get pinged to re-test something it'd make sense to wait for me to follow up before merge. Otherwise I'll likely stop responding to these pings and will open a new issue if something wasn't fixed.

Maybe it would be helpful to let the maintainers/contributors know you're looking into it/ i.e re-running tests after getting pinged, so they know whether to wait or move forward?
If an issue wasn't fixed after a fix is merged and resolved, opening a new issue seems fair.

[dergoegge]

In this case at least one maintainer knew, but in general they can expect me to follow up and if it's urgent they can also ping me elsewhere.

[furszy]

In this case, the crash reproducer script made it straightforward to proceed, and I think people didn't wanted to bother you further since the cause was clear @dergoegge.

What we could have done better was waiting for your feedback on the issue before closing it, I agree. And, at the same time, I also agree that a brief coordination message "running fuzzers, will update soon" would have been very helpful for all of us as well.

I mean, I think we all understand that everyone here is busy and overloaded with parallel projects and tasks, and we all work across different time zones, so being a bit more public on our actions would help minimize misunderstandings.