rpc: Avoid crash when g_thread_http was never started #19006

Avoid a crash during shutdown when the init sequence failed for some reason

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #15363 (wip: http: Exit the event loop as soon as there are no active events by promag)

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.

Marco, any chance you could start making formatting cleanups in separate commits? Trying to skim this PR and quickly figure out what the bug is, and what the fix is is pointlessly difficult. It's like playing Where's Waldo.

Code review ACK faf838bf1cae719c46211ea603848f367ee427cf, though it looks like the new test might be failing to trigger the bug some cases and causing travis errors

Marco, any chance you could start making formatting cleanups in separate commits?

Are you referring to the first commit? My reasoning is that highlighting the call stack would make review easier, as it can be done with a single git command (instead of having to manually grep for the call stack). This is the way I would prefer when reviewing. Though, I will never review my own commits and if this makes review harder for others, I am happy to drop this habit.

Marco, any chance you could start making formatting cleanups in separate commits?

Are you referring to the first commit? My reasoning is that highlighting the call stack would make review easier, as it can be done with a single git command (instead of having to manually grep for the call stack). This is the way I would prefer when reviewing. Though, I will never review my own commits and if this makes review harder for others, I am happy to drop this habit.

The issue is that most of the time when I am looking at a diff I am not reviewing it, I'm trying to understand what it does. I review it after I understand what it does, not before. Making different changes in the same commit makes it take longer to figure out what the commit does.

But I actually don't understand what you're referring to with single git commands and calls stacks. I definitely think it's good to have a bugfix and tests for the bug in a single commit instead of different commits, because understanding the test can help you understand the bugfix and vice versa. I'm just requesting moving reformatting changes to separate commits from real code changes, because these aren't helpful to view together. Real changes break the flow of reviewing reformatting changes, and reformatting changes make it harder to find and notice real changes

Are you referring to the first commit?

I'm referring to no-op reformatting changes both commits: braces, whitespace, comments, variable renames, python log tweaks in both commits that get in the way of understanding the bugfix and test. The cleanups are great, I'm just suggesting to put them in a cleanup commit.

Marco, any chance you could start making formatting cleanups in separate commits

I think going forward, you can say stuff like "Concept ACK, but I refuse to review this as long as formatting cleanups are in the same commit as bug fixes". (This leaves me with no option other than addressing the feedback and I will adjust my behaviour in the future to avoid a review round-trip time)

Tested ACK faf45d1f1f997c316fc4c611a23c4456533eefe9.

Maybe do the same check in StopTorControl?

@promag StopTorControl should be unaffected by the bug, no? Or do you have steps to reproduce?

Code review ACK faf45d1f1f997c316fc4c611a23c4456533eefe9. Thanks for updates, this is much easier to parse for me now. Since previous reviews: split out and reverted some cleanups & replaced chmod with mkdir in test

I think going forward, you can say stuff like "Concept ACK, but I refuse to review this as long as formatting cleanups are in the same commit as bug fixes"

Sorry, I should have prefixed that with concept ACK. I wasn't refusing to review it though, and followed up with a regular code review ACK a few minutes after.

This leaves me with no option other than addressing the feedback and I will adjust my behaviour in the future to avoid a review round-trip time

I was just trying to suggest something to make this easier to review. Sometimes I forget to write it but you can always ignore my other feedback when I ACK something (I wouldn't ACK something I didn't think was an improvement.)

Concept ACK.

re #19006 (comment)

Or do you have steps to reproduce?

No, I don't think it's possible to hit the same problem there.

ACK faf45d1f1f997c316fc4c611a23c4456533eefe9, tested on Linux Mint 19.3 with the following patch:

--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -249,7 +249,7 @@ static bool InitRPCAuthentication()
     if (gArgs.GetArg("-rpcpassword", "") == "")
     {
         LogPrintf("No rpcpassword set - using random cookie authentication.\n");
-        if (!GenerateAuthCookie(&strRPCUserColonPass)) {
+        if (GenerateAuthCookie(&strRPCUserColonPass)) {
             return false;
         }
     } else {

No crash as expected.

The excerpt from the log:

2020-05-20T08:41:15Z [init] HTTP: creating work queue of depth 16
2020-05-20T08:41:15Z [init] No rpcpassword set - using random cookie authentication.
2020-05-20T08:41:15Z [init] Generated RPC authentication cookie /home/hebasto/.bitcoin/.cookie
2020-05-20T08:41:15Z [init] Error: Unable to start HTTP server. See debug log for details.
2020-05-20T08:41:15Z [init] Shutdown: In progress...
2020-05-20T08:41:15Z [scheduler] scheduler thread exit
2020-05-20T08:41:15Z [shutoff] Shutdown: done