init: don't delete PID file if it was not generated #28946

willcl-ark commented at 10:42 AM on November 27, 2023: member

In a similar vein to #28784, if a second bitcoind is started using the same datadir it will fail to start up, but during shutdown remove the PID file from the first bitcoind instance.

DrahtBot commented at 10:42 AM on November 27, 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.

Type	Reviewers
ACK	andrewtoth, romanz, achow101
Stale ACK	stickies-v

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:

#28051 (Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly by ryanofsky)

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.

willcl-ark commented at 10:45 AM on November 27, 2023: member

You can test similarly to #28784:

will@ubuntu in ~ :
₿ bitcoind -regtest -daemon=1
Bitcoin Core starting

will@ubuntu in ~ : 
₿ file ~/.bitcoin/regtest/bitcoind.pid
/home/will/.bitcoin/regtest/bitcoind.pid: ASCII text

will@ubuntu in ~ :
₿ bitcoind -regtest  -daemon=1
Error: Cannot obtain a lock on data directory /home/will/.bitcoin/regtest. Bitcoin Core is probably already running.

will@ubuntu in ~ :
₿ file ~/.bitcoin/regtest/bitcoind.pid
/home/will/.bitcoin/regtest/bitcoind.pid: cannot open `/home/will/.bitcoin/regtest/bitcoind.pid' (No such file or directory)

in src/init.cpp:186 in 5eefe8d243 outdated

 181 | +static void RemovePidFile(NodeContext& node)
 182 | +{
 183 | +    if (!g_generated_pid) return;
 184 | +    try {
 185 | +        if (!fs::remove(GetPidFile(*node.args))) {
 186 | +            LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__);

maflcko commented at 10:48 AM on November 27, 2023:

            LogPrintf("Unable to remove PID file: File does not exist\n");

nit: No need to take this over, if you change the function name, the warning is already unique, and user can enable to print the function name, if they really want to.

in src/init.cpp:185 in 5eefe8d243 outdated

 180 |  
 181 | +static void RemovePidFile(NodeContext& node)
 182 | +{
 183 | +    if (!g_generated_pid) return;
 184 | +    try {
 185 | +        if (!fs::remove(GetPidFile(*node.args))) {

maflcko commented at 10:48 AM on November 27, 2023:

nit: No need to pass node, when you only need args?

willcl-ark commented at 10:51 AM on November 27, 2023:

Oh yeah, that's nicer. Will fix.

in src/init.cpp:189 in 5eefe8d243 outdated

 184 | +    try {
 185 | +        if (!fs::remove(GetPidFile(*node.args))) {
 186 | +            LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__);
 187 | +        }
 188 | +    } catch (const fs::filesystem_error& e) {
 189 | +        LogPrintf("%s: Unable to remove PID file: %s\n", __func__, fsbridge::get_filesystem_error_message(e));

maflcko commented at 10:48 AM on November 27, 2023:

same

willcl-ark force-pushed on Nov 27, 2023

willcl-ark commented at 10:54 AM on November 27, 2023: member

Thanks @maflcko, I took the suggestions in 98ba5ba0cc0cb94b51595248041db972a9ef5a09

in src/init.cpp:192 in 98ba5ba0cc outdated

 185 | +        if (!fs::remove(GetPidFile(args))) {
 186 | +            LogPrintf("Unable to remove PID file: File does not exist\n");
 187 | +        }
 188 | +    } catch (const fs::filesystem_error& e) {
 189 | +        LogPrintf("Unable to remove PID file: %s\n", fsbridge::get_filesystem_error_message(e));
 190 | +    }

stickies-v commented at 2:18 PM on November 27, 2023:

nit: can be simplified

    if (std::error_code error; !fs::remove(GetPidFile(args), error)) {
        std::string msg{error ? error.message() : "File does not exist"};
        LogPrintf("Unable to remove PID file: %s\n", msg);
    }

willcl-ark commented at 2:51 PM on November 27, 2023:

Is passing error codes our preferred error handling technique, or just slightly shorter?

I'd naturally opt for the former, and I think errors from it will be slightly more detailed, including an error msg instead of only error code (although it doesn't make much difference in this case), but can very easily be persuaded to switch to the latter if it's preferred...

stickies-v commented at 3:31 PM on November 27, 2023:

I just like the brevity of it. I'm using error.message() and not error.value() so the user will see a user friendly error message. Could include the pid path in the log line if preferred, probably helpful to the user to know which file to change permissions on indeed.

For example, with this patch and chmod a-w on the datadir this will print:

Unable to remove PID file: Permission denied

For a missing file, this will print

Unable to remove PID file: File does not exist

willcl-ark commented at 3:58 PM on November 27, 2023:

I've taken this in e56e8cdf9cda1712f4a415aa9af34ced3e78dfb2 and added the pid path to the log message.

in test/functional/feature_filelock.py:33 in 98ba5ba0cc outdated

  29 | @@ -30,6 +30,10 @@ def run_test(self):
  30 |          expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['PACKAGE_NAME']} is probably already running."
  31 |          self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir_path}', '-noserver'], expected_msg=expected_msg)
  32 |  
  33 | +        self.log.info("Check that cookie and PID files are not deleted when attempting to start a second bitcoind using the same datadir")

stickies-v commented at 2:25 PM on November 27, 2023:

cookie file is not yet checked here, would leave that for the other pull

willcl-ark commented at 2:44 PM on November 27, 2023:

Oh, thanks. I wrote this commit on top as I was reviewing the other one, then cherry-picked and made it standalone. I guess this slipped through.

willcl-ark commented at 3:58 PM on November 27, 2023:

This is fixed in e56e8cdf9cda1712f4a415aa9af34ced3e78dfb2

in src/init.cpp:185 in 98ba5ba0cc outdated

 176 |      } else {
 177 |          return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));
 178 |      }
 179 |  }
 180 |  
 181 | +static void RemovePidFile(const ArgsManager& args)

stickies-v commented at 2:39 PM on November 27, 2023:

~#28784 stores the path in static std::optional<std::string> g_generated_cookie~. I don't have a particularly strong conviction in either direction, but I'd use the same approach for both. Storing the location of the file to be deleted seems a bit more robust, so I think I'd lean towards this approach:

diff --git a/src/init.cpp b/src/init.cpp
index afa0ab1ee6..f2d74db13f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -96,6 +96,7 @@
 #include <cstdio>
 #include <fstream>
 #include <functional>
+#include <optional>
 #include <set>
 #include <string>
 #include <thread>
@@ -155,7 +156,7 @@ static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map";
  * The PID file facilities.
  */
 static const char* BITCOIN_PID_FILENAME = "bitcoind.pid";
-static bool g_generated_pid{false};
+static std::optional<fs::path> g_generated_pid;
 
 static fs::path GetPidFile(const ArgsManager& args)
 {
@@ -164,25 +165,26 @@ static fs::path GetPidFile(const ArgsManager& args)
 
 [[nodiscard]] static bool CreatePidFile(const ArgsManager& args)
 {
-    std::ofstream file{GetPidFile(args)};
+    const fs::path pid_path{GetPidFile(args)};
+    std::ofstream file{pid_path};
     if (file) {
 #ifdef WIN32
         tfm::format(file, "%d\n", GetCurrentProcessId());
 #else
         tfm::format(file, "%d\n", getpid());
 #endif
-        g_generated_pid = true;
+        g_generated_pid = pid_path;
         return true;
     } else {
-        return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));
+        return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(pid_path), SysErrorString(errno)));
     }
 }
 
-static void RemovePidFile(const ArgsManager& args)
+static void RemovePidFile()
 {
     if (!g_generated_pid) return;
     try {
-        if (!fs::remove(GetPidFile(args))) {
+        if (!fs::remove(g_generated_pid.value())) {
             LogPrintf("Unable to remove PID file: File does not exist\n");
         }
     } catch (const fs::filesystem_error& e) {
@@ -364,7 +366,7 @@ void Shutdown(NodeContext& node)
     node.scheduler.reset();
     node.kernel.reset();
 
-    RemovePidFile(*node.args);
+    RemovePidFile();
 
     LogPrintf("%s: done\n", __func__);
 }

</details>

stickies-v commented at 2:44 PM on November 27, 2023:

Oh, looking at 28784 more closely it seems g_generated_cookie stores the cookie, not the path. My bad.

willcl-ark commented at 2:48 PM on November 27, 2023:

I prefer the idea of storing a bool with whether it has been generated or not, and use the {Get|Create|Remove}PidFile() functions as necessary.

stickies-v commented at 3:33 PM on November 27, 2023:

I'm okay with the boolean approach if you prefer it. I just like the "we created this file, so let's remove this file" explicitness over "we created a file, so let's remove a file". But in practice, no big difference.

stickies-v commented at 2:41 PM on November 27, 2023: contributor

Concept ACK

willcl-ark force-pushed on Nov 27, 2023

achow101 commented at 2:56 PM on November 28, 2023: member

ACK e56e8cdf9cda1712f4a415aa9af34ced3e78dfb2

DrahtBot requested review from stickies-v on Nov 28, 2023

in src/init.cpp:186 in e56e8cdf9c outdated

 181 | +static void RemovePidFile(const ArgsManager& args)
 182 | +{
 183 | +    if (!g_generated_pid) return;
 184 | +    if (std::error_code error; !fs::remove(GetPidFile(args), error)) {
 185 | +        std::string msg{error ? error.message() : "File does not exist"};
 186 | +        auto pid_path = args.GetPathArg("-pid", BITCOIN_PID_FILENAME);

stickies-v commented at 10:38 PM on November 28, 2023:

Wouldn't it be better to just use GetPidFile()?

willcl-ark commented at 9:54 AM on November 29, 2023:

Agree that's cleaner. Fixed in 82f18e907a along with a typo in the commit message.

DrahtBot requested review from stickies-v on Nov 28, 2023

willcl-ark force-pushed on Nov 29, 2023

in src/init.cpp:185 in 82f18e907a outdated

 176 |      } else {
 177 |          return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));
 178 |      }
 179 |  }
 180 |  
 181 | +static void RemovePidFile(const ArgsManager& args)

stickies-v commented at 11:05 AM on November 29, 2023:

nit: quick docstring would be nice

/**
 * Remove PID file, but only if it was also generated by this process to avoid problems when
 * multiple bitcoind processes are started on the same datadir.
 */

diff --git a/src/init.cpp b/src/init.cpp
index fced24a6ff..7a87467252 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -177,7 +177,10 @@ static fs::path GetPidFile(const ArgsManager& args)
         return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));
     }
 }
-
+/**
+ * Remove PID file, but only if it was also generated by this process to avoid problems when
+ * multiple bitcoind processes are started on the same datadir.
+ */
 static void RemovePidFile(const ArgsManager& args)
 {
     if (!g_generated_pid) return;

</details>

in src/init.cpp:184 in 82f18e907a outdated

 179 |  }
 180 |  
 181 | +static void RemovePidFile(const ArgsManager& args)
 182 | +{
 183 | +    if (!g_generated_pid) return;
 184 | +    auto pid_path = GetPidFile(args);

stickies-v commented at 11:05 AM on November 29, 2023:

nit

    const auto pid_path{GetPidFile(args)};

in src/init.cpp:187 in 82f18e907a outdated

 182 | +{
 183 | +    if (!g_generated_pid) return;
 184 | +    auto pid_path = GetPidFile(args);
 185 | +    if (std::error_code error; !fs::remove(pid_path, error)) {
 186 | +        std::string msg{error ? error.message() : "File does not exist"};
 187 | +        LogPrintf("Unable to remove PID file %s: %s\n", fs::quoted(fs::PathToString(pid_path)), msg);

stickies-v commented at 11:15 AM on November 29, 2023:

nit: I'm not sure how important the quoting is, could just use parentheses to make it a bit more concise?

        LogPrintf("Unable to remove PID file (%s): %s\n", fs::PathToString(pid_path), msg);

stickies-v approved

stickies-v commented at 11:16 AM on November 29, 2023: contributor

ACK 82f18e907a2af5a47e805861234de4672ed6d801

DrahtBot requested review from achow101 on Nov 29, 2023

willcl-ark commented at 11:49 AM on December 1, 2023: member

@stickies-v took most of your suggestions in 2b28f7df2b7dd140e52e93ef3e1a330db8da32a9

willcl-ark force-pushed on Dec 1, 2023

in src/init.cpp:161 in 2b28f7df2b outdated

 154 | @@ -155,6 +155,11 @@ static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map";
 155 |   * The PID file facilities.
 156 |   */
 157 |  static const char* BITCOIN_PID_FILENAME = "bitcoind.pid";
 158 | +/**
 159 | + * Whether this process has created a PID file.
 160 | + * Used when determining whether we should remove the PID file on shutdown.
 161 | + */

stickies-v commented at 4:10 PM on December 1, 2023:

nit: I think this more clearly explains the rationale without being too explicit about implementation

//! True if this process created the pid file, so we don't delete it too early if multiple processes were started

stickies-v approved

stickies-v commented at 4:11 PM on December 1, 2023: contributor

ACK 2b28f7df2b7dd140e52e93ef3e1a330db8da32a9

DrahtBot added the label Needs rebase on Dec 1, 2023

init: don't delete PID file if it was not generated

Previously, starting a second bitcoind using the same datadir would
correctly fail to init and shutdown. However during shutdown the PID
file belonging to the first instance would be erroneously removed by
the second process shutting down.

Fix this to only delete the PID file if we created it.

8f6ab31863

willcl-ark force-pushed on Dec 4, 2023

DrahtBot removed the label Needs rebase on Dec 4, 2023

andrewtoth approved