Handle sharing repository object and cleanup (#47)

src/controllers/branches/branches.controller.e2e.ts

@@ -24,6 +24,12 @@ describe("Test branch controller", () => {
     for (const content of others) {
       expect(sortBy(content, x => x.name)).toEqual(sortBy(first, x => x.name));
     }
+
+    // Make sure the server didn't crash. This is a regression test where a segfault happened freeing the Repository object.
+    await delay(1000);
+    const lastResponse = await e2eClient.fetch(`/repos/${TEST_REPO}/branches`);
+
+    expect(lastResponse.status).toEqual(200);
   });
 
   it("List branches in the test repo", async () => {

src/core/index.ts

@@ -1,6 +1,7 @@
 export * from "./repo-auth";
 export * from "./logger";
 export * from "./pagination";
+export * from "./repo";
 export * from "./telemetry";
 export * from "./models";
 export * from "./context";

src/core/repo/gc-repo.ts

@@ -0,0 +1,37 @@
+import { Repository } from "nodegit";
+
+import { RepositoryDestroyedError } from "./repo-destroyed-error";
+
+export class GCRepo {
+  public path: string;
+  private references = 1;
+  private destroyed = false;
+
+  constructor(private repo: Repository) {
+    this.path = this.repo.path();
+  }
+
+  /**
+   * Get access to the repository
+   */
+  public get instance() {
+    // Guard to make sure you are not using the repo after it was destroyed which would cause a segmentation fault and crash the server.
+    // This will just throw an error which will result in a 500 which can be diagnoistied much better
+    if (this.destroyed) {
+      throw new RepositoryDestroyedError(this.path);
+    }
+    return this.repo;
+  }
+
+  public lock() {
+    this.references++;
+  }
+
+  public unlock() {
+    this.references--;
+    if (this.references === 0) {
+      this.destroyed = true;
+      this.repo.cleanup();
+    }
+  }
+}

src/core/repo/index.ts

@@ -0,0 +1,2 @@
+export * from "./gc-repo";
+export * from "./repo-destroyed-error";

src/core/repo/repo-destroyed-error.ts

@@ -0,0 +1,8 @@
+export class RepositoryDestroyedError extends Error {
+  constructor(public path: string) {
+    super(
+      `Cannot access repository that was destroyed. There must be an issue.` +
+        ` Make sure to call .lock() Path: ${path}`,
+    );
+  }
+}

src/services/repo/repo.service.ts

@@ -2,7 +2,7 @@ import { Injectable, NotFoundException } from "@nestjs/common";
 import { Remote, Repository } from "nodegit";
 import path from "path";
 
-import { RepoAuth } from "../../core";
+import { GCRepo, RepoAuth } from "../../core";
 import { FSService } from "../fs";
 import { GitFetchService, repoCacheFolder } from "../git-fetch";
 import { GitRemotePermission, PermissionService } from "../permission";
@@ -28,7 +28,7 @@ export class RepoService {
   /**
    * Map that contains a key and promise when cloning a given repo
    */
-  private cloningRepos = new Map<string, Promise<Repository>>();
+  private cloningRepos = new Map<string, Promise<GCRepo>>();
 
   constructor(
     private fs: FSService,
@@ -41,13 +41,13 @@ export class RepoService {
     return this.using(repo, action);
   }
 
-  public async using<T>(repo: Repository, action: (repo: Repository) => Promise<T>): Promise<T> {
+  public async using<T>(repo: GCRepo, action: (repo: Repository) => Promise<T>): Promise<T> {
     try {
-      const response = await action(repo);
-      repo.cleanup();
+      const response = await action(repo.instance);
+      repo.unlock();
       return response;
     } catch (error) {
-      repo.cleanup();
+      repo.unlock();
       throw error;
     }
   }
@@ -55,80 +55,80 @@ export class RepoService {
   /**
    * Be carfull with using this one. Repository object needs to be clenup. Make sure its with `using` to ensure it gets cleanup after
    */
-  public async get(remote: string, options: GitBaseOptions = {}): Promise<Repository> {
+  public async get(remote: string, options: GitBaseOptions = {}): Promise<GCRepo> {
     await this.validatePermissions([remote], options);
     const repoPath = getRepoMainPath(remote);
 
+    return this.loadRepo({
+      repoPath,
+      fetch: repo => this.fetchService.fetch(remote, repo, options),
+      clone: () => this.fetchService.clone(remote, repoPath, options),
+    });
+  }
+
+  public async createForCompare(base: RemoteDef, head: RemoteDef, options: GitBaseOptions = {}): Promise<GCRepo> {
+    await this.validatePermissions([base.remote, head.remote], options);
+    const localName = `${base.remote}-${head.remote}`;
+    const repoPath = getRepoMainPath(localName, "compare");
+
+    const fetch = (repo: Repository) => this.fetchService.fetch(localName, repo, options);
+    return this.loadRepo({
+      repoPath,
+      fetch,
+      clone: async () => {
+        const repo = await this.cloneWithMultiRemote(repoPath, [head, base]);
+        await fetch(repo);
+        return repo;
+      },
+    });
+  }
+
+  /**
+   * Generic repo loader that manage concurrency issue with cloning/fetching a repo
+   */
+  private async loadRepo(config: {
+    repoPath: string;
+    fetch: (repo: Repository) => Promise<unknown>;
+    clone: () => Promise<Repository>;
+  }) {
+    const repoPath = config.repoPath;
     const cloningRepo = this.cloningRepos.get(repoPath);
     if (cloningRepo) {
-      return cloningRepo;
+      return cloningRepo.then(x => {
+        x.lock(); // Need to lock the repo as this object can be shared between requests
+        return x;
+      });
     }
-
     const exists = await this.fs.exists(repoPath);
-    // Check again if the repo didn't start cloning since the last time
     const isCloningRepo = this.cloningRepos.get(repoPath);
     if (isCloningRepo) {
-      return isCloningRepo;
+      return isCloningRepo.then(x => {
+        x.lock(); // Need to lock the repo as this object can be shared between requests
+        return x;
+      });
     }
+
     if (exists) {
       const repo = await Repository.open(repoPath);
-      return this.fetchService.fetch(remote, repo, options);
+      await config.fetch(repo);
+      return new GCRepo(repo);
     } else {
-      const cloneRepoPromise = this.fetchService.clone(remote, repoPath, options).then(repo => {
+      const cloneRepoPromise = config.clone().then(repo => {
         this.cloningRepos.delete(repoPath);
-        return repo;
+        return new GCRepo(repo);
       });
       this.cloningRepos.set(repoPath, cloneRepoPromise);
       return cloneRepoPromise;
     }
   }
 
-  public async createForCompare(base: RemoteDef, head: RemoteDef, options: GitBaseOptions = {}): Promise<Repository> {
-    await this.validatePermissions([base.remote, head.remote], options);
-    const localName = `${base.remote}-${head.remote}`;
-    const repoPath = getRepoMainPath(localName, "compare");
-    let repo: Repository;
-
-    const cloningRepo = this.cloningRepos.get(repoPath);
-    if (cloningRepo) {
-      return cloningRepo;
-    }
-
-    const exists = await this.fs.exists(repoPath);
-    // Check again if the repo didn't start cloning since the last time
-    const isCloningRepo = this.cloningRepos.get(repoPath);
-    if (isCloningRepo) {
-      return isCloningRepo;
-    }
-
-    if (exists) {
-      repo = await Repository.open(repoPath);
-    } else {
-      const cloneRepoPromise = this.cloneWithMultiRemote(localName, repoPath, [head, base], options).then(r => {
-        this.cloningRepos.delete(repoPath);
-        return r;
-      });
-      this.cloningRepos.set(repoPath, cloneRepoPromise);
-      repo = await cloneRepoPromise;
-    }
-
-    return repo;
-  }
-
-  private async cloneWithMultiRemote(
-    localName: string,
-    repoPath: string,
-    remotes: RemoteDef[],
-    options: GitBaseOptions = {},
-  ) {
+  private async cloneWithMultiRemote(repoPath: string, remotes: RemoteDef[]) {
     const repo = await Repository.init(repoPath, 0);
     // Remotes cannot be added in parrelel.
     for (const { name, remote } of remotes) {
       await Remote.create(repo, name, `https://${remote}`);
     }
 
-    await this.fetchService.fetch(localName, repo, options);
-
     return repo;
   }