Prevent Repository objects from being prematurely GCed #1337

jcansdale · 2017-11-23T16:57:49Z

The Bug

The following code can fail on the repo.RetrieveStatus() line with an AccessViolationException:

var repo = new Repository(path);
var status = repo.RetrieveStatus();

This is because RetrieveStatus creates a native pointer to the Repository object and but doesn't hold on to a managed reference. This allows the Repository object to be GCed before RetrieveStatus has finished with the native pointer.

When does an object become available for garbage collection?https://blogs.msdn.microsoft.com/oldnewthing/20100810-00/?p=13193/

To vaoid random crashes, we need to be careful to keep a reference to Repository when calling its methods. The above code can be fixed like this:

var repo = new Repository(path);
var status = repo.RetrieveStatus();
GC.KeepAlive(repo);

Alternatively since the Repository object implements IDisposable, the following will keep a reference and dispose of it afterwards:

using (var repo = new Repository(path))
{
    var status = repo.RetrieveStatus();
}

This PR wraps all temporary references to a Repository object in a using statement.

Possible Gotchas

This bug only shows up when the extension is compiled in Release configuration (in Debug configuration, references are held on to for longer and this bug doesn't occur).
We need to make sure we don't return objects that keep references to disposed Repository objects. For example here, we need to make sure that CommitMessage doesn't cache a reference to the disposed Repository object:

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

Line 101 in 7fb09a3

return gitClient.GetMessagesForUniqueCommits(repo, baseBranch, compareBranch, maxCommits).ToObservable();

Fixes #1315
See also: libgit2/libgit2sharp#1513

By wrapping the Repository object in a using statement we prevent it from being being GCed when there are only native pointers to it.

Use with a using statement.

Dispose of Repository object.

jcansdale · 2017-11-24T11:51:46Z

I got the following when trying to create a new PR:

 	LibGit2Sharp.dll!LibGit2Sharp.Core.Proxy.git_revparse_ext(LibGit2Sharp.Core.Handles.RepositoryHandle repo, string objectish)	Unknown
 	LibGit2Sharp.dll!LibGit2Sharp.Core.Proxy.git_revparse_single(LibGit2Sharp.Core.Handles.RepositoryHandle repo, string objectish)	Unknown
 	LibGit2Sharp.dll!LibGit2Sharp.Repository.Lookup(string objectish, LibGit2Sharp.Core.GitObjectType type, LibGit2Sharp.Core.LookUpOptions lookUpOptions)	Unknown
 	LibGit2Sharp.dll!LibGit2Sharp.Repository.Lookup(string objectish, LibGit2Sharp.ObjectType type)	Unknown
 	LibGit2Sharp.dll!LibGit2Sharp.RepositoryExtensions.Lookup<LibGit2Sharp.Commit>(LibGit2Sharp.IRepository repository, string objectish)	Unknown
>	GitHub.App.dll!GitHub.Services.GitClient.GetMessagesForUniqueCommits.AnonymousMethod__0() Line 486	C#

jcansdale · 2017-11-24T12:41:20Z

src/GitHub.App/Services/PullRequestService.cs

+                // CommitMessage doesn't keep a reference to Repository
+                using (var repo = gitService.GetRepository(repository.LocalPath))
+                {
+                    return gitClient.GetMessagesForUniqueCommits(repo, baseBranch, compareBranch, maxCommits).ToObservable();


ToObservable / threading issue.

jcansdale · 2017-11-24T13:00:02Z

src/GitHub.App/Services/PullRequestService.cs

+                using (var repo = gitService.GetRepository(repository.LocalPath))
+                {
+                    var result = GetLocalBranchesInternal(repository, repo, pullRequest).Select(x => new BranchModel(x, repository));
+                    return result.ToObservable();


@grokys I have a bad feeling about this one. What do you think?

Yup, as expected!

Hmm yeah, should probably call ToList() on result here.

For reference, the other option is to destroy the repo with a Using operator, I don't think that'd be necessary here however.

We don't want to dispose of Repository while still enumerating its branches.

jcansdale · 2017-11-24T15:00:26Z

Hmmm.

jcansdale · 2017-11-24T15:42:41Z

I'm seeing an issue where after clicking the checkout link, the checkout appears to succeed but the UI doesn't update. Any idea what might be causing that?

grokys · 2017-11-24T15:56:08Z

I'm seeing an issue where after clicking the checkout link, the checkout appears to succeed but the UI doesn't update. Any idea what might be causing that?

Not seen that before. Does it happen on master?

jcansdale · 2017-11-24T15:57:42Z

@grokys I thought this was likely an issue introduced with this PR. Haven't seen it on master before.

Update, this is also happening on master. 😕

grokys

👍 LGTM! Just a small nit about a commented-out line.

grokys · 2017-11-27T15:06:21Z

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

-            repository.Dispose();
+
+            // NOTE: IDiffService doesn't own the Repository object
+            //repository.Dispose();


I'd rather remove this commented-out line.

jcansdale added 5 commits November 23, 2017 14:58

Keep alive reference to Repository and Dispose

e1b5f6a

By wrapping the Repository object in a using statement we prevent it from being being GCed when there are only native pointers to it.

Test GitService.GetRepository in a consistent way

7cfbda3

Use with a using statement.

Dispose after using PullRequestSessionService.GetRepository

6f1a890

Use IRepository returned by GetActiveRepo inside using

8b17782

Access GetRepositoryFromSolution from inside using

604a82c

Dispose of Repository object.

jcansdale changed the title ~~Prevent Repository objects from being prematurely GCed~~ [WIP] Prevent Repository objects from being prematurely GCed Nov 23, 2017

jcansdale added 2 commits November 23, 2017 17:33

Check we don't return reference to disposed Repository

c1746ae

Keep reference to TreeChanges and Dispose

7fb09a3

jcansdale changed the title ~~[WIP] Prevent Repository objects from being prematurely GCed~~ Prevent Repository objects from being prematurely GCed Nov 24, 2017

jcansdale requested review from grokys and meaghanlewis November 24, 2017 10:07

jcansdale commented Nov 24, 2017

View reviewed changes

< 8000 div class="AvatarStack flex-self-start " >

Don't dispose of Repository before it's used

4db51b2

jcansdale force-pushed the fixes/1315-using-repository branch from 08afc5c to 4db51b2 Compare November 24, 2017 12:56

jcansdale commented Nov 24, 2017

View reviewed changes

Force the enumeration to complete before disposing

88a753a

We don't want to dispose of Repository while still enumerating its branches.

grokys previously approved these changes Nov 27, 2017

View reviewed changes

Clean up dead code

b923899

jcansdale dismissed grokys’s stale review via b923899 November 27, 2017 17:54

Merge branch 'master' into fixes/1315-using-repository

c61a75d

grokys approved these changes Nov 28, 2017

View reviewed changes

jcansdale merged commit 9002364 into master Nov 28, 2017

jcansdale deleted the fixes/1315-using-repository branch November 28, 2017 10:25
9153

jcansdale mentioned this pull request Nov 28, 2017

Don't dispose of repo before task completes #1351

Merged

This was referenced Dec 5, 2017

Error and hang/crash when opening PR detail #1293

Closed

PR Details: Changed files list doesn't display deleted files correctly #1347

Closed

jbestrom mentioned this pull request Dec 12, 2018

LibGit2Sharp upgrade??? jzoss/Git-Source-Control-Provider#110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent Repository objects from being prematurely GCed #1337

Prevent Repository objects from being prematurely GCed #1337

Prevent Repository objects from being prematurely GCed #1337

Prevent Repository objects from being prematurely GCed #1337

Conversation

The Bug

Possible Gotchas

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment