Merge #2867: [Core][Tests] Cleanup HTTPServer start and stop

9d42360 Cleanup HTTPServer start and stop (Liquid)

Pull request description:

  Lately, we have been spending a lot of time messing with our test cases. We are hitting random errors at various points and it is causing us to waste time constantly fixing and re-running tests in order to be 'ready for review'. After discussion with the other developers, I went and looked into the code and found some tiny improvements that can assist us in not dealing with these cases anymore.

  Threads are still considered active, even if the code is finished executing before being joined, and we do not want to try to join them if the code is not finished executing. So we are now checking if the threads can be closed and we are doing a little bit better cleanup of 0 > nullptr. Lastly, we want to make sure any queue is empty before closing.

  To test:
  The best route is to fork this branch and base it over your master or any branch you can run GitHub actions on, and run the tests several times, as well as running the test-runner.py on your local PC.

  I have run over 50+ iterations of the tests over the weekend and have not encountered an issue in shutdown since.
  We still have some issues in `tier_two` tests that may appear, but evaluate the logs for whether or not you are incurring an issue from the `stop_node()` function.

ACKs for top commit:
  Fuzzbawls:
    ACK 9d42360
  panleone:
    tACK 9d42360

Tree-SHA512: e223ddcb5bdb0946560de56014b1a2d15cd8a4cc21ef917315c53e0df2b6c8218ade4ce2f22868ae7c06eec1b2dfaf706d5161bf6089b3895ff39f0b57f94245

src/httpserver.cpp

@@ -94,7 +94,7 @@ class WorkQueue
  bool Enqueue(WorkItem* item)
  {
  std::unique_lock<std::mutex> lock(cs);
- if (queue.size() >= maxDepth) {
+ if (!running || queue.size() >= maxDepth) {
  return false;
  }
  queue.push_back(item);
@@ -110,7 +110,7 @@ class WorkQueue
  std::unique_lock<std::mutex> lock(cs);
  while (running && queue.empty())
  cond.wait(lock);
- if (!running)
+ if (!running && queue.empty())
  break;
  i = queue.front();
  queue.pop_front();
@@ -434,6 +434,7 @@ bool StartHTTPServer()
  LogPrint(BCLog::HTTP, "Starting HTTP server\n");
  int rpcThreads = std::max((long)gArgs.GetArg("-rpcthreads", DEFAULT_HTTP_THREADS), 1L);
  LogPrintf("HTTP: starting %d worker threads\n", rpcThreads);
+
  std::packaged_task<bool(event_base*, evhttp*)> task(ThreadHTTP);
  threadResult = task.get_future();
  threadHTTP = std::thread(std::move(task), eventBase, eventHTTP);
@@ -460,15 +461,20 @@ void InterruptHTTPServer()
 void StopHTTPServer()
 {
  LogPrint(BCLog::HTTP, "Stopping HTTP server\n");
+
  if (workQueue) {
  LogPrint(BCLog::HTTP, "Waiting for HTTP worker threads to exit\n");
- for (auto& thread: g_thread_http_workers) {
- thread.join();
+ for (auto& thread : g_thread_http_workers) {
+ // Guard threadHTTP
+ if (thread.joinable()) {
+ thread.join();
+ }
  }
  g_thread_http_workers.clear();
- delete workQueue;
  }
+
  MilliSleep(500); // Avoid race condition while the last HTTP-thread is exiting
+
  if (eventBase) {
  LogPrint(BCLog::HTTP, "Waiting for HTTP event thread to exit\n");
  // Give event loop a few seconds to exit (to send back last RPC responses), then break it
@@ -480,17 +486,24 @@ void StopHTTPServer()
  if (threadResult.valid() && threadResult.wait_for(std::chrono::milliseconds(2000)) == std::future_status::timeout) {
  LogPrintf("HTTP event loop did not exit within allotted time, sending loopbreak\n");
  event_base_loopbreak(eventBase);
-
  }
- threadHTTP.join();
+ // Guard threadHTTP
+ if (threadHTTP.joinable()) {
+ threadHTTP.join();
+ }
  }
+
  if (eventHTTP) {
  evhttp_free(eventHTTP);
- eventHTTP = 0;
+ eventHTTP = nullptr;
  }
  if (eventBase) {
  event_base_free(eventBase);
- eventBase = 0;
+ eventBase = nullptr;
+ }
+ if (workQueue) {
+ delete workQueue;
+ workQueue = nullptr;
  }
  LogPrint(BCLog::HTTP, "Stopped HTTP server\n");
 }