Skip to content

Commit

Permalink
scripted-diff: Replace MilliSleep with UninterruptibleSleep
Browse files Browse the repository at this point in the history
This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.

* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main thread)

-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g' $(git grep -l MilliSleep)
-END VERIFY SCRIPT-

Cherry-picked from: fa9af06
  • Loading branch information
MarcoFalke authored and xanimo committed Mar 30, 2024
1 parent 1d65ba3 commit fa541d1
Show file tree
Hide file tree
Showing 12 changed files with 87 additions and 14 deletions.
2 changes: 1 addition & 1 deletion src/bench/Examples.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
static void Sleep100ms(benchmark::State& state)
{
while (state.KeepRunning()) {
MilliSleep(100);
UninterruptibleSleep(std::chrono::milliseconds{100});
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/bitcoin-cli.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ int CommandLineRPC(int argc, char *argv[])
}
catch (const CConnectionFailed&) {
if (fWait)
MilliSleep(1000);
UninterruptibleSleep(std::chrono::milliseconds{1000});
else
throw;
}
Expand Down
2 changes: 1 addition & 1 deletion src/bitcoind.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ void WaitForShutdown(boost::thread_group* threadGroup)
// Tell the main threads to shutdown.
while (!fShutdown)
{
MilliSleep(200);
UninterruptibleSleep(std::chrono::milliseconds{200});
fShutdown = ShutdownRequested();
}
if (threadGroup)
Expand Down
2 changes: 1 addition & 1 deletion src/httprpc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
/* Deter brute-forcing
If this results in a DoS the user really
shouldn't have their RPC port exposed. */
MilliSleep(250);
UninterruptibleSleep(std::chrono::milliseconds{250});

req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA);
req->WriteReply(HTTP_UNAUTHORIZED);
Expand Down
3 changes: 3 additions & 0 deletions src/rpc/server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,9 @@ UniValue stop(const JSONRPCRequest& jsonRequest)
// Event loop will exit after current HTTP requests have been handled, so
// this reply will get back to the client.
StartShutdown();
if (jsonRequest.params[0].isNum()) {
UninterruptibleSleep(std::chrono::milliseconds{jsonRequest.params[0].get_int()});
}
return "Dogecoin server stopping";
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/checkqueue_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueueControl_Locks)
CCheckQueueControl<FakeCheck> control(queue.get());
// While sleeping, no other thread should execute to this point
auto observed = ++nThreads;
MilliSleep(10);
UninterruptibleSleep(std::chrono::milliseconds{10});
fails += observed != nThreads;
});
}
Expand Down
24 changes: 24 additions & 0 deletions src/test/util_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "primitives/transaction.h"
#include "sync.h"
#include "utilstrencodings.h"
#include "utiltime.h"
#include "utilmoneystr.h"
#include "test/test_bitcoin.h"

Expand All @@ -16,6 +17,8 @@

#include <boost/test/unit_test.hpp>

#include <chrono>

extern std::map<std::string, std::string> mapArgs;

BOOST_FIXTURE_TEST_SUITE(util_tests, BasicTestingSetup)
Expand Down Expand Up @@ -311,6 +314,27 @@ BOOST_AUTO_TEST_CASE(gettime)
BOOST_CHECK((GetTime() & ~0xFFFFFFFFLL) == 0);
}

BOOST_AUTO_TEST_CASE(util_time_GetTime)
{
SetMockTime(111);
// Check that mock time does not change after a sleep
for (const auto& num_sleep : {0, 1}) {
UninterruptibleSleep(std::chrono::milliseconds{num_sleep});
BOOST_CHECK_EQUAL(111, GetTime()); // Deprecated time getter
BOOST_CHECK_EQUAL(111, GetTime<std::chrono::seconds>().count());
BOOST_CHECK_EQUAL(111000, GetTime<std::chrono::milliseconds>().count());
BOOST_CHECK_EQUAL(111000000, GetTime<std::chrono::microseconds>().count());
}

SetMockTime(0);
// Check that system time changes after a sleep
const auto ms_0 = GetTime<std::chrono::milliseconds>().count();
const auto us_0 = GetTime<std::chrono::microseconds>().count();
UninterruptibleSleep(std::chrono::milliseconds{1});
BOOST_CHECK(ms_0 < GetTime<std::chrono::milliseconds>().count());
BOOST_CHECK(us_0 < GetTime<std::chrono::microseconds>().count());
}

BOOST_AUTO_TEST_CASE(test_IsDigit)
{
BOOST_CHECK_EQUAL(IsDigit('0'), true);
Expand Down
15 changes: 15 additions & 0 deletions src/utiltime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <boost/date_time/posix_time/posix_time.hpp>
#include <boost/thread.hpp>
#include <ctime>
#include <chrono>
#include <thread>

using namespace std;
Expand All @@ -32,6 +33,20 @@ int64_t GetTime()
return now;
}

template <typename T>
T GetTime()
{
const std::chrono::seconds mocktime{nMockTime.load(std::memory_order_relaxed)};

return std::chrono::duration_cast<T>(
mocktime.count() ?
mocktime :
std::chrono::microseconds{GetTimeMicros()});
}
template std::chrono::seconds GetTime();
template std::chrono::milliseconds GetTime();
template std::chrono::microseconds GetTime();

int64_t GetMockableTimeMicros()
{
int64_t mocktime = GetMockTime();
Expand Down
43 changes: 37 additions & 6 deletions src/utiltime.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,34 @@

void UninterruptibleSleep(const std::chrono::microseconds& n);

/**
* Helper to count the seconds of a duration.
*
* All durations should be using std::chrono and calling this should generally be avoided in code. Though, it is still
* preferred to an inline t.count() to protect against a reliance on the exact type of t.
*/
inline int64_t count_seconds(std::chrono::seconds t) { return t.count(); }

/**
* DEPRECATED
* Use either GetSystemTimeInSeconds (not mockable) or GetTime<T> (mockable)
*/
int64_t GetTime();

/** Returns the system time (not mockable) */
int64_t GetTimeMillis();
/** Returns the system time (not mockable) */
int64_t GetTimeMicros();
/** Returns the system time (not mockable) */
int64_t GetSystemTimeInSeconds(); // Like GetTime(), but not mockable

/** For testing. Set e.g. with the setmocktime rpc, or -mocktime argument */
void SetMockTime(int64_t nMockTimeIn);
/** For testing */
int64_t GetMockTime();

void MilliSleep(int64_t n);

/**
* GetTimeMicros() and GetTimeMillis() both return the system time, but in
* different units. GetTime() returns the system time in seconds, but also
Expand All @@ -22,17 +50,20 @@ void UninterruptibleSleep(const std::chrono::microseconds& n);
* TODO: Rework these functions to be type-safe (so that we don't inadvertently
* compare numbers with different units, or compare a mocktime to system time).
*/

int64_t GetTime();
int64_t GetTimeMillis();
int64_t GetTimeMicros();
int64_t GetSystemTimeInSeconds(); // Like GetTime(), but not mockable
int64_t GetLogTimeMicros();
int64_t GetMockableTimeMicros();
int64_t GetMockTime();

/** For testing. Set e.g. with the setmocktime rpc, or -mocktime argument */
void SetMockTime(int64_t nMockTimeIn);
/** For testing */
int64_t GetMockTime();

void MilliSleep(int64_t n);

/** Return system time (or mocked time, if set) */
template <typename T>
T GetTime();

std::string DateTimeStrFormat(const char* pszFormat, int64_t nTime);

#endif // BITCOIN_UTILTIME_H
2 changes: 1 addition & 1 deletion src/wallet/db.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ bool CDB::Rewrite(const string& strFile, const char* pszSkip)
return fSuccess;
}
}
MilliSleep(100);
UninterruptibleSleep(std::chrono::milliseconds{100});
}
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4032,7 +4032,7 @@ bool CWallet::BackupWallet(const std::string& strDest)
}
}
}
MilliSleep(100);
UninterruptibleSleep(std::chrono::milliseconds{100});
}
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/walletdb.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,7 @@ void ThreadFlushWalletDB()
int64_t nLastWalletUpdate = GetTime();
while (true)
{
MilliSleep(500);
UninterruptibleSleep(std::chrono::milliseconds{500});

if (nLastSeen != CWalletDB::GetUpdateCounter())
{
Expand Down

0 comments on commit fa541d1

Please sign in to comment.