From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Fri, 4 Feb 2022 07:35:15 +0000 (+0100)
Subject: Do not hold the lock while running the task.
X-Git-Tag: auth-4.7.0-alpha1~19^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ad531f88fdca9ee1c908f069aca58f75d1b39711;p=thirdparty%2Fpdns.git

Do not hold the lock while running the task.

PLus some refactoring.
---

diff --git a/pdns/recursordist/rec-taskqueue.cc b/pdns/recursordist/rec-taskqueue.cc
index 60e1d48e6b..cc1e71f265 100644
--- a/pdns/recursordist/rec-taskqueue.cc
+++ b/pdns/recursordist/rec-taskqueue.cc
@@ -71,7 +71,18 @@ static void resolve(const struct timeval& now, bool logErrors, const pdns::Resol
 
 void runTaskOnce(bool logErrors)
 {
-  s_taskQueue.lock()->runOnce(logErrors);
+  pdns::ResolveTask task;
+  {
+    auto lock = s_taskQueue.lock();
+    if (lock->empty()) {
+      return;
+    }
+    task = lock->pop();
+  }
+  bool expired = task.run(logErrors);
+  if (expired) {
+    s_taskQueue.lock()->incExpired();
+  }
 }
 
 void pushAlmostExpiredTask(const DNSName& qname, uint16_t qtype, time_t deadline)
diff --git a/pdns/recursordist/rec-taskqueue.hh b/pdns/recursordist/rec-taskqueue.hh
index 2e9df55523..95e734b68d 100644
--- a/pdns/recursordist/rec-taskqueue.hh
+++ b/pdns/recursordist/rec-taskqueue.hh
@@ -25,6 +25,7 @@
 
 void runTaskOnce(bool logErrors);
 void pushAlmostExpiredTask(const DNSName& qname, uint16_t qtype, time_t deadline);
+
 // General task stats
 uint64_t getTaskPushes();
 uint64_t getTaskExpired();
diff --git a/pdns/recursordist/taskqueue.cc b/pdns/recursordist/taskqueue.cc
index 817bd94e21..cc1a4b4be7 100644
--- a/pdns/recursordist/taskqueue.cc
+++ b/pdns/recursordist/taskqueue.cc
@@ -28,20 +28,9 @@
 namespace pdns
 {
 
-bool TaskQueue::empty() const
-{
-  return d_queue.empty();
-}
-
-size_t TaskQueue::size() const
-{
-  return d_queue.size();
-}
-
 void TaskQueue::push(ResolveTask&& task)
 {
   // Insertion fails if it's already there, no problem since we're already scheduled
-  // and the deadline would remain the same anyway.
   auto result = d_queue.insert(std::move(task));
   if (result.second) {
     d_pushes++;
@@ -55,34 +44,23 @@ ResolveTask TaskQueue::pop()
   return ret;
 }
 
-bool TaskQueue::runOnce(bool logErrors)
+bool ResolveTask::run(bool logErrors)
 {
-  if (d_queue.empty()) {
+  if (!d_func) {
+    g_log << Logger::Debug << "TaskQueue: null task for " << d_qname.toString() << '|' << QType(d_qtype).toString() << endl;
     return false;
   }
-  ResolveTask task = pop();
-  if (task.func == nullptr) {
-    g_log << Logger::Debug << "TaskQueue: null task for " << task.d_qname.toString() << '|' << QType(task.d_qtype).toString() << endl;
-    return true;
-  }
   struct timeval now;
   Utility::gettimeofday(&now);
-  if (task.d_deadline >= now.tv_sec) {
-    task.func(now, logErrors, task);
+  if (d_deadline >= now.tv_sec) {
+    d_func(now, logErrors, *this);
   }
   else {
     // Deadline passed
-    g_log << Logger::Debug << "TaskQueue: deadline for " << task.d_qname.toString() << '|' << QType(task.d_qtype).toString() << " passed" << endl;
-    d_expired++;
-  }
-  return true;
-}
-
-void TaskQueue::runAll(bool logErrors)
-{
-  while (runOnce(logErrors)) {
-    /* empty */
+    g_log << Logger::Debug << "TaskQueue: deadline for " << d_qname.toString() << '|' << QType(d_qtype).toString() << " passed" << endl;
+    return true;
   }
+  return false;
 }
 
 } /* namespace pdns */
diff --git a/pdns/recursordist/taskqueue.hh b/pdns/recursordist/taskqueue.hh
index 3094079ae8..55c307a902 100644
--- a/pdns/recursordist/taskqueue.hh
+++ b/pdns/recursordist/taskqueue.hh
@@ -47,36 +47,26 @@ struct ResolveTask
   uint16_t d_qtype;
   time_t d_deadline;
   bool d_refreshMode; // Whether to run this task in regular mode (false) or in the mode that refreshes almost expired tasks
-  void (*func)(const struct timeval&, bool logErrors, const ResolveTask&);
-};
+  std::function<void(const struct timeval& now, bool logErrors, const ResolveTask& task)> d_func;
 
-struct HashTag
-{
+  bool run(bool logErrors);
 };
-struct SequencedTag
-{
-};
-
-typedef multi_index_container<
-  ResolveTask,
-  indexed_by<
-    hashed_unique<tag<HashTag>,
-                  composite_key<ResolveTask,
-                                member<ResolveTask, DNSName, &ResolveTask::d_qname>,
-                                member<ResolveTask, uint16_t, &ResolveTask::d_qtype>,
-                                member<ResolveTask, bool, &ResolveTask::d_refreshMode>>>,
-    sequenced<tag<SequencedTag>>>>
-  queue_t;
 
 class TaskQueue
 {
 public:
-  bool empty() const;
-  size_t size() const;
+  bool empty() const
+  {
+    return d_queue.empty();
+  }
+
+  size_t size() const
+  {
+    return d_queue.size();
+  }
+
   void push(ResolveTask&& task);
   ResolveTask pop();
-  bool runOnce(bool logErrors); // Run one task if the queue is not empty
-  void runAll(bool logErrors);
 
   uint64_t getPushes()
   {
@@ -88,7 +78,29 @@ public:
     return d_expired;
   }
 
+  void incExpired()
+  {
+    d_expired++;
+  }
+
 private:
+  struct HashTag
+  {
+  };
+  struct SequencedTag
+  {
+  };
+  typedef multi_index_container<
+    ResolveTask,
+    indexed_by<
+      hashed_unique<tag<HashTag>,
+                    composite_key<ResolveTask,
+                                  member<ResolveTask, DNSName, &ResolveTask::d_qname>,
+                                  member<ResolveTask, uint16_t, &ResolveTask::d_qtype>,
+                                  member<ResolveTask, bool, &ResolveTask::d_refreshMode>>>,
+      sequenced<tag<SequencedTag>>>>
+    queue_t;
+
   queue_t d_queue;
   uint64_t d_pushes{0};
   uint64_t d_expired{0};