From 5b700dc0c72de9c2f398e912784e14f1ae08b05f Mon Sep 17 00:00:00 2001 From: Matthias Thieroff Date: Tue, 21 Dec 2021 15:09:41 +0100 Subject: [PATCH] Fix closing of repository while getting the latest commit asynchronously (#1903) Getting the latest commit can run asynchronously, which can lead to a reopening of a repository after the RepositoryService has already closed it. This lead to an open file handle which prevented a proper deletion of the repository. We now explicitly open and close the repository when getting the latest commit. --- .../changelog/close_on_async_repo_access.yaml | 2 + .../scm/repository/spi/GitBrowseCommand.java | 42 ++++++++++--------- 2 files changed, 25 insertions(+), 19 deletions(-) create mode 100644 gradle/changelog/close_on_async_repo_access.yaml diff --git a/gradle/changelog/close_on_async_repo_access.yaml b/gradle/changelog/close_on_async_repo_access.yaml new file mode 100644 index 0000000000..3e3991c29f --- /dev/null +++ b/gradle/changelog/close_on_async_repo_access.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Closing of repository while getting the latest commit asynchronously ([#1903](https://github.com/scm-manager/scm-manager/pull/1903)) diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index 6baa421816..8378285e52 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -76,14 +76,14 @@ import static sonia.scm.NotFoundException.notFound; import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS; /** - * * @author Sebastian Sdorra */ public class GitBrowseCommand extends AbstractGitCommand - implements BrowseCommand -{ + implements BrowseCommand { - /** Field description */ + /** + * Field description + */ public static final String PATH_MODULES = ".gitmodules"; /** @@ -91,7 +91,9 @@ public class GitBrowseCommand extends AbstractGitCommand */ private static final Logger logger = LoggerFactory.getLogger(GitBrowseCommand.class); - /** sub repository cache */ + /** + * sub repository cache + */ private final Map> subrepositoryCache = Maps.newHashMap(); private final Object asyncMonitor = new Object(); @@ -170,21 +172,17 @@ public class GitBrowseCommand extends AbstractGitCommand file.setName(treeEntry.getNameString()); file.setPath(path); - if (treeEntry.getType() == TreeType.SUB_REPOSITORY) - { + if (treeEntry.getType() == TreeType.SUB_REPOSITORY) { logger.trace("{} seems to be a sub repository", path); file.setDirectory(true); file.setSubRepository(treeEntry.subRepository); - } - else - { + } else { ObjectLoader loader = repo.open(treeEntry.getObjectId()); file.setDirectory(loader.getType() == Constants.OBJ_TREE); // don't show message and date for directories to improve performance - if (!file.isDirectory() &&!request.isDisableLastCommit()) - { + if (!file.isDirectory() && !request.isDisableLastCommit()) { file.setPartialResult(true); RevCommit commit; try (RevWalk walk = new RevWalk(repo)) { @@ -232,7 +230,7 @@ public class GitBrowseCommand extends AbstractGitCommand return result; } else { FileObject result = findFirstMatch(treeWalk); - if ( result.isDirectory() ) { + if (result.isDirectory()) { treeWalk.enterSubtree(); findChildren(result, treeWalk); } @@ -255,8 +253,7 @@ public class GitBrowseCommand extends AbstractGitCommand List files = Lists.newArrayList(); Iterator entryIterator = entries.iterator(); boolean hasNext; - while ((hasNext = entryIterator.hasNext()) && resultCount < request.getLimit() + request.getOffset()) - { + while ((hasNext = entryIterator.hasNext()) && resultCount < request.getLimit() + request.getOffset()) { TreeEntry entry = entryIterator.next(); FileObject fileObject = createFileObject(entry); @@ -302,7 +299,7 @@ public class GitBrowseCommand extends AbstractGitCommand private String appendTrailingSlashIfNeeded(Deque parents) { String path = parents.peek().pathString; - return path.length() == 0? path: path + "/"; + return path.length() == 0 ? path : path + "/"; } private FileObject findFirstMatch(TreeWalk treeWalk) throws IOException { @@ -337,7 +334,7 @@ public class GitBrowseCommand extends AbstractGitCommand logger.debug("read submodules of {} at {}", repository, revId); - try ( ByteArrayOutputStream baos = new ByteArrayOutputStream() ) { + try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) { new GitCatCommand(context, lfsBlobStoreFactory).getContent(repo, revId, PATH_MODULES, baos); return GitSubModuleParser.parse(baos.toString()); } catch (NotFoundException ex) { @@ -416,8 +413,13 @@ public class GitBrowseCommand extends AbstractGitCommand logger.trace("finished loading of last commit {} of {} in {}", revId.getName(), path, sw.stop()); } - private Optional getLatestCommit(org.eclipse.jgit.lib.Repository repo, - ObjectId revId, String path) { + private Optional getLatestCommit(org.eclipse.jgit.lib.Repository repo, ObjectId revId, String path) { + // getLatestCommit can run async, which can lead to an open repository. + // Because if the RepositoryService is closed and getLatestCommit runs after that it will reopen the repository. + // So we increment the open counter and close after. + // The increment is required to not close the repository if the getLatestCommit runs before the RepositoryService + // is closed. + repo.incrementOpen(); try (RevWalk walk = new RevWalk(repo)) { walk.setTreeFilter(AndTreeFilter.create(TreeFilter.ANY_DIFF, PathFilter.create(path))); @@ -428,6 +430,8 @@ public class GitBrowseCommand extends AbstractGitCommand } catch (IOException ex) { logger.error("could not parse commit for file", ex); return empty(); + } finally { + repo.close(); } }