Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Prevent Repository objects from being prematurely GCed #1337

Merged
merged 11 commits into from
Nov 28, 2017
Merged

Prevent Repository objects from being prematurely GCed #1337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e1b5f6a
­Keep­ alive reference to Repository and Dispose
jcansdale Nov 23, 2017
7cfbda3
Test GitService.GetRepository in a consistent way
jcansdale Nov 23, 2017
6f1a890
Dispose after using PullRequestSessionService.GetRepository
jcansdale Nov 23, 2017
8b17782
Use IRepository returned by GetActiveRepo inside using
jcansdale Nov 23, 2017
604a82c
Access GetRepositoryFromSolution from inside using
jcansdale Nov 23, 2017
c1746ae
Check we don't return reference to disposed Repository
jcansdale Nov 23, 2017
7fb09a3
Keep reference to TreeChanges and Dispose
jcansdale Nov 23, 2017
4db51b2
Don't dispose of Repository before it's used
jcansdale Nov 24, 2017
88a753a
Force the enumeration to complete before disposing
jcansdale Nov 24, 2017
b923899
Clean up dead code
jcansdale Nov 27, 2017
c61a75d
Merge branch 'master' into fixes/1315-using-repository
jcansdale Nov 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
318 changes: 177 additions & 141 deletions src/GitHub.App/Services/PullRequestService.cs
View file
Open in desktop

Large diffs are not rendered by default.

23 changes: 14 additions & 9 deletions src/GitHub.App/Services/RepositoryPublishService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@ public string LocalRepositoryName
{
get
{
var activeRepo = vsGitServices.GetActiveRepo();
if (!string.IsNullOrEmpty(activeRepo?.Info?.WorkingDirectory))
return new DirectoryInfo(activeRepo.Info.WorkingDirectory).Name ?? "";
return string.Empty;
using (var activeRepo = vsGitServices.GetActiveRepo())
{
if (!string.IsNullOrEmpty(activeRepo?.Info?.WorkingDirectory))
return new DirectoryInfo(activeRepo.Info.WorkingDirectory).Name ?? "";
return string.Empty;
}
}
}

Expand All @@ -43,11 +45,14 @@ public string LocalRepositoryName
.Select(remoteRepo => new { RemoteRepo = remoteRepo, LocalRepo = vsGitServices.GetActiveRepo() }))
.SelectMany(async repo =>
{
await gitClient.SetRemote(repo.LocalRepo, "origin", new Uri(repo.RemoteRepo.CloneUrl));
await gitClient.Push(repo.LocalRepo, "master", "origin");
await gitClient.Fetch(repo.LocalRepo, "origin");
await gitClient.SetTrackingBranch(repo.LocalRepo, "master", "origin");
return repo.RemoteRepo;
using (repo.LocalRepo)
{
await gitClient.SetRemote(repo.LocalRepo, "origin", new Uri(repo.RemoteRepo.CloneUrl));
await gitClient.Push(repo.LocalRepo, "master", "origin");
await gitClient.Fetch(repo.LocalRepo, "origin");
await gitClient.SetTrackingBranch(repo.LocalRepo, "master", "origin");
return repo.RemoteRepo;
}
});
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,8 +387,10 @@ public async Task Load(string remoteRepositoryOwner, IPullRequestModel pullReque
CommentCount = pullRequest.Comments.Count + pullRequest.ReviewComments.Count;
Body = !string.IsNullOrWhiteSpace(pullRequest.Body) ? pullRequest.Body : Resources.NoDescriptionProvidedMarkdown;

var changes = await pullRequestsService.GetTreeChanges(LocalRepository, pullRequest);
ChangedFilesTree = (await CreateChangedFilesTree(pullRequest, changes)).Children.ToList();
using (var changes = await pullRequestsService.GetTreeChanges(LocalRepository, pullRequest))
{
ChangedFilesTree = (await CreateChangedFilesTree(pullRequest, changes)).Children.ToList();
}

var localBranches = await pullRequestsService.GetLocalBranches(LocalRepository, pullRequest).ToList();

Expand Down
6 changes: 4 additions & 2 deletions src/GitHub.Exports/Extensions/LocalRepositoryModelExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ public static class LocalRepositoryModelExtensions
{
public static bool HasCommits(this ILocalRepositoryModel repository)
{
var repo = GitService.GitServiceHelper.GetRepository(repository.LocalPath);
return repo?.Commits.Any() ?? false;
using (var repo = GitService.GitServiceHelper.GetRepository(repository.LocalPath))
{
return repo?.Commits.Any() ?? false;
}
}

public static bool MightContainSolution(this ILocalRepositoryModel repository)
Expand Down
13 changes: 9 additions & 4 deletions src/GitHub.Exports/Models/LocalRepositoryModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,10 @@ public string HeadSha
{
get
{
var repo = GitService.GitServiceHelper.GetRepository(LocalPath);
return repo?.Commits.FirstOrDefault()?.Sha ?? String.Empty;
using (var repo = GitService.GitServiceHelper.GetRepository(LocalPath))
{
return repo?.Commits.FirstOrDefault()?.Sha ?? string.Empty;
}
}
}

Expand All @@ -169,8 +171,11 @@ public IBranch CurrentBranch
{
get
{
var repo = GitService.GitServiceHelper.GetRepository(LocalPath);
return new BranchModel(repo?.Head, this);
// BranchModel doesn't keep a reference to Repository
using (var repo = GitService.GitServiceHelper.GetRepository(LocalPath))
{
return new BranchModel(repo?.Head, this);
}
}
}

Expand Down
46 changes: 25 additions & 21 deletions src/GitHub.Exports/Services/GitService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@ public UriString GetUri(IRepository repository, string remote = "origin")
/// <returns>Returns a <see cref="UriString"/> representing the uri of the remote normalized to a GitHub repository url or null if none found.</returns>
public UriString GetUri(string path, string remote = "origin")
{
return GetUri(GetRepository(path), remote);
using (var repo = GetRepository(path))
{
return GetUri(repo, remote);
}
}

/// <summary>
Expand Down Expand Up @@ -82,29 +85,30 @@ public UriString GetRemoteUri(IRepository repo, string remote = "origin")
public Task<string> GetLatestPushedSha(string path)
{
Guard.ArgumentNotNull(path, nameof(path));
var repo = GetRepository(path);

if (repo == null)
return null;

if (repo.Head.IsTracking && repo.Head.Tip.Sha == repo.Head.TrackedBranch.Tip.Sha)
using (var repo = GetRepository(path))
{
return Task.FromResult(repo.Head.Tip.Sha);
}
if (repo == null)
return null;

if (repo.Head.IsTracking && repo.Head.Tip.Sha == repo.Head.TrackedBranch.Tip.Sha)
{
return Task.FromResult(repo.Head.Tip.Sha);
}

return Task.Factory.StartNew(() =>
{
var remoteHeads = repo.Refs.Where(r => r.IsRemoteTrackingBranch).ToList();
return Task.Factory.StartNew(() =>
{
var remoteHeads = repo.Refs.Where(r => r.IsRemoteTrackingBranch).ToList();

foreach (var c in repo.Commits)
{
if (repo.Refs.ReachableFrom(remoteHeads, new[] { c }).Any())
{
return c.Sha;
}
}
return null;
});
foreach (var c in repo.Commits)
{
if (repo.Refs.ReachableFrom(remoteHeads, new[] { c }).Any())
{
return c.Sha;
}
}
return null;
});
}
}
}
}
77 changes: 44 additions & 33 deletions src/GitHub.InlineReviews/Services/PullRequestSessionService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,19 @@ public PullRequestSessionService(
/// <inheritdoc/>
public virtual async Task<IReadOnlyList<DiffChunk>> Diff(ILocalRepositoryModel repository, string baseSha, string headSha, string relativePath)
{
var repo = await GetRepository(repository);
return await diffService.Diff(repo, baseSha, headSha, relativePath);
using (var repo = await GetRepository(repository))
{
return await diffService.Diff(repo, baseSha, headSha, relativePath);
}
}

/// <inheritdoc/>
public virtual async Task<IReadOnlyList<DiffChunk>> Diff(ILocalRepositoryModel repository, string baseSha, string headSha, string relativePath, byte[] contents)
{
var repo = await GetRepository(repository);
return await diffService.Diff(repo, baseSha, headSha, relativePath, contents);
using (var repo = await GetRepository(repository))
{
return await diffService.Diff(repo, baseSha, headSha, relativePath, contents);
}
}

/// <inheritdoc/>
Expand Down Expand Up @@ -175,18 +179,22 @@ public ITextDocument GetDocument(ITextBuffer buffer)
/// <inheritdoc/>
public virtual async Task<string> GetTipSha(ILocalRepositoryModel repository)
{
var repo = await GetRepository(repository);
return repo.Head.Tip.Sha;
using (var repo = await GetRepository(repository))
{
return repo.Head.Tip.Sha;
}
}

/// <inheritdoc/>
public async Task<bool> IsUnmodifiedAndPushed(ILocalRepositoryModel repository, string relativePath, byte[] contents)
{
var repo = await GetRepository(repository);
var modified = await gitClient.IsModified(repo, relativePath, contents);
var pushed = await gitClient.IsHeadPushed(repo);
using (var repo = await GetRepository(repository))
{
var modified = await gitClient.IsModified(repo, relativePath, contents);
var pushed = await gitClient.IsHeadPushed(repo);

return !modified && pushed;
return !modified && pushed;
}
}

public async Task<byte[]> ExtractFileFromGit(
Expand All @@ -195,17 +203,18 @@ public async Task<byte[]> ExtractFileFromGit(
string sha,
string relativePath)
{
var repo = await GetRepository(repository);

try
{
return await gitClient.ExtractFileBinary(repo, sha, relativePath);
}
catch (FileNotFoundException)
using (var repo = await GetRepository(repository))
{
var pullHeadRef = $"refs/pull/{pullRequestNumber}/head";
await gitClient.Fetch(repo, "origin", sha, pullHeadRef);
return await gitClient.ExtractFileBinary(repo, sha, relativePath);
try
{
return await gitClient.ExtractFileBinary(repo, sha, relativePath);
}
catch (FileNotFoundException)
{
var pullHeadRef = $"refs/pull/{pullRequestNumber}/head";
await gitClient.Fetch(repo, "origin", sha, pullHeadRef);
return await gitClient.ExtractFileBinary(repo, sha, relativePath);
}
}
}

Expand Down Expand Up @@ -242,21 +251,23 @@ public virtual async Task<string> GetPullRequestMergeBase(ILocalRepositoryModel
return mergeBase;
}

var repo = await GetRepository(repository);
var targetUrl = pullRequest.Base.RepositoryCloneUrl;
var headUrl = pullRequest.Head.RepositoryCloneUrl;
var baseRef = pullRequest.Base.Ref;
var pullNumber = pullRequest.Number;
try
using (var repo = await GetRepository(repository))
{
mergeBase = await gitClient.GetPullRequestMergeBase(repo, targetUrl, baseSha, headSha, baseRef, pullNumber);
}
catch (NotFoundException ex)
{
throw new NotFoundException("The Pull Request failed to load. Please check your network connection and click refresh to try again. If this issue persists, please let us know at support@github.com", ex);
}
var targetUrl = pullRequest.Base.RepositoryCloneUrl;
var headUrl = pullRequest.Head.RepositoryCloneUrl;
var baseRef = pullRequest.Base.Ref;
var pullNumber = pullRequest.Number;
try
{
mergeBase = await gitClient.GetPullRequestMergeBase(repo, targetUrl, baseSha, headSha, baseRef, pullNumber);
}
catch (NotFoundException ex)
{
throw new NotFoundException("The Pull Request failed to load. Please check your network connection and click refresh to try again. If this issue persists, please let us know at support@github.com", ex);
}

return mergeBaseCache[key] = mergeBase;
return mergeBaseCache[key] = mergeBase;
}
}

/// <inheritdoc/>
Expand Down
7 changes: 6 additions & 1 deletion src/GitHub.TeamFoundation.14/Services/VSGitServices.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,12 @@ public string GetActiveRepoPath()
if (repo != null)
ret = repo.RepositoryPath;
if (ret == null)
ret = serviceProvider.GetSolution().GetRepositoryFromSolution()?.Info?.Path;
{
using (var repository = serviceProvider.GetSolution().GetRepositoryFromSolution())
{
ret = repository?.Info?.Path;
}
}
return ret ?? String.Empty;
}

Expand Down
8 changes: 5 additions & 3 deletions src/GitHub.VisualStudio/Menus/MenuBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ protected ISimpleApiClient SimpleApiClient
protected ISimpleApiClientFactory ApiFactory => apiFactory.Value;

protected MenuBase()
{}
{ }

protected MenuBase(IGitHubServiceProvider serviceProvider)
{
Expand All @@ -55,8 +55,10 @@ protected ILocalRepositoryModel GetRepositoryByPath(string path)
{
if (!string.IsNullOrEmpty(path))
{
var repo = ServiceProvider.TryGetService<IGitService>().GetRepository(path);
return new LocalRepositoryModel(repo.Info.WorkingDirectory.TrimEnd('\\'));
using (var repo = ServiceProvider.TryGetService<IGitService>().GetRepository(path))
{
return new LocalRepositoryModel(repo.Info.WorkingDirectory.TrimEnd('\\'));
}
}
}
catch (Exception ex)
Expand Down
1 change: 0 additions & 1 deletion test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ public string AddFile(string path, string contents, string commitAlias)
public void Dispose()
{
var path = repository.Info.WorkingDirectory;
repository.Dispose();

// The .git folder has some files marked as readonly, meaning that a simple
// Directory.Delete doesn't work here.
Expand Down
Loading