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

Commit 9002364

Browse files
jcansdalejcansdale
authored
Merge pull request #1337 from github/fixes/1315-using-repository
Prevent Repository objects from being prematurely GCed
2 parents c034b4f + c61a75d commit 9002364

File tree

11 files changed

+333
-258
lines changed

11 files changed

+333
-258
lines changed

‎src/GitHub.App/Services/PullRequestService.cs‎

Lines changed: 177 additions & 141 deletions
Large diffs are not rendered by default.

‎src/GitHub.App/Services/RepositoryPublishService.cs‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,12 @@ public string LocalRepositoryName
2626
{
2727
get
2828
{
29-
var activeRepo = vsGitServices.GetActiveRepo();
30-
if (!string.IsNullOrEmpty(activeRepo?.Info?.WorkingDirectory))
31-
return new DirectoryInfo(activeRepo.Info.WorkingDirectory).Name ?? "";
32-
return string.Empty;
29+
using (var activeRepo = vsGitServices.GetActiveRepo())
30+
{
31+
if (!string.IsNullOrEmpty(activeRepo?.Info?.WorkingDirectory))
32+
return new DirectoryInfo(activeRepo.Info.WorkingDirectory).Name ?? "";
33+
return string.Empty;
34+
}
3335
}
3436
}
3537

@@ -43,11 +45,14 @@ public string LocalRepositoryName
4345
.Select(remoteRepo => new { RemoteRepo = remoteRepo, LocalRepo = vsGitServices.GetActiveRepo() }))
4446
.SelectMany(async repo =>
4547
{
46-
await gitClient.SetRemote(repo.LocalRepo, "origin", new Uri(repo.RemoteRepo.CloneUrl));
47-
await gitClient.Push(repo.LocalRepo, "master", "origin");
48-
await gitClient.Fetch(repo.LocalRepo, "origin");
49-
await gitClient.SetTrackingBranch(repo.LocalRepo, "master", "origin");
50-
return repo.RemoteRepo;
48+
using (repo.LocalRepo)
49+
{
50+
await gitClient.SetRemote(repo.LocalRepo, "origin", new Uri(repo.RemoteRepo.CloneUrl));
51+
await gitClient.Push(repo.LocalRepo, "master", "origin");
52+
await gitClient.Fetch(repo.LocalRepo, "origin");
53+
await gitClient.SetTrackingBranch(repo.LocalRepo, "master", "origin");
54+
return repo.RemoteRepo;
55+
}
5156
});
5257
}
5358
}

‎src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -387,8 +387,10 @@ public async Task Load(string remoteRepositoryOwner, IPullRequestModel pullReque
387387
CommentCount = pullRequest.Comments.Count + pullRequest.ReviewComments.Count;
388388
Body = !string.IsNullOrWhiteSpace(pullRequest.Body) ? pullRequest.Body : Resources.NoDescriptionProvidedMarkdown;
389389

390-
var changes = await pullRequestsService.GetTreeChanges(LocalRepository, pullRequest);
391-
ChangedFilesTree = (await CreateChangedFilesTree(pullRequest, changes)).Children.ToList();
390+
using (var changes = await pullRequestsService.GetTreeChanges(LocalRepository, pullRequest))
391+
{
392+
ChangedFilesTree = (await CreateChangedFilesTree(pullRequest, changes)).Children.ToList();
393+
}
392394

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

‎src/GitHub.Exports/Extensions/LocalRepositoryModelExtensions.cs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,10 @@ public static class LocalRepositoryModelExtensions
1010
{
1111
public static bool HasCommits(this ILocalRepositoryModel repository)
1212
{
13-
var repo = GitService.GitServiceHelper.GetRepository(repository.LocalPath);
14-
return repo?.Commits.Any() ?? false;
13+
using (var repo = GitService.GitServiceHelper.GetRepository(repository.LocalPath))
14+
{
15+
return repo?.Commits.Any() ?? false;
16+
}
1517
}
1618

1719
public static bool MightContainSolution(this ILocalRepositoryModel repository)

‎src/GitHub.Exports/Models/LocalRepositoryModel.cs‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,10 @@ public string HeadSha
157157
{
158158
get
159159
{
160-
var repo = GitService.GitServiceHelper.GetRepository(LocalPath);
161-
return repo?.Commits.FirstOrDefault()?.Sha ?? String.Empty;
160+
using (var repo = GitService.GitServiceHelper.GetRepository(LocalPath))
161+
{
162+
return repo?.Commits.FirstOrDefault()?.Sha ?? string.Empty;
163+
}
162164
}
163165
}
164166

@@ -169,8 +171,11 @@ public IBranch CurrentBranch
169171
{
170172
get
171173
{
172-
var repo = GitService.GitServiceHelper.GetRepository(LocalPath);
173-
return new BranchModel(repo?.Head, this);
174+
// BranchModel doesn't keep a reference to Repository
175+
using (var repo = GitService.GitServiceHelper.GetRepository(LocalPath))
176+
{
177+
return new BranchModel(repo?.Head, this);
178+
}
174179
}
175180
}
176181

‎src/GitHub.Exports/Services/GitService.cs‎

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,10 @@ public UriString GetUri(IRepository repository, string remote = "origin")
3838
/// <returns>Returns a <see cref="UriString"/> representing the uri of the remote normalized to a GitHub repository url or null if none found.</returns>
3939
public UriString GetUri(string path, string remote = "origin")
4040
{
41-
return GetUri(GetRepository(path), remote);
41+
using (var repo = GetRepository(path))
42+
{
43+
return GetUri(repo, remote);
44+
}
4245
}
4346

4447
/// <summary>
@@ -82,29 +85,30 @@ public UriString GetRemoteUri(IRepository repo, string remote = "origin")
8285
public Task<string> GetLatestPushedSha(string path)
8386
{
8487
Guard.ArgumentNotNull(path, nameof(path));
85-
var repo = GetRepository(path);
86-
87-
if (repo == null)
88-
return null;
89-
90-
if (repo.Head.IsTracking && repo.Head.Tip.Sha == repo.Head.TrackedBranch.Tip.Sha)
88+
using (var repo = GetRepository(path))
9189
{
92-
return Task.FromResult(repo.Head.Tip.Sha);
93-
}
90+
if (repo == null)
91+
return null;
92+
93+
if (repo.Head.IsTracking && repo.Head.Tip.Sha == repo.Head.TrackedBranch.Tip.Sha)
94+
{
95+
return Task.FromResult(repo.Head.Tip.Sha);
96+
}
9497

95-
return Task.Factory.StartNew(() =>
96-
{
97-
var remoteHeads = repo.Refs.Where(r => r.IsRemoteTrackingBranch).ToList();
98+
return Task.Factory.StartNew(() =>
99+
{
100+
var remoteHeads = repo.Refs.Where(r => r.IsRemoteTrackingBranch).ToList();
98101

99-
foreach (var c in repo.Commits)
100-
{
101-
if (repo.Refs.ReachableFrom(remoteHeads, new[] { c }).Any())
102-
{
103-
return c.Sha;
104-
}
105-
}
106-
return null;
107-
});
102+
foreach (var c in repo.Commits)
103+
{
104+
if (repo.Refs.ReachableFrom(remoteHeads, new[] { c }).Any())
105+
{
106+
return c.Sha;
107+
}
108+
}
109+
return null;
110+
});
111+
}
108112
}
109113
}
110114
}

‎src/GitHub.InlineReviews/Services/PullRequestSessionService.cs‎

Lines changed: 44 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,19 @@ public PullRequestSessionService(
5353
/// <inheritdoc/>
5454
public virtual async Task<IReadOnlyList<DiffChunk>> Diff(ILocalRepositoryModel repository, string baseSha, string headSha, string relativePath)
5555
{
56-
var repo = await GetRepository(repository);
57-
return await diffService.Diff(repo, baseSha, headSha, relativePath);
56+
using (var repo = await GetRepository(repository))
57+
{
58+
return await diffService.Diff(repo, baseSha, headSha, relativePath);
59+
}
5860
}
5961

6062
/// <inheritdoc/>
6163
public virtual async Task<IReadOnlyList<DiffChunk>> Diff(ILocalRepositoryModel repository, string baseSha, string headSha, string relativePath, byte[] contents)
6264
{
63-
var repo = await GetRepository(repository);
64-
return await diffService.Diff(repo, baseSha, headSha, relativePath, contents);
65+
using (var repo = await GetRepository(repository))
66+
{
67+
return await diffService.Diff(repo, baseSha, headSha, relativePath, contents);
68+
}
6569
}
6670

6771
/// <inheritdoc/>
@@ -175,18 +179,22 @@ public ITextDocument GetDocument(ITextBuffer buffer)
175179
/// <inheritdoc/>
176180
public virtual async Task<string> GetTipSha(ILocalRepositoryModel repository)
177181
{
178-
var repo = await GetRepository(repository);
179-
return repo.Head.Tip.Sha;
182+
using (var repo = await GetRepository(repository))
183+
{
184+
return repo.Head.Tip.Sha;
185+
}
180186
}
181187

182188
/// <inheritdoc/>
183189
public async Task<bool> IsUnmodifiedAndPushed(ILocalRepositoryModel repository, string relativePath, byte[] contents)
184190
{
185-
var repo = await GetRepository(repository);
186-
var modified = await gitClient.IsModified(repo, relativePath, contents);
187-
var pushed = await gitClient.IsHeadPushed(repo);
191+
using (var repo = await GetRepository(repository))
192+
{
193+
var modified = await gitClient.IsModified(repo, relativePath, contents);
194+
var pushed = await gitClient.IsHeadPushed(repo);
188195

189-
return !modified && pushed;
196+
return !modified && pushed;
197+
}
190198
}
191199

192200
public async Task<byte[]> ExtractFileFromGit(
@@ -195,17 +203,18 @@ public async Task<byte[]> ExtractFileFromGit(
195203
string sha,
196204
string relativePath)
197205
{
198-
var repo = await GetRepository(repository);
199-
200-
try
201-
{
202-
return await gitClient.ExtractFileBinary(repo, sha, relativePath);
203-
}
204-
catch (FileNotFoundException)
206+
using (var repo = await GetRepository(repository))
205207
{
206-
var pullHeadRef = $"refs/pull/{pullRequestNumber}/head";
207-
await gitClient.Fetch(repo, "origin", sha, pullHeadRef);
208-
return await gitClient.ExtractFileBinary(repo, sha, relativePath);
208+
try
209+
{
210+
return await gitClient.ExtractFileBinary(repo, sha, relativePath);
211+
}
212+
catch (FileNotFoundException)
213+
{
214+
var pullHeadRef = $"refs/pull/{pullRequestNumber}/head";
215+
await gitClient.Fetch(repo, "origin", sha, pullHeadRef);
216+
return await gitClient.ExtractFileBinary(repo, sha, relativePath);
217+
}
209218
}
210219
}
211220

@@ -242,21 +251,23 @@ public virtual async Task<string> GetPullRequestMergeBase(ILocalRepositoryModel
242251
return mergeBase;
243252
}
244253

245-
var repo = await GetRepository(repository);
246-
var targetUrl = pullRequest.Base.RepositoryCloneUrl;
247-
var headUrl = pullRequest.Head.RepositoryCloneUrl;
248-
var baseRef = pullRequest.Base.Ref;
249-
var pullNumber = pullRequest.Number;
250-
try
254+
using (var repo = await GetRepository(repository))
251255
{
252-
mergeBase = await gitClient.GetPullRequestMergeBase(repo, targetUrl, baseSha, headSha, baseRef, pullNumber);
253-
}
254-
catch (NotFoundException ex)
255-
{
256-
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 [email protected]", ex);
257-
}
256+
var targetUrl = pullRequest.Base.RepositoryCloneUrl;
257+
var headUrl = pullRequest.Head.RepositoryCloneUrl;
258+
var baseRef = pullRequest.Base.Ref;
259+
var pullNumber = pullRequest.Number;
260+
try
261+
{
262+
mergeBase = await gitClient.GetPullRequestMergeBase(repo, targetUrl, baseSha, headSha, baseRef, pullNumber);
263+
}
264+
catch (NotFoundException ex)
265+
{
266+
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 [email protected]", ex);
267+
}
258268

259-
return mergeBaseCache[key] = mergeBase;
269+
return mergeBaseCache[key] = mergeBase;
270+
}
260271
}
261272

262273
/// <inheritdoc/>

‎src/GitHub.TeamFoundation.14/Services/VSGitServices.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,12 @@ public string GetActiveRepoPath()
116116
if (repo != null)
117117
ret = repo.RepositoryPath;
118118
if (ret == null)
119-
ret = serviceProvider.GetSolution().GetRepositoryFromSolution()?.Info?.Path;
119+
{
120+
using (var repository = serviceProvider.GetSolution().GetRepositoryFromSolution())
121+
{
122+
ret = repository?.Info?.Path;
123+
}
124+
}
120125
return ret ?? String.Empty;
121126
}
122127

‎src/GitHub.VisualStudio/Menus/MenuBase.cs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ protected ISimpleApiClient SimpleApiClient
3939
protected ISimpleApiClientFactory ApiFactory => apiFactory.Value;
4040

4141
protected MenuBase()
42-
{}
42+
{ }
4343

4444
protected MenuBase(IGitHubServiceProvider serviceProvider)
4545
{
@@ -55,8 +55,10 @@ protected ILocalRepositoryModel GetRepositoryByPath(string path)
5555
{
5656
if (!string.IsNullOrEmpty(path))
5757
{
58-
var repo = ServiceProvider.TryGetService<IGitService>().GetRepository(path);
59-
return new LocalRepositoryModel(repo.Info.WorkingDirectory.TrimEnd('\\'));
58+
using (var repo = ServiceProvider.TryGetService<IGitService>().GetRepository(path))
59+
{
60+
return new LocalRepositoryModel(repo.Info.WorkingDirectory.TrimEnd('\\'));
61+
}
6062
}
6163
}
6264
catch (Exception ex)

‎test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ public string AddFile(string path, string contents, string commitAlias)
5454
public void Dispose()
5555
{
5656
var path = repository.Info.WorkingDirectory;
57-
repository.Dispose();
5857

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

0 commit comments

Comments
 (0)