Part of #19303.
Replace ArgsManager::cs_args from RecursiveMutex to Mutex.
The conversion follows the pattern established in prior RecursiveMutex removals (e.g. CAddrMan in #19238, CBlockPolicyEstimator in #22014): extract private lock-held helpers with trailing underscore naming (GetSetting_(), GetArgFlags_(), GetPathArg_()), then replace recursive calls in methods that already hold cs_args with those helpers.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
216 | @@ -217,29 +217,37 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters) 217 | const char *argv_test[] = {"-ignored", "-a", "-b", "-ccc=argument", "-ccc=multiple", "f", "-d=e"}; 218 | 219 | std::string error; 220 | - LOCK(testArgs.cs_args); 221 | testArgs.SetupArgs({a, b, ccc, d}); 222 | BOOST_CHECK(testArgs.ParseParameters(0, argv_test, error)); 223 | - BOOST_CHECK(testArgs.m_settings.command_line_options.empty() && testArgs.m_settings.ro_config.empty());
Seems these could all use LockSettings() instead of having to mess with the internal mutex?
With these changed, I believe you can pretty easily change almost all of the the protected: parts of ArgsManager (including cs_args) to private:, just leaving ReadConfigStream() and ReadConfigString() as protected, test-only, methods for testing the behaviour of ReadConfigFiles(). See https://github.com/ajtowns/bitcoin/commits/202603-argsman-private/
Done. Added a new commit fdc64114dd22d259503a6ac63846e9f4b803854b for this and you as co-author.
Nice. Concept ACK.
314 | path = "";
315 | return path;
316 | }
317 | } else {
318 | - path = GetDataDirBase();
319 | + path = GetDataDir(false);
GetDataDir(/*net_specific=*/false) seems like it would be a win here.
Done. Thanks.
ACK 2a7f53bb9149eb883c174681add60bf4649ca95a
Concept ACK.
ACK 2a7f53bb9149eb883c174681add60bf4649ca95a
ajtown's suggestions for making these methods private seems nice too.
434 | @@ -435,13 +435,18 @@ class ArgsManager 435 | void LogArgs() const; 436 | 437 | private: 438 | + // Lock-free internal helpers, for use by callers that already hold cs_args.
Done. Thanks.
285 | - if (IsArgNegated(arg)) return fs::path{}; 286 | - std::string path_str = GetArg(arg, ""); 287 | + AssertLockHeld(cs_args); 288 | + const auto value = GetSetting_(arg); 289 | + if (value.isFalse()) return {}; 290 | + std::string path_str = SettingToString(value).value_or("");
Done. Thanks.
459 | @@ -455,7 +460,7 @@ class ArgsManager
460 | void logArgsPrefix(
Done. Thanks.
321 | @@ -308,15 +322,18 @@ fs::path ArgsManager::GetBlocksDirPath() const 322 | return path; 323 | } 324 | 325 | +fs::path ArgsManager::GetDataDirBase() const { LOCK(cs_args); return GetDataDir(false); } 326 | +fs::path ArgsManager::GetDataDirNet() const { LOCK(cs_args); return GetDataDir(true); }
2c06582 args: extract lock-requiring internal helpers:
We don't usually add multiple commands on the same line, especially when it's a move + change.
But if we do, we could use WITH_LOCK instead - and I'd do this change in a separate commit:
fs::path ArgsManager::GetDataDirBase() const { return WITH_LOCK(cs_args, return GetDataDir(false)); }
fs::path ArgsManager::GetDataDirNet() const { return WITH_LOCK(cs_args, return GetDataDir(true)); }
Done. Thanks.
Not even a nit, but WITH_LOCK seems strictly worse here to me fwiw -- more typing (double return) and more internal complexity (an implicit lambda) for no benefit.
I'm fine with multi-line as well, just don't move methods in a non-trivial commit, and also change the methods and add multiple commands on the same line.
I see you've reverted it since. Can you please make it multiline to make it obvious that the method isn't just moved, but the implementation was also changed?
Done. It is multiline now. Thanks.
I have started reviewing it, let me push the comments I have so far.
571 | @@ -572,8 +572,8 @@ INSTANTIATE_INT_TYPE(uint64_t); 572 | bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strValue) 573 | { 574 | LOCK(cs_args); 575 | - if (IsArgSet(strArg)) return false; 576 | - ForceSetArg(strArg, strValue); 577 | + if (!GetSetting_(strArg).isNull()) return false; 578 | + m_settings.forced_settings[SettingName(strArg)] = strValue;
Yes, since ForceSetArg is just a one-liner (m_settings.forced_settings[SettingName(strArg)] = strValue). It's not worth creating a ForceSetArg_ for a single statement.
124 | @@ -125,7 +125,8 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) 125 | LOCK(cs_args); 126 | m_settings.ro_config.clear(); 127 | m_config_sections.clear(); 128 | - m_config_path = AbsPathForConfigVal(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false); 129 | + const auto conf_val = GetPathArg_("-conf", BITCOIN_CONF_FILENAME); 130 | + m_config_path = (conf_val.is_absolute() || conf_val.empty()) ? conf_val : fsbridge::AbsPathJoin(GetDataDir(false), conf_val);
e43b202 args: eliminate all recursive locking of cs_args:
would it be better to keep using a AbsPathForConfigVal_ abstraction here instead of reimplementing that logic inline?
If not, can we unify it with other DataDir calls?
m_config_path = (conf_val.is_absolute() || conf_val.empty()) ? conf_val : fsbridge::AbsPathJoin(GetDataDir(/*net_specific=*/false), conf_val);
Done. Thanks.
77 | @@ -78,7 +78,6 @@ struct TestArgsManager : public ArgsManager 78 | using ArgsManager::GetSettingsList; 79 | using ArgsManager::ReadConfigStream; 80 | using ArgsManager::cs_args; 81 | - using ArgsManager::m_network; 82 | using ArgsManager::m_settings;
bfc7468 test: scope cs_args locks to avoid recursive locking:
Seems like the other two are also unnecessary now
diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
--- a/src/test/argsman_tests.cpp (revision 92583fb1e50592bb924422c6b01caa06760e0a0e)
+++ b/src/test/argsman_tests.cpp (date 1772981835870)
@@ -77,8 +77,6 @@
using ArgsManager::GetSetting;
using ArgsManager::GetSettingsList;
using ArgsManager::ReadConfigStream;
- using ArgsManager::cs_args;
- using ArgsManager::m_settings;
};
//! Test GetSetting and GetArg type coercion, negation, and default value handling.
Done in 2800cca0e953a2a148df9b912eb50c693a3f692c. Thanks.
I don't see it in https://github.com/bitcoin/bitcoin/compare/fdc64114dd22d259503a6ac63846e9f4b803854b..2800cca0e953a2a148df9b912eb50c693a3f692c - though the final sources indicate it was removed. I don't see this in the range diffs either - weird...
Done. Thanks.
Done. Thanks.
Tested ACK fdc64114dd22d259503a6ac63846e9f4b803854b
Went through this series commit by commit and recreated the changes locally, comparing them with a few alternative approaches along the way. Rebased locally and ran all tests. I left some non-blocking comments and nits in the review, but overall LGTM. Happy to re-ACK if you decide to apply the suggestions.
ACK fdc64114dd22d259503a6ac63846e9f4b803854b. I've reviewed the code, and it looks OK.
Happy to re-ACK, once @ajtowns's suggestion is taken.
Happy to re-ACK, once @ajtowns's #34745 (review) is taken.
Done. Thanks.
Extract GetSetting_(), GetArgFlags_(), and GetPathArg_() as private
lock-held helpers with EXCLUSIVE_LOCKS_REQUIRED(cs_args) annotations.
Public methods delegate to these after acquiring the lock.
Annotate GetDataDir() with EXCLUSIVE_LOCKS_REQUIRED(cs_args) and move
its lock acquisition into GetDataDirBase()/GetDataDirNet().
Annotate logArgsPrefix() with EXCLUSIVE_LOCKS_REQUIRED(cs_args).
This is a pure refactoring with no behavior change, preparing for
the conversion of cs_args from RecursiveMutex to Mutex.
In methods that already hold cs_args, replace public method calls with
their lock-held private counterparts:
- ParseParameters: GetArgFlags() -> GetArgFlags_()
- GetBlocksDirPath: IsArgSet()/GetPathArg()/GetDataDirBase() ->
GetSetting_()/GetPathArg_()/GetDataDir()
- ReadSettingsFile: GetArgFlags() -> GetArgFlags_()
- SoftSetArg: IsArgSet()/ForceSetArg() -> GetSetting_()/direct write
- CheckMultipleCLIArgs: IsArgSet() -> GetSetting_()
- logArgsPrefix: GetArgFlags() -> GetArgFlags_()
- ReadConfigStream: GetArgFlags() -> GetArgFlags_()
- ReadConfigFiles: GetPathArg() -> GetPathArg_()
No behavior change. This eliminates all recursive lock acquisitions,
preparing for the conversion of cs_args from RecursiveMutex to Mutex.
Restructure argsman tests to use scoped LOCK(cs_args) blocks only
around direct protected member access (m_settings, m_network), keeping
public method calls outside lock scopes. This avoids recursive lock
acquisitions that would deadlock with a non-recursive Mutex.
- util_ParseParameters: scope locks around m_settings checks
- util_GetBoolArg: scope lock around m_settings size check
- util_ReadConfigStream: scope lock around m_settings checks
- util_GetArg: scope lock around m_settings writes
- util_ArgsMerge: use SelectConfigNetwork() instead of m_network
- util_ChainMerge: remove unnecessary lock
Replace the RecursiveMutex with a plain Mutex now that all recursive
lock acquisitions have been eliminated in the preceding commits.
Add EXCLUSIVE_LOCKS_REQUIRED(!cs_args) negative capability annotations
to all public and protected methods that acquire cs_args, following the
pattern established in prior RecursiveMutex conversions (e.g. CAddrMan,
CBlockPolicyEstimator).
Move the first `protected` block (struct Arg, cs_args, m_settings, and
all other member variables) to `private`. Only `ReadConfigStream` and
`ReadConfigString` remain `protected` for test access.
Changes:
- Move `ReadConfigString` from `TestArgsManager` into `ArgsManager`
itself (declared in args.h, defined in config.cpp) so tests no longer
need direct access to `cs_args` or `m_settings` for config parsing.
- Replace test-only `SetNetworkOnlyArg` helper with the existing
`NETWORK_ONLY` flag passed through `SetupArgs`/`AddArg`.
- Remove `TestArgsManager` constructor that cleared
`m_network_only_args`.
- Remove `using` declarations for `cs_args`, `m_settings`, `GetSetting`,
and `GetSettingsList` from `TestArgsManager`.
- Clear `m_config_sections` in `ClearArgs()`.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
ACK 20fb7618b001937befbf8757e5881674b4a55147
Adjusted the remaining requests since last review
Re-ACK 20fb7618b001937befbf8757e5881674b4a55147
Concept ACK https://github.com/bitcoin/bitcoin/commit/20fb7618b001937befbf8757e5881674b4a55147
I will get to reviewing it soon. Edit: If it's not merged by then.