nodegit · ianhattendorf · Mar 17, 2022 · Dec 14, 2021 · Jan 23, 2022 · Jan 23, 2022
diff --git a/generate/templates/manual/include/thread_pool.h b/generate/templates/manual/include/thread_pool.h
@@ -8,6 +8,20 @@
 
 #include "async_worker.h"
 
+// Temporary workaround for LFS checkout. Comment added to be reverted.
+// With the threadpool rewrite, a Worker will execute its callbacks with
+// objects temporary unlock (to prevent deadlocks), and we'll wait until
+// the callback is done to lock them back again (to make sure it's thread-safe).
+// LFS checkout lost performance after this, and the proper way to fix it is
+// to integrate nodegit-lfs into nodegit. Until this is implemented, a
+// temporary workaround has been applied, which affects only Workers leveraging
+// threaded libgit2 functions (at the moment only checkout) and does the
+// following:
+// - do not wait for the current callback to end, so that it can send the
+//  next callback to the main JS thread.
+// - do not temporary unlock the objects, since they would be locked back
+//  again before the callback is executed.
+
 namespace nodegit {
   class Context;
   class AsyncContextCleanupHandle;
@@ -17,7 +31,9 @@ namespace nodegit {
     public:
       typedef std::function<void()> Callback;
       typedef std::function<void(Callback, Callback)> QueueCallbackFn;
-      typedef std::function<Callback(QueueCallbackFn, Callback)> OnPostCallbackFn;
+      // Temporary workaround for LFS checkout. Code modified to be reverted.
+      // typedef std::function<Callback(QueueCallbackFn, Callback)> OnPostCallbackFn;
+      typedef std::function<Callback(QueueCallbackFn, Callback, bool)> OnPostCallbackFn;
 
       // Initializes thread pool and spins up the requested number of threads
       // The provided loop will be used for completion callbacks, whenever

diff --git a/generate/templates/manual/src/async_baton.cc b/generate/templates/manual/src/async_baton.cc
@@ -46,7 +46,8 @@ namespace nodegit {
       ThreadPool::PostCallbackEvent(
         [jsCallback, cancelCallback](
           ThreadPool::QueueCallbackFn queueCallback,
-          ThreadPool::Callback callbackCompleted
+          ThreadPool::Callback callbackCompleted,
+          bool isThreaded // Temporary workaround for LFS checkout. Code added to be reverted.
         ) -> ThreadPool::Callback {
           queueCallback(jsCallback, cancelCallback);
           callbackCompleted();
@@ -58,13 +59,22 @@ namespace nodegit {
       ThreadPool::PostCallbackEvent(
         [this, jsCallback, cancelCallback](
           ThreadPool::QueueCallbackFn queueCallback,
-          ThreadPool::Callback callbackCompleted
+          ThreadPool::Callback callbackCompleted,
+          bool isThreaded // Temporary workaround for LFS checkout. Code added to be reverted.
         ) -> ThreadPool::Callback {
-          this->onCompletion = callbackCompleted;
+          // Temporary workaround for LFS checkout. Code modified to be reverted.
+          if (!isThreaded) {
+            this->onCompletion = callbackCompleted;
 
-          queueCallback(jsCallback, cancelCallback);
+            queueCallback(jsCallback, cancelCallback);
 
-          return std::bind(&AsyncBaton::SignalCompletion, this);
+            return std::bind(&AsyncBaton::SignalCompletion, this);
+          }
+          else {
+            this->onCompletion = std::bind(&AsyncBaton::SignalCompletion, this);
+            queueCallback(jsCallback, cancelCallback);
+            return []() {};
+          }
         }
       );
 

diff --git a/generate/templates/manual/src/thread_pool.cc b/generate/templates/manual/src/thread_pool.cc
@@ -7,6 +7,7 @@
 #include <queue>
 #include <thread>
 #include <utility>
+#include <atomic> // Temporary workaround for LFS checkout. Code added to be reverted.
 
 extern "C" {
   #include <git2/sys/custom_tls.h>
@@ -81,8 +82,11 @@ namespace nodegit {
           : Event(CALLBACK_TYPE), callback(initCallback)
         {}
 
-        ThreadPool::Callback operator()(ThreadPool::QueueCallbackFn queueCb, ThreadPool::Callback completedCb) {
-          return callback(queueCb, completedCb);
+        // Temporary workaround for LFS checkout. Code modified to be reverted.
+        // ThreadPool::Callback operator()(ThreadPool::QueueCallbackFn queueCb, ThreadPool::Callback completedCb) {
+        //   return callback(queueCb, completedCb);
+        ThreadPool::Callback operator()(ThreadPool::QueueCallbackFn queueCb, ThreadPool::Callback completedCb, bool isThreaded) {
+          return callback(queueCb, completedCb, isThreaded);
         }
 
         private:
@@ -102,6 +106,10 @@ namespace nodegit {
       // the Orchestrator's memory
       void WaitForThreadClose();
 
+      // Temporary workaround for LFS checkout. Code added to be reverted.
+      // Returns true if the task running spawned threads within libgit2
+      bool IsGitThreaded() { return currentGitThreads > kInitialGitThreads; }
+
       static Nan::AsyncResource *GetCurrentAsyncResource();
 
       static const nodegit::Context *GetCurrentContext();
@@ -139,6 +147,12 @@ namespace nodegit {
       PostCompletedEventToOrchestratorFn postCompletedEventToOrchestrator;
       TakeNextTaskFn takeNextTask;
       std::thread thread;
+
+      // Temporary workaround for LFS checkout. Code added to be reverted.
+      static constexpr int kInitialGitThreads {0};
+      // Number of threads spawned internally by libgit2 to deal with
+      // the task of this Executor instance. Defaults to kInitialGitThreads.
+      std::atomic<int> currentGitThreads {kInitialGitThreads};      
   };
 
   Executor::Executor(
@@ -170,6 +184,9 @@ namespace nodegit {
 
       WorkTask *workTask = static_cast<WorkTask *>(task.get());
 
+      // Temporary workaround for LFS checkout. Code added to be reverted.
+      currentGitThreads = kInitialGitThreads;
+
       currentAsyncResource = workTask->asyncResource;
       currentCallbackErrorHandle = workTask->callbackErrorHandle;
       workTask->callback();
@@ -221,6 +238,8 @@ namespace nodegit {
   }
 
   void *Executor::RetrieveTLSForLibgit2ChildThread() {
+    // Temporary workaround for LFS checkout. Code added to be reverted.
+    ++Executor::executor->currentGitThreads;
     return Executor::executor;
   }
 
@@ -230,6 +249,8 @@ namespace nodegit {
 
   void Executor::TeardownTLSOnLibgit2ChildThread() {
     if (!isExecutorThread) {
+      // Temporary workaround for LFS checkout. Code added to be reverted.
+      --Executor::executor->currentGitThreads;
       Executor::executor = nullptr;
     }
   }
@@ -378,21 +399,46 @@ namespace nodegit {
             std::shared_ptr<std::condition_variable> callbackCondition(new std::condition_variable);
             bool hasCompleted = false;
 
-            LockMaster::TemporaryUnlock temporaryUnlock;
+            // Temporary workaround for LFS checkout. Code removed to be reverted.
+            //LockMaster::TemporaryUnlock temporaryUnlock;
+
+            // Temporary workaround for LFS checkout. Code added to be reverted.
+            bool isWorkerThreaded = executor.IsGitThreaded();
+            ThreadPool::Callback callbackCompleted = []() {};
+            if (!isWorkerThreaded) {
+              callbackCompleted = [callbackCondition, callbackMutex, &hasCompleted]() {
+                std::lock_guard<std::mutex> lock(*callbackMutex);
+                hasCompleted = true;
+                callbackCondition->notify_one();
+              };
+            }
+            std::unique_ptr<LockMaster::TemporaryUnlock> temporaryUnlock {nullptr};
+            if (!isWorkerThreaded) {
+              temporaryUnlock = std::make_unique<LockMaster::TemporaryUnlock>();
+            }
+
             auto onCompletedCallback = (*callbackEvent)(
               [this](ThreadPool::Callback callback, ThreadPool::Callback cancelCallback) {
                 queueCallbackOnJSThread(callback, cancelCallback, false);
               },
+              // Temporary workaround for LFS checkout. Code modified to be reverted.
+              /*
               [callbackCondition, callbackMutex, &hasCompleted]() {
                 std::lock_guard<std::mutex> lock(*callbackMutex);
                 hasCompleted = true;
                 callbackCondition->notify_one();
               }
+              */
+              callbackCompleted,
+              isWorkerThreaded
             );
 
-            std::unique_lock<std::mutex> lock(*callbackMutex);
-            while (!hasCompleted) callbackCondition->wait(lock);
-            onCompletedCallback();
+            // Temporary workaround for LFS checkout. Code modified to be reverted.
+            if (!isWorkerThreaded) {
+              std::unique_lock<std::mutex> lock(*callbackMutex);
+              while (!hasCompleted) callbackCondition->wait(lock);
+              onCompletedCallback();
+            }
           }
 
           queueCallbackOnJSThread(
@@ -479,10 +525,6 @@ namespace nodegit {
 
       void QueueCallbackOnJSThread(ThreadPool::Callback callback, ThreadPool::Callback cancelCallback, bool isWork);
 
-      static void RunJSThreadCallbacksFromOrchestrator(uv_async_t *handle);
-
-      void RunJSThreadCallbacksFromOrchestrator();
-
       static void RunLoopCallbacks(uv_async_t *handle);
 
       void Shutdown(std::unique_ptr<AsyncContextCleanupHandle> cleanupHandle);
@@ -498,7 +540,6 @@ namespace nodegit {
 
     private:
       bool isMarkedForDeletion;
-      nodegit::Context *currentContext;
 
       struct JSThreadCallback {
         JSThreadCallback(ThreadPool::Callback callback, ThreadPool::Callback cancelCallback, bool isWork)
@@ -539,9 +580,9 @@ namespace nodegit {
       std::vector<Orchestrator> orchestrators;
   };
 
+  // context required to be passed to Orchestrators, but ThreadPoolImpl doesn't need to keep it
   ThreadPoolImpl::ThreadPoolImpl(int numberOfThreads, uv_loop_t *loop, nodegit::Context *context)
     : isMarkedForDeletion(false),
-      currentContext(context),
       orchestratorJobMutex(new std::mutex),
       jsThreadCallbackMutex(new std::mutex)
   {

diff --git a/test/tests/filter.js b/test/tests/filter.js
@@ -333,6 +333,82 @@ describe("Filter", function() {
         });
     });
 
+    it("can run sync callback on checkout without deadlocking", function() { // jshint ignore:line
+      var test = this;
+      var syncCallbackResult = 1;
+
+      return Registry.register(filterName, {
+        apply: function() {
+          syncCallbackResult = test.repository.isEmpty();
+        },
+        check: function() {
+          return NodeGit.Error.CODE.OK;
+        }
+      }, 0)
+        .then(function(result) {
+          assert.strictEqual(result, NodeGit.Error.CODE.OK);
+          return fse.writeFile(
+            packageJsonPath,
+            "Changing content to trigger checkout",
+            { encoding: "utf-8" }
+          );
+        })
+        .then(function() {
+          var opts = {
+            checkoutStrategy: Checkout.STRATEGY.FORCE,
+            paths: "package.json"
+          };
+          return Checkout.head(test.repository, opts);
+        })
+        .then(function() {
+          assert.strictEqual(syncCallbackResult, 0);
+        });
+    });
+
+    // Temporary workaround for LFS checkout. Test skipped.
+    // To activate when reverting workaround.
+    // 'Checkout.head' and 'Submodule.lookup' do work with the repo locked.
+    // They should work together without deadlocking.
+    it.skip("can run async callback on checkout without deadlocking", function() { // jshint ignore:line
+      var test = this;
+      var submoduleNameIn = "vendor/libgit2";
+      var asyncCallbackResult = "";
+
+      return Registry.register(filterName, {
+        apply: function() {
+          return NodeGit.Submodule.lookup(test.repository, submoduleNameIn)
+            .then(function(submodule) {
+              return submodule.name();
+            })
+            .then(function(name) {
+              asyncCallbackResult = name;
+              return NodeGit.Error.CODE.OK;
+            });
+        },
+        check: function() {
+          return NodeGit.Error.CODE.OK;
+        }
+      }, 0)
+        .then(function(result) {
+          assert.strictEqual(result, NodeGit.Error.CODE.OK);
+          return fse.writeFile(
+            packageJsonPath,
+            "Changing content to trigger checkout",
+            { encoding: "utf-8" }
+          );
+        })
+        .then(function() {
+          var opts = {
+            checkoutStrategy: Checkout.STRATEGY.FORCE,
+            paths: "package.json"
+          };
+          return Checkout.head(test.repository, opts);
+        })
+        .then(function() {
+          assert.equal(asyncCallbackResult, submoduleNameIn);
+        });
+    });
+
     // this test is useless on 32 bit CI, because we cannot construct
     // a buffer big enough to test anything of significance :)...
     if (process.arch === "x64") {

diff --git a/test/tests/submodule.js b/test/tests/submodule.js
@@ -157,4 +157,38 @@ describe("Submodule", function() {
         assert.equal(entries[1].path, submodulePath);
       });
   });
+
+  it("can run sync callback without deadlocking", function() {
+    var repo = this.workdirRepository;
+    var submodules = [];
+    var submoduleCallback = function(submodule, name, payload) {
+      var submoduleName = submodule.name();
+      assert.equal(submoduleName, name);
+      submodules.push(name);
+    };
+
+    return Submodule.foreach(repo, submoduleCallback).then(function() {
+      assert.equal(submodules.length, 1);
+    });
+  });
+
+  // 'Submodule.foreach' and 'Submodule.lookup' do work with the repo locked.
+  // They should work together without deadlocking.
+  it("can run async callback without deadlocking", function() {
+    var repo = this.workdirRepository;
+    var submodules = [];
+    var submoduleCallback = function(submodule, name, payload) {
+      var owner = submodule.owner();
+
+      return Submodule.lookup(owner, name)
+        .then(function(submodule) {
+          assert.equal(submodule.name(), name);
+          submodules.push(name);
+        });
+    };
+
+    return Submodule.foreach(repo, submoduleCallback).then(function() {
+      assert.equal(submodules.length, 1);
+    });
+  });
 });