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

pull willcl-ark wants to merge 1 commits into bitcoin:master from willcl-ark:fix-pidfile-delete changing 2 files +21 −7
  1. willcl-ark commented at 10:42 am on November 27, 2023: contributor
    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.
  2. 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.

  3. willcl-ark commented at 10:45 am on November 27, 2023: contributor

    You can test similarly to #28784:

     0will@ubuntu in ~ :
 1₿ bitcoind -regtest -daemon=1
 2Bitcoin Core starting
 3
 4will@ubuntu in ~ : 
 5₿ file ~/.bitcoin/regtest/bitcoind.pid
 6/home/will/.bitcoin/regtest/bitcoind.pid: ASCII text
 7
 8will@ubuntu in ~ :
 9₿ bitcoind -regtest  -daemon=1
10Error: Cannot obtain a lock on data directory /home/will/.bitcoin/regtest. Bitcoin Core is probably already running.
11
12will@ubuntu in ~ :
13₿ file ~/.bitcoin/regtest/bitcoind.pid
14/home/will/.bitcoin/regtest/bitcoind.pid: cannot open `/home/will/.bitcoin/regtest/bitcoind.pid' (No such file or directory)
  4. 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: 
    0            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.

  5. 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.
  6. 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
  7. willcl-ark force-pushed on Nov 27, 2023
  8. willcl-ark commented at 10:54 am on November 27, 2023: contributor
    Thanks @maflcko, I took the suggestions in 98ba5ba0cc0cb94b51595248041db972a9ef5a09
  9. 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

    0    if (std::error_code error; !fs::remove(GetPidFile(args), error)) {
1        std::string msg{error ? error.message() : "File does not exist"};
2        LogPrintf("Unable to remove PID file: %s\n", msg);
3    }
    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.
  10. 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
  11. 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:

     0diff --git a/src/init.cpp b/src/init.cpp
 1index afa0ab1ee6..f2d74db13f 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -96,6 +96,7 @@
 5 #include <cstdio>
 6 #include <fstream>
 7 #include <functional>
 8+#include <optional>
 9 #include <set>
10 #include <string>
11 #include <thread>
12@@ -155,7 +156,7 @@ static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map";
13  * The PID file facilities.
14  */
15 static const char* BITCOIN_PID_FILENAME = "bitcoind.pid";
16-static bool g_generated_pid{false};
17+static std::optional<fs::path> g_generated_pid;
18 
19 static fs::path GetPidFile(const ArgsManager& args)
20 {
21@@ -164,25 +165,26 @@ static fs::path GetPidFile(const ArgsManager& args)
22 
23 [[nodiscard]] static bool CreatePidFile(const ArgsManager& args)
24 {
25-    std::ofstream file{GetPidFile(args)};
26+    const fs::path pid_path{GetPidFile(args)};
27+    std::ofstream file{pid_path};
28     if (file) {
29 #ifdef WIN32
30         tfm::format(file, "%d\n", GetCurrentProcessId());
31 #else
32         tfm::format(file, "%d\n", getpid());
33 #endif
34-        g_generated_pid = true;
35+        g_generated_pid = pid_path;
36         return true;
37     } else {
38-        return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));
39+        return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(pid_path), SysErrorString(errno)));
40     }
41 }
42 
43-static void RemovePidFile(const ArgsManager& args)
44+static void RemovePidFile()
45 {
46     if (!g_generated_pid) return;
47     try {
48-        if (!fs::remove(GetPidFile(args))) {
49+        if (!fs::remove(g_generated_pid.value())) {
50             LogPrintf("Unable to remove PID file: File does not exist\n");
51         }
52     } catch (const fs::filesystem_error& e) {
53@@ -364,7 +366,7 @@ void Shutdown(NodeContext& node)
54     node.scheduler.reset();
55     node.kernel.reset();
56 
57-    RemovePidFile(*node.args);
58+    RemovePidFile();
59 
60     LogPrintf("%s: done\n", __func__);
61 }
    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.
  12. stickies-v commented at 2:41 pm on November 27, 2023: contributor
    Concept ACK
  13. willcl-ark force-pushed on Nov 27, 2023
  14. achow101 commented at 2:56 pm on November 28, 2023: member
    ACK e56e8cdf9cda1712f4a415aa9af34ced3e78dfb2
  15. DrahtBot requested review from stickies-v on Nov 28, 2023
  16. 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.
  17. DrahtBot requested review from stickies-v on Nov 28, 2023
  18. willcl-ark force-pushed on Nov 29, 2023
  19. 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

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

     0diff --git a/src/init.cpp b/src/init.cpp
 1index fced24a6ff..7a87467252 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -177,7 +177,10 @@ static fs::path GetPidFile(const ArgsManager& args)
 5         return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));
 6     }
 7 }
 8-
 9+/**
10+ * Remove PID file, but only if it was also generated by this process to avoid problems when
11+ * multiple bitcoind processes are started on the same datadir.
12+ */
13 static void RemovePidFile(const ArgsManager& args)
14 {
15     if (!g_generated_pid) return;
  20. 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

    0    const auto pid_path{GetPidFile(args)};
  21. 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?

    0        LogPrintf("Unable to remove PID file (%s): %s\n", fs::PathToString(pid_path), msg);
  22. stickies-v approved
  23. stickies-v commented at 11:16 am on November 29, 2023: contributor
    ACK 82f18e907a2af5a47e805861234de4672ed6d801
  24. DrahtBot requested review from achow101 on Nov 29, 2023
  25. willcl-ark commented at 11:49 am on December 1, 2023: contributor
    @stickies-v took most of your suggestions in 2b28f7df2b7dd140e52e93ef3e1a330db8da32a9
  26. willcl-ark force-pushed on Dec 1, 2023
  27. 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

    0//! True if this process created the pid file, so we don't delete it too early if multiple processes were started
  28. stickies-v approved
  29. stickies-v commented at 4:11 pm on December 1, 2023: contributor
    ACK 2b28f7df2b7dd140e52e93ef3e1a330db8da32a9
  30. DrahtBot added the label Needs rebase on Dec 1, 2023
  31. 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
  32. willcl-ark force-pushed on Dec 4, 2023
  33. DrahtBot removed the label Needs rebase on Dec 4, 2023
  34. andrewtoth approved
  35. andrewtoth commented at 5:04 pm on December 4, 2023: contributor
    ACK 8f6ab318635d823a7e6ad9020ac17b8d2dd0e91b
  36. DrahtBot requested review from stickies-v on Dec 4, 2023
  37. romanz approved
  39. achow101 commented at 7:27 pm on December 4, 2023: member
    ACK 8f6ab318635d823a7e6ad9020ac17b8d2dd0e91b
  40. DrahtBot removed review request from achow101 on Dec 4, 2023
  41. achow101 merged this on Dec 4, 2023
  42. achow101 closed this on Dec 4, 2023


willcl-ark DrahtBot maflcko stickies-v achow101 andrewtoth romanz


stickies-v

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me