test: don't throw from the destructor of DebugLogHelper

vasild commented at 9:03 am on September 15, 2025: contributor

Throwing an exception from the destructor of a class is a bad practice because the destructor will be called when an object of that type is alive on the stack and another exception is thrown, which will result in “exception during the exception”. This would terminate the program without any messages.

Instead print the message to the standard error output and call std::abort().

This change is part of #26812. It is an improvement on its own, so creating a separate PR for it following the discussion at #32604 (review). Getting it in will reduce the size of #26812.

DrahtBot added the label Tests on Sep 15, 2025

DrahtBot commented at 9:03 am on September 15, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33388.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	optout21, Crypt-iQ, furszy, l0rinc
Concept ACK	enirox001

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

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.

optout21 commented at 2:00 pm on September 15, 2025: none

ACK 7f87cddd36f0456be52d795a53887d53294f824e

I’ve reviewed, I’ve executed the unit tests. The change is Test only. The relevant change is in DebugLogHelper destructor, exception throwing is replaced by print and abort(). Other changes improve documentation, parameters of the class. Changes are not in separate commits, but changes are small and test-only.

Crypt-iQ commented at 2:45 pm on September 15, 2025: contributor

ACK 7f87cddd36f0456be52d795a53887d53294f824e

purpleKarrot commented at 2:49 pm on September 15, 2025: contributor

Throwing an exception from the destructor of a class is a bad practice because the destructor will be called when an object of that type is alive on the stack and another exception is thrown, which will result in “exception during the exception”. This would terminate the program without any messages.

This is not precisely what happens. If an exception is emitted from a function marked noexcept(false), the current terminate handler will be invoked. It does not matter whether the destructor is called due to another exception or not. The default terminate handler would print the exception and then call abort.

The proposed implementation prints the error and then calls abort.

So basically, the change is that the class is no longer sensitive to a custom terminate handler. Is that really desired?

ryanofsky commented at 5:29 pm on September 15, 2025: contributor

Cap’n Proto has a policy of declaring destructors noexcept(false) and using std::uncaught_exception() to prevent throwing more than once under the assumption that “ff another exception is already active, the new exception is assumed to be a side-effect of the main exception, and is either silently swallowed or reported on a side channel.” It seems like that could be a reasonable approach here. The DebugLogHelper destructor could continue to throw normally, but just log instead in the case where another exception was active. And if caller cared to prevent this, it could call the check_found method explicitly outside of the destructor.

More information about cap’n proto’s policy:

Not endorsing it generally but it might be a good fit here.

in src/test/util/logging.cpp:31 in 7f87cddd36 outdated

27     noui_reconnect();
28     LogInstance().DeleteCallback(m_print_connection);
29     if (!m_found && m_match(nullptr)) {
30-        throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
31+        std::cerr << "Fatal error: expected message not found in the debug log: " << m_message << "\n";
32+        std::abort();

l0rinc commented at 6:42 pm on September 15, 2025:

Please add #include <cstdlib> for std::abort()

vasild commented at 2:06 pm on September 18, 2025:

Done.

in src/test/util/logging.cpp:13 in 7f87cddd36

 7@@ -8,10 +8,10 @@
 8 #include <noui.h>
 9 #include <tinyformat.h>
10 
11-#include <stdexcept>
12+#include <iostream>
13 
14-DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
15-    : m_message{std::move(message)}, m_match(std::move(match))
16+DebugLogHelper::DebugLogHelper(const std::string& message, MatchFn match)

l0rinc commented at 6:43 pm on September 15, 2025:

This parameter change is a separate concern. Please split it into its own commit to keep the risky behavior change easy to review. Though personally, I’d pass both by value and move into the members to handle rvalues efficiently.

enirox001 commented at 9:26 pm on September 16, 2025:

This commit mixes exception safety fixes with parameter optimization. Like @l0rinc mention perhaps consider splitting the parameter changes into a separate commit for cleaner git history and easier debugging.

vasild commented at 2:06 pm on September 18, 2025:

Removed the parameter change altogether.

vasild commented at 2:12 pm on September 18, 2025:

Dropped altogether.

in src/test/util/logging.h:44 in 7f87cddd36

39+     * - if the destructor is called without a successful match, then this function will be
40+     *   called with a `nullptr` argument to decide what the final action should be:
41+     *   - if it returns `true`, then a fatal error will be produced
42+     *   - if it returns `false`, then the no-match will be ignored and operation will
43+     *     continue normally
44+     */

l0rinc commented at 7:18 pm on September 15, 2025:

I’m very much against these long and scary comments - seems like a strong code smell. The comment will quickly reflect an old version of the code since it’s not high-level enough and we simply won’t update it when the code changes. And honestly reading the whole thing didn’t help me with understanding the gist of it.

If we feel the code isn’t self-explanatory we should adjust the code instead of writing long documentation for it. If you still feel docs are needed, let’s do it more high-level without all the details that the code can already tell us, maybe something like:

0    /**
1     * RAII test helper that asserts a log line containing `message` is emitted during its lifetime.
2     * The optional `match` predicate is called for each such line, and once at destruction with `nullptr`, if none.
3     * If that final call returns true, an error is printed and the process aborts (default).
4     */

vasild commented at 2:06 pm on September 18, 2025:

Reverted to the original comment and its location.

in src/test/util/logging.cpp:30 in 7f87cddd36 outdated

26 {
27     noui_reconnect();
28     LogInstance().DeleteCallback(m_print_connection);
29     if (!m_found && m_match(nullptr)) {
30-        throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
31+        std::cerr << "Fatal error: expected message not found in the debug log: " << m_message << "\n";

l0rinc commented at 7:34 pm on September 15, 2025:

LogError would flush after this line, so we could safely abort after (since abort doesn’t call any destructors). Can you confirm that cerr will also flush safely?

vasild commented at 2:08 pm on September 18, 2025:

Could add std::cerr.flush(), but I think it is not necessary because cerr is unbuffered.

l0rinc commented at 6:33 pm on September 18, 2025:

That is my understanding as well, thanks

in src/test/util/logging.cpp:28 in 7f87cddd36 outdated

20@@ -21,11 +21,12 @@ DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
21     noui_test_redirect();
22 }
23 
24-void DebugLogHelper::check_found()
25+DebugLogHelper::~DebugLogHelper()
26 {
27     noui_reconnect();
28     LogInstance().DeleteCallback(m_print_connection);

l0rinc commented at 7:35 pm on September 15, 2025:

I admit that I don’t understand this well enough, why reconnect before deleting the callback? Shouldn’t we delete the callback first, then call noui_reconnect()?

vasild commented at 2:09 pm on September 18, 2025:

Hmm, I am not sure either. Maybe it should be done like you say. Unrelated to this PR.

in src/test/util/logging.h:45 in 7f87cddd36

40+     *   called with a `nullptr` argument to decide what the final action should be:
41+     *   - if it returns `true`, then a fatal error will be produced
42+     *   - if it returns `false`, then the no-match will be ignored and operation will
43+     *     continue normally
44+     */
45+    explicit DebugLogHelper(const std::string& message, MatchFn match = [](const std::string*){ return true; });

l0rinc commented at 7:40 pm on September 15, 2025:

nit: we can simplify it a bit (+ pass by value mentioned earlier):

0    explicit DebugLogHelper(std::string message, MatchFn match = [](auto){ return true; });

vasild commented at 2:10 pm on September 18, 2025:

Removed any changes to those parameters.

l0rinc commented at 8:22 pm on September 15, 2025: contributor

using std::uncaught_exception() to prevent throwing

https://en.cppreference.com/w/cpp/error/uncaught_exception.html indicates std::uncaught_exception() is deprecated in C++17 and removed in C++20 - but we could use std::uncaught_exceptions().

Added a few comments, thanks for tackling this.

furszy commented at 8:52 pm on September 15, 2025: member

As DebugLogHelper is only used within the unit test framework; could use BOOST_REQUIRE with a message within the destructor too.

in src/test/util/logging.cpp:14 in 7f87cddd36

12+#include <iostream>
13 
14-DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
15-    : m_message{std::move(message)}, m_match(std::move(match))
16+DebugLogHelper::DebugLogHelper(const std::string& message, MatchFn match)
17+    : m_message{message}, m_match(std::move(match))

enirox001 commented at 9:23 pm on September 16, 2025:

The message parameter was optimized to const std::string&, but match still uses pass-by-value. For consistency, shouldn’t match also be const MatchFn&?

l0rinc commented at 9:33 pm on September 16, 2025:

I prefer passing both by value and moving into the members, since most call sites pass temporaries from string literals for the message.

enirox001 commented at 9:45 pm on September 16, 2025:

Good point. Using pass-by-value + move for both parameters would indeed solve the consistency issue while being optimal for the common case of string literals. That approach makes both parameters follow the same pattern.

vasild commented at 2:12 pm on September 18, 2025:

I dropped the changes to the parameters since they are not the aim of this PR and they caused disproportional amount of discussion.

enirox001 commented at 9:27 pm on September 16, 2025: contributor

Concept ACK 7f87cdd

vasild force-pushed on Sep 18, 2025

vasild commented at 2:05 pm on September 18, 2025: contributor

7f87cddd36...ba6689aa9e: drop some unrelated changes and pick some suggestions

purpleKarrot commented at 2:24 pm on September 18, 2025: contributor

Unrelated to the PR, but it is error prone that DebugLogHelper is copyable: https://godbolt.org/z/YdT5EWzMM

I suggest to augment the PR with a commit that contains:

0DebugLogHelper(const DebugLogHelper&) = delete;
1DebugLogHelper& operator=(const DebugLogHelper&) = delete;

vasild commented at 2:30 pm on September 18, 2025: contributor

@purpleKarrot,

Throwing an exception from the destructor of a class is a bad practice because the destructor will be called when an object of that type is alive on the stack and another exception is thrown, which will result in “exception during the exception”. This would terminate the program without any messages.

This is not precisely what happens. If an exception is emitted from a function marked noexcept(false), the current terminate handler will be invoked.

Hmm, not what I observe here:

 0void f() noexcept(false)
 1{
 2    throw std::runtime_error("exception from f()");
 3}
 4
 5int main(int, char**)
 6{
 7    try {
 8        f();
 9    } catch (const std::runtime_error& e) {
10        std::cerr << "catch: " << e.what() << "\n";
11    }
12    return 0;
13}

prints “catch: exception from f()”. No terminate handler is invoked.

It does not matter whether the destructor is called due to another exception or not.

It seems to matter because if I extend the program like this:

 0class A
 1{
 2public:
 3    ~A() noexcept(false)
 4    {
 5        throw std::runtime_error("exception from ~A()");
 6    }
 7};
 8...
 9    try {
10        A a;
11        f();
12    } catch ...

Then this is printed: “Terminating due to uncaught exception 0xd800c40110 of type std::runtime_error” and the program terminates. Note the missing error text from the exception. The same happens with try { std::vector<A> v(2); } catch ... as well.

The default terminate handler would print the exception and then call abort.

Yes, but without the error message of the exception, just “exception 0xd800c40110 of type std::runtime_error”.

The proposed implementation prints the error and then calls abort.

Prints the proper, more useful, error message.

So basically, the change is that the class is no longer sensitive to a custom terminate handler. Is that really desired?

The change is that a bad practice of throwing from a destructor is eliminated.

@ryanofsky, @l0rinc,

Cap’n Proto … std::uncaught_exception() …

The swallowing of the 2nd and subsequent exceptions seems to assume that one has control and has implemented the same policy in all destructors in the program. @furszy,

As DebugLogHelper is only used within the unit test framework; could use BOOST_REQUIRE with a message within the destructor too.

Good idea. I explored this, but it turns out that BOOST_REQUIRE() throws an exception on failure :/

vasild commented at 2:35 pm on September 18, 2025: contributor

@purpleKarrot

I suggest to augment the PR with a commit that contains…

Done

purpleKarrot commented at 2:51 pm on September 18, 2025: contributor

If an exception is emitted from a function marked noexcept(false), the current terminate handler will be invoked.

My bad. That is the behavior for noexcept(true), of course. I agree that marking a destructor with noexcept(false) is bad.

in src/test/util/logging.cpp:30 in 2dcf0c69b8

27 {
28     noui_reconnect();
29     LogInstance().DeleteCallback(m_print_connection);
30     if (!m_found && m_match(nullptr)) {
31-        throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
32+        std::cerr << "Fatal error: expected message not found in the debug log: " << m_message << "\n";

purpleKarrot commented at 2:55 pm on September 18, 2025:

The original error had the message string in quotes. I think this should be preserved because it is easier to debug when the string contains whitespace at the end.

l0rinc commented at 6:35 pm on September 18, 2025:

0        tfm::format(std::cerr, "Fatal error: expected message not found in the debug log: '%s'\n", m_message);

vasild commented at 9:29 am on September 19, 2025:

Done, thanks!

ryanofsky commented at 4:19 pm on September 18, 2025: contributor

re: #33388 (comment)

The swallowing of the 2nd and subsequent exceptions seems to assume that one has control and has implemented the same policy in all destructors in the program.

That’s not true, and wanted to be clear I wasn’t advocating the policy generally. I was just pointing out that letting this particular destructor throw as it was designed to do, instead of aborting, is easy to support in practice with the noexcept(false) std::uncaught_exceptions technique. And it seems like a nice and safe way to provide error feedback. But if you think it’s better to abort, that also seems fine.

in src/test/util/logging.h:38 in ba6689aa9e outdated

35+    ~DebugLogHelper();
36+
37+private:
38+    const std::string m_message;
39+    bool m_found{false};
40+    std::list<std::function<void(const std::string&)>>::iterator m_print_connection;

l0rinc commented at 6:37 pm on September 18, 2025:

nit/unrelated: I’m surprised to see an std::list here, std::vector is likely faster even for random erases

l0rinc approved

l0rinc commented at 6:44 pm on September 18, 2025: contributor

Core review ACK 2dcf0c69b8659886ecfc85b174e59cd7f210f5c0

I would also prefer adding quotes around the message but it’s not a blocker.

DrahtBot requested review from optout21 on Sep 18, 2025

DrahtBot requested review from enirox001 on Sep 18, 2025

Crypt-iQ commented at 8:28 pm on September 18, 2025: contributor

crACK 2dcf0c69b8659886ecfc85b174e59cd7f210f5c0

test: don't throw from the destructor of DebugLogHelper

Throwing an exception from the destructor of a class is a bad practice,
avoid that and instead print the message to the standard error output
and call `std::abort()`.

d6aa266d43

test: forbid copying of DebugLogHelper 2427939935

vasild force-pushed on Sep 19, 2025

vasild commented at 9:29 am on September 19, 2025: contributor

2dcf0c69b8...2427939935: take #33388 (review)

optout21 commented at 9:55 am on September 19, 2025: none

crACK 2427939935f3e6669be6bf553be89639e0afabaa

DrahtBot requested review from l0rinc on Sep 19, 2025

Crypt-iQ commented at 12:39 pm on September 19, 2025: contributor

crACK 2427939

in src/test/util/logging.cpp:30 in d6aa266d43 outdated

27 {
28     noui_reconnect();
29     LogInstance().DeleteCallback(m_print_connection);
30     if (!m_found && m_match(nullptr)) {
31-        throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
32+        tfm::format(std::cerr, "Fatal error: expected message not found in the debug log: '%s'\n", m_message);

furszy commented at 1:44 pm on September 19, 2025:

nit: “Fatal error: expected message ‘%s’ not found in the logs\n” otherwise it reads as you were printing the debug log at the end.

vasild commented at 12:21 pm on September 22, 2025:

Will do if I retouch. Change applied locally.

furszy commented at 1:44 pm on September 19, 2025: member

utACK 2427939935f3e6669be6bf553be89639e0afabaa

l0rinc commented at 11:45 pm on September 19, 2025: contributor

Code review reACK 2427939935f3e6669be6bf553be89639e0afabaa

glozow merged this on Sep 23, 2025

glozow closed this on Sep 23, 2025

vasild deleted the branch on Sep 24, 2025

test: don’t throw from the destructor of DebugLogHelper #33388

Code Coverage & Benchmarks

Reviews

Conflicts