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

willcl-ark · 2023-11-27T10:42:19Z

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 · 2023-11-27T10:42:22Z

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 · 2023-11-27T10:45:41Z

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)

maflcko · 2023-11-27T10:48:22Z

src/init.cpp

+    if (!g_generated_pid) return;
+    try {
+        if (!fs::remove(GetPidFile(*node.args))) {
+            LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__);


Suggested change

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

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.

maflcko · 2023-11-27T10:48:38Z

src/init.cpp

+{
+    if (!g_generated_pid) return;
+    try {
+        if (!fs::remove(GetPidFile(*node.args))) {


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

Oh yeah, that's nicer. Will fix.

maflcko · 2023-11-27T10:48:47Z

src/init.cpp

+            LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__);
+        }
+    } catch (const fs::filesystem_error& e) {
+        LogPrintf("%s: Unable to remove PID file: %s\n", __func__, fsbridge::get_filesystem_error_message(e));


willcl-ark · 2023-11-27T10:54:55Z

Thanks @maflcko, I took the suggestions in 98ba5ba

stickies-v

Concept ACK

stickies-v · 2023-11-27T14:18:54Z

src/init.cpp

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


nit: can be simplified

Suggested change

try {

if (!fs::remove(GetPidFile(args))) {

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

}

} catch (const fs::filesystem_error& e) {

LogPrintf("Unable to remove PID file: %s\n", fsbridge::get_filesystem_error_message(e));

}

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);

}

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...

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

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

stickies-v · 2023-11-27T14:25:09Z

test/functional/feature_filelock.py

@@ -30,6 +30,10 @@ def run_test(self):
        expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['PACKAGE_NAME']} is probably already running."
        self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir_path}', '-noserver'], expected_msg=expected_msg)

+        self.log.info("Check that cookie and PID files are not deleted when attempting to start a second bitcoind using the same datadir")


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

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.

This is fixed in e56e8cd

stickies-v · 2023-11-27T14:39:49Z

src/init.cpp

        return true;
    } else {
        return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));
    }
 }

+static void RemovePidFile(const ArgsManager& args)


~~#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:

git diff on 98ba5ba

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__); }

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

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.

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.

achow101 · 2023-11-28T14:56:11Z

ACK e56e8cd

stickies-v · 2023-11-28T22:38:39Z

src/init.cpp

+    if (!g_generated_pid) return;
+    if (std::error_code error; !fs::remove(GetPidFile(args), error)) {
+        std::string msg{error ? error.message() : "File does not exist"};
+        auto pid_path = args.GetPathArg("-pid", BITCOIN_PID_FILENAME);


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

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

stickies-v

ACK 82f18e9

stickies-v · 2023-11-29T11:05:13Z

src/init.cpp

        return true;
    } else {
        return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));
    }
 }

+static void RemovePidFile(const ArgsManager& args)


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. */

git diff on 82f18e9

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;

stickies-v · 2023-11-29T11:05:38Z

src/init.cpp

+static void RemovePidFile(const ArgsManager& args)
+{
+    if (!g_generated_pid) return;
+    auto pid_path = GetPidFile(args);


nit

Suggested change

auto pid_path = GetPidFile(args);

const auto pid_path{GetPidFile(args)};

stickies-v · 2023-11-29T11:15:57Z

src/init.cpp

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


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

Suggested change

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

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

willcl-ark · 2023-12-01T11:49:14Z

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

stickies-v

ACK 2b28f7d

stickies-v · 2023-12-01T16:10:53Z

src/init.cpp

+/**
+ * Whether this process has created a PID file.
+ * Used when determining whether we should remove the PID file on shutdown.
+ */


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

Suggested change

/**

* Whether this process has created a PID file.

* Used when determining whether we should remove the PID file on shutdown.

*/

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

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.

andrewtoth

ACK 8f6ab31

romanz

ACK 8f6ab31

achow101 · 2023-12-04T19:27:39Z

ACK 8f6ab31

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. Github-Pull: bitcoin#28946 Rebased-From: 8f6ab31

maflcko reviewed Nov 27, 2023

View reviewed changes

willcl-ark force-pushed the fix-pidfile-delete branch from 5eefe8d to 98ba5ba Compare November 27, 2023 10:54

stickies-v reviewed Nov 27, 2023

View reviewed changes

stickies-v mentioned this pull request Nov 27, 2023

rpc: keep .cookie file if it was not generated #28784

Merged

willcl-ark force-pushed the fix-pidfile-delete branch from 98ba5ba to e56e8cd Compare November 27, 2023 15:58

DrahtBot mentioned this pull request Nov 27, 2023

Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Merged

DrahtBot requested a review from stickies-v November 28, 2023 14:56

stickies-v reviewed Nov 28, 2023

View reviewed changes

DrahtBot requested a review from stickies-v November 28, 2023 22:38

willcl-ark force-pushed the fix-pidfile-delete branch from e56e8cd to 82f18e9 Compare November 29, 2023 09:54

stickies-v approved these changes Nov 29, 2023

View reviewed changes

DrahtBot requested a review from achow101 November 29, 2023 11:16

willcl-ark force-pushed the fix-pidfile-delete branch from 82f18e9 to 2b28f7d Compare December 1, 2023 11:49

stickies-v approved these changes Dec 1, 2023

View reviewed changes

DrahtBot added the Needs rebase label Dec 1, 2023

willcl-ark force-pushed the fix-pidfile-delete branch from 2b28f7d to 8f6ab31 Compare December 4, 2023 12:54

DrahtBot removed the Needs rebase label Dec 4, 2023

andrewtoth approved these changes Dec 4, 2023

View reviewed changes

DrahtBot requested a review from stickies-v December 4, 2023 17:04

romanz approved these changes Dec 4, 2023

View reviewed changes

DrahtBot removed the request for review from achow101 December 4, 2023 19:27

achow101 merged commit afdc4c3 into bitcoin:master Dec 4, 2023

bitcoin locked and limited conversation to collaborators Dec 3, 2024

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

	auto pid_path = GetPidFile(args);
	const auto pid_path{GetPidFile(args)};

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

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

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

Uh oh!

Conversation

willcl-ark commented Nov 27, 2023

Uh oh!

DrahtBot commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

willcl-ark commented Nov 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented Nov 27, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Nov 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewtoth left a comment

Choose a reason for hiding this comment

Uh oh!

romanz left a comment

DrahtBot commented Nov 27, 2023 •

edited

Loading

stickies-v Nov 27, 2023 •

edited

Loading

willcl-ark commented Dec 1, 2023 •

edited

Loading

stickies-v Dec 1, 2023 •

edited

Loading