http: Join worker threads before deleting work queue #12366

laanwj · 2018-02-06T18:32:19Z

This prevents a potential race condition if control flow ends up in
ShutdownHTTPServer before the thread gets to queue->Run(),
deleting the work queue while workers are still going to use it.

Meant to fix #12362.

ryanofsky

utACK 2fefa3077e69dfdbccbd010cda5627787ec92bb8. Untested, but change makes sense. Detached threads are always a little suspicious.

ryanofsky · 2018-02-06T18:44:14Z

src/httpserver.cpp

@@ -449,6 +449,7 @@ bool UpdateHTTPServerLogging(bool enable) {

 std::thread threadHTTP;
 std::future<bool> threadResult;
+static std::vector<std::thread> threadHTTPWorkers;


Could follow g_thread_http_workers naming convention.

Can we please first make sure that this fixes the issue before jumping on naming issues

TheBlueMatt

Tested ACK 2fefa3077e69dfdbccbd010cda5627787ec92bb8 - my previous sleeps and valgrind are no longer able to find any issue here.

Would love to see @jnewbery confirm he cannot reproduce anymore as well.

TheBlueMatt · 2018-02-06T19:05:18Z

src/httpserver.cpp

@@ -460,8 +461,7 @@ bool StartHTTPServer()
    threadHTTP = std::thread(std::move(task), eventBase, eventHTTP);

    for (int i = 0; i < rpcThreads; i++) {
-        std::thread rpc_worker(HTTPWorkQueueRun, workQueue);
-        rpc_worker.detach();
+        threadHTTPWorkers.emplace_back(std::thread(HTTPWorkQueueRun, workQueue));


No need to create the temporary just to move it into a new std::thread constructor.

This prevents a potential race condition if control flow ends up in `ShutdownHTTPServer` before the thread gets to `queue->Run()`, deleting the work queue while workers are still going to use it. Meant to fix bitcoin#12362. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com>

laanwj · 2018-02-06T19:14:04Z

src/httpserver.cpp

@@ -487,6 +487,10 @@ void StopHTTPServer()
    if (workQueue) {
        LogPrint(BCLog::HTTP, "Waiting for HTTP worker threads to exit\n");
        workQueue->WaitExit();


I think we don't need WaitExit() anymore with this?

It would seem not, indeed. Testing shows no difference with it removed.

Ok, will remove it in a separate commit.

This function, which waits for all threads to exit, is no longer needed now that threads are joined instead. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com>

laanwj · 2018-02-06T19:35:44Z

ok, renamed the variable and added a commit to remove WaitExit()

jnewbery · 2018-02-06T19:36:55Z

I can confirm that I'm not able to reproduce the error when running with commit f946654.

ryanofsky

utACK f946654. Only changes were variable rename and temp std::thread / WaitExit remove suggestions above.

ryanofsky · 2018-02-06T19:51:09Z

src/httpserver.cpp

-    void WaitExit()
-    {
-        std::unique_lock<std::mutex> lock(cs);
-        while (numThreads > 0)


In commit "http: Remove WaitExit from WorkQueue"

It looks like the numThreads variable is unused now, so it might be worth removing as well.

Thanks, I removed those in a new commit.

promag

utACK.

promag · 2018-02-06T22:47:54Z

src/httpserver.cpp

@@ -486,7 +478,10 @@ void StopHTTPServer()
    LogPrint(BCLog::HTTP, "Stopping HTTP server\n");
    if (workQueue) {
        LogPrint(BCLog::HTTP, "Waiting for HTTP worker threads to exit\n");
-        workQueue->WaitExit();
+        for (auto& thread: g_thread_http_workers) {
+            thread.join();


From the documentation, I don't think this is needed, clear() below is enough.

No, if you delete a thread without either calling detach() or join(), terminate() is called. join() is not automatically called for you.

sipa · 2018-02-07T03:22:54Z

utACK f946654. This is easier to read as well.

dcousens

utACK

The HTTP worker thread counter, as well as the RAII object that was used to maintain it, is unused now, so can be removed. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com>

promag

utACK 11e0151.

promag · 2018-02-07T21:03:34Z

src/httpserver.cpp


 public:
    explicit WorkQueue(size_t _maxDepth) : running(true),
-                                 maxDepth(_maxDepth),
-                                 numThreads(0)
+                                 maxDepth(_maxDepth)


Nit, dunno about correct indentation here..

promag · 2018-02-07T21:06:09Z

src/httpserver.cpp

@@ -486,7 +457,10 @@ void StopHTTPServer()
    LogPrint(BCLog::HTTP, "Stopping HTTP server\n");
    if (workQueue) {
        LogPrint(BCLog::HTTP, "Waiting for HTTP worker threads to exit\n");
-        workQueue->WaitExit();
+        for (auto& thread: g_thread_http_workers) {


Nit, space before :.

promag · 2018-02-07T21:08:53Z

src/httpserver.cpp

@@ -449,6 +420,7 @@ bool UpdateHTTPServerLogging(bool enable) {

 std::thread threadHTTP;
 std::future<bool> threadResult;


Nit, this and the above could be static too.

Yes, but out of scope of this PR to change that.

meshcollider · 2018-02-07T21:52:28Z

Nice cleanup!
utACK 11e0151

laanwj · 2018-02-08T07:55:58Z

Nice cleanup!

FWIW I wrote HTTPWorkQueue with the idea that it could likely be replaced with a 1-liner after c++11, I think there's scope for more code removal. Out of scope for a 0.16.0 fix ofc.

11e0151 http: Remove numThreads and ThreadCounter (Wladimir J. van der Laan) f946654 http: Remove WaitExit from WorkQueue (Wladimir J. van der Laan) b1c2370 http: Join worker threads before deleting work queue (Wladimir J. van der Laan) Pull request description: This prevents a potential race condition if control flow ends up in `ShutdownHTTPServer` before the thread gets to `queue->Run()`, deleting the work queue while workers are still going to use it. Meant to fix #12362. Tree-SHA512: 8108514aeee5b2067a3736ed028014b580d1cbf8530ac7682b8a23070133dfa1ca21db4358c9158ea57e8811e0551395b6cb769887876b9cfce067ee968d0642

This prevents a potential race condition if control flow ends up in `ShutdownHTTPServer` before the thread gets to `queue->Run()`, deleting the work queue while workers are still going to use it. Meant to fix #12362. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com> Github-Pull: #12366 Rebased-From: b1c2370 Tree-SHA512: 7fbe8e562ba0e1138f95deb42e84cda734bef0d7058349728b3e1602b9f65777c5b1be0f6bd5a7e513ac38487dc82e62fd0a6ee4ed985cbf36c90a7eec18eced

This function, which waits for all threads to exit, is no longer needed now that threads are joined instead. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com> Github-Pull: #12366 Rebased-From: f946654 Tree-SHA512: 5cda90b4c081424d637031c0bbf168f177667733ff20b6f77eac84e503f7fad6fab3eb897f191edd819f18b270e3ecea78974978abd102d323f42d9d06216e53

The HTTP worker thread counter, as well as the RAII object that was used to maintain it, is unused now, so can be removed. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com> Github-Pull: #12366 Rebased-From: 11e0151 Tree-SHA512: 87055c4c14986973f4c1604db264fb5a9de21bb481e9d39b201774e2d17ed92a7d1617449471c13f56e0f1f09a8aebdf1254a71d6c7b856c880a5b71e0c3ba9d

This prevents a potential race condition if control flow ends up in `ShutdownHTTPServer` before the thread gets to `queue->Run()`, deleting the work queue while workers are still going to use it. Meant to fix bitcoin#12362. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com> Github-Pull: bitcoin#12366 Rebased-From: b1c2370 Tree-SHA512: 7fbe8e562ba0e1138f95deb42e84cda734bef0d7058349728b3e1602b9f65777c5b1be0f6bd5a7e513ac38487dc82e62fd0a6ee4ed985cbf36c90a7eec18eced

This function, which waits for all threads to exit, is no longer needed now that threads are joined instead. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com> Github-Pull: bitcoin#12366 Rebased-From: f946654 Tree-SHA512: 5cda90b4c081424d637031c0bbf168f177667733ff20b6f77eac84e503f7fad6fab3eb897f191edd819f18b270e3ecea78974978abd102d323f42d9d06216e53

The HTTP worker thread counter, as well as the RAII object that was used to maintain it, is unused now, so can be removed. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com> Github-Pull: bitcoin#12366 Rebased-From: 11e0151 Tree-SHA512: 87055c4c14986973f4c1604db264fb5a9de21bb481e9d39b201774e2d17ed92a7d1617449471c13f56e0f1f09a8aebdf1254a71d6c7b856c880a5b71e0c3ba9d Conflicts: src/httpserver.cpp

…ueue 11e0151 http: Remove numThreads and ThreadCounter (Wladimir J. van der Laan) f946654 http: Remove WaitExit from WorkQueue (Wladimir J. van der Laan) b1c2370 http: Join worker threads before deleting work queue (Wladimir J. van der Laan) Pull request description: This prevents a potential race condition if control flow ends up in `ShutdownHTTPServer` before the thread gets to `queue->Run()`, deleting the work queue while workers are still going to use it. Meant to fix bitcoin#12362. Tree-SHA512: 8108514aeee5b2067a3736ed028014b580d1cbf8530ac7682b8a23070133dfa1ca21db4358c9158ea57e8811e0551395b6cb769887876b9cfce067ee968d0642

aab15d7 ReplayBlocks: use find instead of brackets operator to access to the element. (furszy) e898353 [Refactoring] Use const CBlockIndex* where appropriate (random-zebra) c76fa04 qa: Extract rpc_timewait as test param (furszy) 0f832e3 shutdown: Stop threads before resetting ptrs (MarcoFalke) 67aebbf http: Remove numThreads and ThreadCounter (Wladimir J. van der Laan) e24c710 http: Remove WaitExit from WorkQueue (Wladimir J. van der Laan) b8f7364 http: Join worker threads before deleting work queue (Wladimir J. van der Laan) 7d68769 rpc: further constrain the libevent workaround (Cory Fields) 75af065 rpc: work-around an upstream libevent bug (Cory Fields) 50e5833 Always return true if AppInitMain got to the end (Matt Corallo) bd70dcc [qa] Test non-atomic chainstate writes (furszy) 8f04970 Dont create pcoinsTip until after ReplayBlocks. (Matt Corallo) 93f2b15 Random db flush crash simulator (Pieter Wuille) 72f3b17 Adapt memory usage estimation for flushing (Pieter Wuille) 8540113 Non-atomic flushing using the blockchain as replay journal (Pieter Wuille) 8d6625f [MOVEONLY] Move LastCommonAncestor to chain (Pieter Wuille) Pull request description: > This patch adds an extra "head blocks" to the chainstate, which gives the range of blocks for writes may be incomplete. At the start of a flush, we write this record, write the dirty dbcache entries in 16 MiB batches, and at the end we remove the heads record again. If it is present at startup it means we crashed during flush, and we rollback/roll forward blocks inside of it to get a consistent tip on disk before proceeding. > If a flush completes succesfully, the resulting database is compatible with previous versions. If the node crashes in the middle of a flush, a version of the code with this patch is needed to recovery. An adaptation of the following PRs with further modifications to the `feature_dbcrash.py` test to be up-to-date with upstream and solve RPC related bugs. * bitcoin#10148. * Increase RPC wait time. * bitcoin#11831 * bitcoin#11593 * bitcoin#12366 * bitcoin#13837 * bitcoin#13894 ACKs for top commit: random-zebra: ACK aab15d7 Fuzzbawls: ACK aab15d7 Tree-SHA512: 898806746f581a9eb8deb0155c558481abf4454c6f3b3c3ad505c557938d0700fe9796e98e36492286ae869378647072c3ad77ad65e9dd7075108ff96469ade1

This prevents a potential race condition if control flow ends up in `ShutdownHTTPServer` before the thread gets to `queue->Run()`, deleting the work queue while workers are still going to use it. Meant to fix #12362. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com> zcash: cherry picked from commit b1c2370 zcash: bitcoin/bitcoin#12366

This function, which waits for all threads to exit, is no longer needed now that threads are joined instead. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com> zcash: cherry picked from commit f946654 zcash: bitcoin/bitcoin#12366

The HTTP worker thread counter, as well as the RAII object that was used to maintain it, is unused now, so can be removed. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com> zcash: cherry picked from commit 11e0151 zcash: bitcoin/bitcoin#12366

Bitcoin 0.16 locking PRs These are locking changes from upstream (bitcoin core) release 0.16 (Aug 14, 2017, to Feb 16, 2018), oldest to newest (when merged to the master branch). Each commit also includes a reference both to the PR and the upstream commit. - bitcoin/bitcoin#11126 - Excludes changes to wallet tests that we don't have. - bitcoin/bitcoin#10916 - first commit only; second commit already merged by d9fcc2b - bitcoin/bitcoin#11107 - Only the last commit. - bitcoin/bitcoin#11593 - bitcoin/bitcoin#11585 - bitcoin/bitcoin#11618 - bitcoin/bitcoin#10286 - Only the third and last commits. - bitcoin/bitcoin#11870 - bitcoin/bitcoin#12330 - bitcoin/bitcoin#12366 - bitcoin/bitcoin#12368 - bitcoin/bitcoin#12333 - Only the first commit.

laanwj added RPC/REST/ZMQ Bug labels Feb 6, 2018

laanwj added this to the 0.16.0 milestone Feb 6, 2018

ryanofsky reviewed Feb 6, 2018

View reviewed changes

TheBlueMatt reviewed Feb 6, 2018

View reviewed changes

laanwj commented Feb 6, 2018

View reviewed changes

http: Remove WaitExit from WorkQueue

f946654

This function, which waits for all threads to exit, is no longer needed now that threads are joined instead. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com>

laanwj force-pushed the 2017_02_httpserver_join branch from 2fefa30 to f946654 Compare February 6, 2018 19:34

laanwj added the Needs backport label Feb 6, 2018

ryanofsky reviewed Feb 6, 2018

View reviewed changes

promag reviewed Feb 6, 2018

View reviewed changes

dcousens approved these changes Feb 7, 2018

View reviewed changes

http: Remove numThreads and ThreadCounter

11e0151

The HTTP worker thread counter, as well as the RAII object that was used to maintain it, is unused now, so can be removed. Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com>

dcousens approved these changes Feb 7, 2018

View reviewed changes

promag reviewed Feb 7, 2018

View reviewed changes

laanwj merged commit 11e0151 into bitcoin:master Feb 8, 2018

laanwj removed the Needs backport label Feb 8, 2018

ryanofsky mentioned this pull request Feb 8, 2018

Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection #11640

Merged

dagurval mentioned this pull request Jun 14, 2018

Fix dbcrash failures bitcoinxt/bitcoinxt#456

Merged

sickpig mentioned this pull request Jun 15, 2018

[PORT] RPC HTTP server fixes BitcoinUnlimited/BitcoinUnlimited#1138

Merged

furszy mentioned this pull request Feb 16, 2021

Use non-atomic flushing with block replay PIVX-Project/PIVX#2207

Merged

LarryRuane mentioned this pull request Mar 4, 2021

Bitcoin 0.16 locking PRs zcash/zcash#5017

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -449,6 +420,7 @@ bool UpdateHTTPServerLogging(bool enable) {

		std::thread threadHTTP;
		std::future<bool> threadResult;

http: Join worker threads before deleting work queue #12366

http: Join worker threads before deleting work queue #12366

Uh oh!

Conversation

laanwj commented Feb 6, 2018

Uh oh!

ryanofsky 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!

TheBlueMatt 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!

laanwj commented Feb 6, 2018

Uh oh!

jnewbery commented Feb 6, 2018

Uh oh!

ryanofsky 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!

promag 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!

sipa commented Feb 7, 2018

Uh oh!

dcousens left a comment

Choose a reason for hiding this comment

Uh oh!

promag 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!

meshcollider commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Feb 8, 2018

Uh oh!

Uh oh!

meshcollider commented Feb 7, 2018 •

edited

Loading