http: Join worker threads before deleting work queue #12366

laanwj commented at 6:32 PM on February 6, 2018: member

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.

laanwj added the label RPC/REST/ZMQ on Feb 6, 2018

laanwj added the label Bug on Feb 6, 2018

laanwj added this to the milestone 0.16.0 on Feb 6, 2018

in src/httpserver.cpp:452 in 2fefa3077e outdated

 448 | @@ -449,6 +449,7 @@ bool UpdateHTTPServerLogging(bool enable) {
 449 |  
 450 |  std::thread threadHTTP;
 451 |  std::future<bool> threadResult;
 452 | +static std::vector<std::thread> threadHTTPWorkers;

ryanofsky commented at 6:44 PM on February 6, 2018:

Could follow g_thread_http_workers naming convention.

laanwj commented at 6:50 PM on February 6, 2018:

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

ryanofsky commented at 6:47 PM on February 6, 2018: member

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

in src/httpserver.cpp:464 in 2fefa3077e outdated

 460 | @@ -460,8 +461,7 @@ bool StartHTTPServer()
 461 |      threadHTTP = std::thread(std::move(task), eventBase, eventHTTP);
 462 |  
 463 |      for (int i = 0; i < rpcThreads; i++) {
 464 | -        std::thread rpc_worker(HTTPWorkQueueRun, workQueue);
 465 | -        rpc_worker.detach();
 466 | +        threadHTTPWorkers.emplace_back(std::thread(HTTPWorkQueueRun, workQueue));

TheBlueMatt commented at 7:05 PM on February 6, 2018:

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

laanwj commented at 7:09 PM on February 6, 2018:

Right

TheBlueMatt commented at 7:06 PM on February 6, 2018: member

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.

http: Join worker threads before deleting work queue

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>

b1c2370dde

http: Remove WaitExit from WorkQueue

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>

f94665466e

in src/httpserver.cpp:489 in 2fefa3077e outdated

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

laanwj commented at 7:14 PM on February 6, 2018:

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

TheBlueMatt commented at 7:19 PM on February 6, 2018:

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

laanwj commented at 7:30 PM on February 6, 2018:

Ok, will remove it in a separate commit.

laanwj force-pushed on Feb 6, 2018

laanwj commented at 7:35 PM on February 6, 2018: member

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

jnewbery commented at 7:36 PM on February 6, 2018: member

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

laanwj added the label Needs backport on Feb 6, 2018

in src/httpserver.cpp:148 in f94665466e outdated

 139 | @@ -141,13 +140,6 @@ class WorkQueue
 140 |          running = false;
 141 |          cond.notify_all();
 142 |      }
 143 | -    /** Wait for worker threads to exit */
 144 | -    void WaitExit()
 145 | -    {
 146 | -        std::unique_lock<std::mutex> lock(cs);
 147 | -        while (numThreads > 0)

ryanofsky commented at 7:51 PM on February 6, 2018:

In commit "http: Remove WaitExit from WorkQueue"

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

laanwj commented at 8:55 AM on February 7, 2018:

Thanks, I removed those in a new commit.

ryanofsky commented at 7:53 PM on February 6, 2018: member

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

in src/httpserver.cpp:482 in f94665466e outdated

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

promag commented at 10:47 PM on February 6, 2018:

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

TheBlueMatt commented at 10:52 PM on February 6, 2018:

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

promag commented at 10:48 PM on February 6, 2018: member

utACK.

sipa commented at 3:22 AM on February 7, 2018: member

utACK f94665466ed50e868c98b1a1c708ad5767727bb6. This is easier to read as well.

dcousens approved

dcousens commented at 3:46 AM on February 7, 2018: contributor

utACK

http: Remove numThreads and ThreadCounter

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>

11e01515fe

dcousens approved

in src/httpserver.cpp:79 in 11e01515fe

  95 |  
  96 |  public:
  97 |      explicit WorkQueue(size_t _maxDepth) : running(true),
  98 | -                                 maxDepth(_maxDepth),
  99 | -                                 numThreads(0)
 100 | +                                 maxDepth(_maxDepth)

promag commented at 9:03 PM on February 7, 2018:

Nit, dunno about correct indentation here..

in src/httpserver.cpp:460 in 11e01515fe

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

promag commented at 9:06 PM on February 7, 2018:

Nit, space before :.

in src/httpserver.cpp:422 in 11e01515fe

 419 | @@ -449,6 +420,7 @@ bool UpdateHTTPServerLogging(bool enable) {
 420 |  
 421 |  std::thread threadHTTP;
 422 |  std::future<bool> threadResult;

promag commented at 9:08 PM on February 7, 2018:

Nit, this and the above could be static too.

laanwj commented at 7:39 AM on February 8, 2018:

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

promag commented at 9:12 PM on February 7, 2018: member

utACK 11e0151.

meshcollider commented at 9:52 PM on February 7, 2018: contributor

Nice cleanup! utACK https://github.com/bitcoin/bitcoin/pull/12366/commits/11e01515fe0fbc7823d4111ad6e016a02c485a78

laanwj commented at 7:55 AM on February 8, 2018: member

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.

laanwj merged this on Feb 8, 2018

laanwj closed this on Feb 8, 2018

laanwj referenced this in commit 6db4fa7ad3 on Feb 8, 2018

laanwj referenced this in commit dd346cb262 on Feb 8, 2018

laanwj referenced this in commit 93de37a12b on Feb 8, 2018

laanwj referenced this in commit 4cd7edba47 on Feb 8, 2018

laanwj removed the label Needs backport on Feb 8, 2018

HashUnlimited referenced this in commit 6219f45d29 on Mar 16, 2018

HashUnlimited referenced this in commit df0442c703 on Mar 16, 2018

HashUnlimited referenced this in commit c0696a5fc2 on Mar 16, 2018

ccebrecos referenced this in commit 05abb648f1 on Sep 14, 2018

ccebrecos referenced this in commit b5f762d0c4 on Sep 14, 2018

codablock referenced this in commit ebf671aa76 on Oct 14, 2019

codablock referenced this in commit 00cbfac3c8 on Oct 14, 2019

barrystyle referenced this in commit f2fe2b3ecb on Jan 22, 2020

random-zebra referenced this in commit ac523660b4 on Feb 21, 2021

LarryRuane referenced this in commit 380c84613a on Mar 4, 2021

LarryRuane referenced this in commit 7580007e97 on Mar 4, 2021

LarryRuane referenced this in commit 1d32812ce1 on Mar 4, 2021

LarryRuane referenced this in commit 0dcbfc9147 on Apr 13, 2021

LarryRuane referenced this in commit c733ded07d on Apr 13, 2021

LarryRuane referenced this in commit cbd11135ff on Apr 13, 2021

zkbot referenced this in commit 0e36226271 on Apr 17, 2021

DrahtBot locked this on Sep 8, 2021