Bug 51

Thread-busy test checks an error message no standard library produces

Status

Fixed

Symptom

Wrong result; worst effect seen: log message only

Where seen

Libmultiprocess tests (mptest)

Trigger

Normal use; no special event is needed

First reported

2025-10-02 (first report)

Fixed

2025-10-07

Fix

mp#222

Shipped to Bitcoin Core

bitcoin#33518, bitcoin#33519

Summary

The mptest test "Make simultaneous IPC callbacks with same request_thread and callback_thread", added in mp#214 (which fixed a crash on simultaneous IPC calls to one thread), compared std::future_error::what() against the literal "Future already retrieved". Neither libstdc++ nor libc++ produces that text, so the check failed. The failure was printed but did not fail the test, because the check ran off the main test thread. The defect was in test code and did not affect users. Fixed in mp#222 ("test, ci: Fix threadsanitizer errors in mptest") by comparing the error code instead.

What happened

While reworking the libmultiprocess sanitizer CI job in mp#222 (2025-10-02), ryanofsky saw this in the mptest output:

test/mp/test/test.cpp:339: error: failed: expected e.what() == std::string("Future already retrieved") [std::future_error: Future already retrieved == Future already retrieved]

The test still passed. The fix's commit message says the KJ_EXPECT "prints error output without causing the test to fail", possibly because it is not called on the main test thread, and leaves that for follow-up.

Root cause

The test sends three simultaneous requests on one server thread, and the server-side function calls signal.get_future().get(). The first request runs it and blocks, the second waits in the thread's queue, and the third is rejected with "thread busy", whose error handler calls signal.set_value(). The first call then returns and the second runs the function again, where get_future() throws std::future_error with future_already_retrieved. The test caught std::exception and compared e.what() to "Future already retrieved". libstdc++ builds the message as "std::future_error: " + code.message(), which is the prefixed text in the output above, and libc++ returns "The future has already been retrieved from the promise or packaged_task." So the check failed with both standard libraries from the day the test merged, and went unnoticed because the failed expectation did not fail the test.

The fix's commit message calls the string match "fragile" and says it "does not work on all platforms"; since it matches neither libstdc++'s nor libc++'s text, it most likely did not work on any platform the tests run on.

The code

The test "Make simultaneous IPC callbacks with same request_thread and callback_thread" in test/mp/test/test.cpp, quoted from c332774409ad^. The server function waits on a promise, and expects a second attempt to get its future to fail with a particular message:

setup.server->m_impl->m_fn = [&] {
    try
    {
        signal.get_future().get();
    }
    catch(const std::exception& e)
    {
        KJ_EXPECT(e.what() == std::string("Future already retrieved"));
    }
};

The test sends three requests on one server thread. The first runs m_fn and blocks in get(), the second waits in the thread's queue, and the third is rejected with "thread busy", whose error handler calls signal.set_value(). The first call then returns and the second runs m_fn, where get_future() throws std::future_error with future_already_retrieved. Its what() text is up to the standard library: libstdc++ gives "std::future_error: Future already retrieved" and libc++ "The future has already been retrieved from the promise or packaged_task.", so the comparison fails with both. The failure was printed but did not fail the test, because the KJ_EXPECT runs on a server worker thread rather than the main test thread.

The fix (c332774409ad) checks the error code instead (include cleanups omitted):

@@ -333,9 +333,9 @@ KJ_TEST("Make simultaneous IPC callbacks with same request_thread and callback_t
         {
             signal.get_future().get();
         }
-        catch(const std::exception& e)
+        catch (const std::future_error& e)
         {
-            KJ_EXPECT(e.what() == std::string("Future already retrieved"));
+            KJ_EXPECT(e.code() == std::make_error_code(std::future_errc::future_already_retrieved));
         }

std::future_errc::future_already_retrieved is defined by the standard, so the check no longer depends on how a particular library words the message.

Fix

The fix commit c332774409ad catches std::future_error and compares e.code() with std::make_error_code(std::future_errc::future_already_retrieved), which does not depend on the message text (+5/-5, including include cleanups). @Eunovo suggested dropping the check; ryanofsky and @Sjors kept it as documentation of the expected error (1, 2).

Timeline

• 2025-10-02: introduced by 1238170f68e8, merged in mp#214.
• 2025-10-02: noticed by ryanofsky in mptest output.
• 2025-10-07: fix merged in mp#222.
• 2025-10-10: shipped to Bitcoin Core master in the subtree update bitcoin#33518; 2025-10-16: to the 30.x branch in the subtree update bitcoin#33519.

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.