Skip to content

Commit c379b4e

Browse files
tyrielvtyrielv
authored
Merge pull request #2021 from tyrielv/tyrielv/fix-flaky-rebase-pipe
Mount: prevent process crash on unhandled request handler exceptions
2 parents 632186a + ab1ab73 commit c379b4e

2 files changed

Lines changed: 125 additions & 87 deletions

File tree

‎GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitRepoTests.cs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,13 @@ public virtual void SetupForTest()
137137
{
138138
this.CreateEnlistment();
139139
}
140+
else if (!this.Enlistment.IsMounted())
141+
{
142+
Assert.Fail(
143+
"GVFS mount is not running for the shared enlistment. " +
144+
"A previous test likely caused the mount process to crash. " +
145+
"Check earlier test failures for the root cause.");
146+
}
140147

141148
if (this.validateWorkingTree == Settings.ValidateWorkingTreeMode.SparseMode)
142149
{

‎GVFS/GVFS.Mount/InProcessMount.cs‎

Lines changed: 118 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -495,68 +495,79 @@ private void HandleRequest(ITracer tracer, string request, NamedPipeServer.Conne
495495
return;
496496
}
497497

498-
switch (message.Header)
498+
try
499499
{
500-
case NamedPipeMessages.GetStatus.Request:
501-
this.HandleGetStatusRequest(connection);
502-
break;
500+
switch (message.Header)
501+
{
502+
case NamedPipeMessages.GetStatus.Request:
503+
this.HandleGetStatusRequest(connection);
504+
break;
503505

504-
case NamedPipeMessages.Unmount.Request:
505-
this.HandleUnmountRequest(connection);
506-
break;
506+
case NamedPipeMessages.Unmount.Request:
507+
this.HandleUnmountRequest(connection);
508+
break;
507509

508-
case NamedPipeMessages.AcquireLock.AcquireRequest:
509-
this.HandleLockRequest(message.Body, connection);
510-
break;
510+
case NamedPipeMessages.AcquireLock.AcquireRequest:
511+
this.HandleLockRequest(message.Body, connection);
512+
break;
511513

512-
case NamedPipeMessages.ReleaseLock.Request:
513-
this.HandleReleaseLockRequest(message.Body, connection);
514-
break;
514+
case NamedPipeMessages.ReleaseLock.Request:
515+
this.HandleReleaseLockRequest(message.Body, connection);
516+
break;
515517

516-
case NamedPipeMessages.DownloadObject.DownloadRequest:
517-
this.HandleDownloadObjectRequest(message, connection);
518-
break;
518+
case NamedPipeMessages.DownloadObject.DownloadRequest:
519+
this.HandleDownloadObjectRequest(message, connection);
520+
break;
519521

520-
case NamedPipeMessages.ModifiedPaths.ListRequest:
521-
this.HandleModifiedPathsListRequest(message, connection);
522-
break;
522+
case NamedPipeMessages.ModifiedPaths.ListRequest:
523+
this.HandleModifiedPathsListRequest(message, connection);
524+
break;
523525

524-
case NamedPipeMessages.PostIndexChanged.NotificationRequest:
525-
this.HandlePostIndexChangedRequest(message, connection);
526-
break;
526+
case NamedPipeMessages.PostIndexChanged.NotificationRequest:
527+
this.HandlePostIndexChangedRequest(message, connection);
528+
break;
527529

528-
case NamedPipeMessages.PrepareForUnstage.Request:
529-
this.HandlePrepareForUnstageRequest(message, connection);
530-
break;
530+
case NamedPipeMessages.PrepareForUnstage.Request:
531+
this.HandlePrepareForUnstageRequest(message, connection);
532+
break;
531533

532-
case NamedPipeMessages.RunPostFetchJob.PostFetchJob:
533-
this.HandlePostFetchJobRequest(message, connection);
534-
break;
534+
case NamedPipeMessages.RunPostFetchJob.PostFetchJob:
535+
this.HandlePostFetchJobRequest(message, connection);
536+
break;
535537

536-
case NamedPipeMessages.DehydrateFolders.Dehydrate:
537-
this.HandleDehydrateFolders(message, connection);
538-
break;
538+
case NamedPipeMessages.DehydrateFolders.Dehydrate:
539+
this.HandleDehydrateFolders(message, connection);
540+
break;
539541

540-
case NamedPipeMessages.PrefetchCommits.Request:
541-
this.HandlePrefetchCommitsRequest(connection);
542-
break;
542+
case NamedPipeMessages.PrefetchCommits.Request:
543+
this.HandlePrefetchCommitsRequest(connection);
544+
break;
543545

544-
case NamedPipeMessages.PrefetchBlobs.RequestHeader:
545-
this.HandlePrefetchBlobsRequest(message, connection);
546-
break;
546+
case NamedPipeMessages.PrefetchBlobs.RequestHeader:
547+
this.HandlePrefetchBlobsRequest(message, connection);
548+
break;
547549

548-
case NamedPipeMessages.HydrationStatus.Request:
549-
this.HandleGetHydrationStatusRequest(connection);
550-
break;
550+
case NamedPipeMessages.HydrationStatus.Request:
551+
this.HandleGetHydrationStatusRequest(connection);
552+
break;
551553

552-
default:
553-
EventMetadata metadata = new EventMetadata();
554-
metadata.Add("Area", "Mount");
555-
metadata.Add("Header", message.Header);
556-
this.tracer.RelatedError(metadata, "HandleRequest: Unknown request");
554+
default:
555+
EventMetadata metadata = new EventMetadata();
556+
metadata.Add("Area", "Mount");
557+
metadata.Add("Header", message.Header);
558+
this.tracer.RelatedError(metadata, "HandleRequest: Unknown request");
557559

558-
connection.TrySendResponse(NamedPipeMessages.UnknownRequest);
559-
break;
560+
connection.TrySendResponse(NamedPipeMessages.UnknownRequest);
561+
break;
562+
}
563+
}
564+
catch (Exception e) when (e is not OutOfMemoryException)
565+
{
566+
EventMetadata metadata = new EventMetadata();
567+
metadata.Add("Area", "Mount");
568+
metadata.Add("Header", message.Header);
569+
metadata.Add("Exception", e.ToString());
570+
this.tracer.RelatedError(metadata, "HandleRequest: Unhandled exception in request handler");
560571
}
561572
}
562573

@@ -901,56 +912,76 @@ private void HandleDownloadObjectRequest(NamedPipeMessages.Message message, Name
901912
}
902913
else
903914
{
904-
Stopwatch downloadTime = Stopwatch.StartNew();
905-
906-
/* If this is the root tree for a commit that was was just downloaded, assume that more
907-
* trees will be needed soon and download them as well by using the download commit API.
908-
*
909-
* Otherwise, or as a fallback if the commit download fails, download the object directly.
910-
*/
911-
if (this.ShouldDownloadCommitPack(objectSha, out string commitSha)
912-
&& this.gitObjects.TryDownloadCommit(commitSha))
913-
{
914-
this.DownloadedCommitPack(commitSha);
915-
response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.SuccessResult);
916-
// FUTURE: Should the stats be updated to reflect all the trees in the pack?
917-
// FUTURE: Should we try to clean up duplicate trees or increase depth of the commit download?
918-
}
919-
else if (this.gitObjects.TryDownloadAndSaveObject(objectSha, GVFSGitObjects.RequestSource.NamedPipeMessage) == GitObjects.DownloadAndSaveObjectResult.Success)
915+
try
920916
{
921-
this.UpdateTreesForDownloadedCommits(objectSha);
922-
response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.SuccessResult);
917+
response = this.DownloadObject(objectSha);
923918
}
924-
else
919+
catch (Exception e) when (e is not OutOfMemoryException)
925920
{
921+
EventMetadata metadata = new EventMetadata();
922+
metadata.Add("Area", "Mount");
923+
metadata.Add("objectSha", objectSha);
924+
metadata.Add("Exception", e.ToString());
925+
this.tracer.RelatedWarning(metadata, nameof(this.HandleDownloadObjectRequest) + ": Exception downloading object");
926+
926927
response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.DownloadFailed);
927928
}
929+
}
930+
}
928931

932+
connection.TrySendResponse(response.CreateMessage());
933+
}
929934

930-
Native.ObjectTypes? objectType;
931-
this.context.Repository.TryGetObjectType(objectSha, out objectType);
932-
this.context.Repository.GVFSLock.Stats.RecordObjectDownload(objectType == Native.ObjectTypes.Blob, downloadTime.ElapsedMilliseconds);
935+
private NamedPipeMessages.DownloadObject.Response DownloadObject(string objectSha)
936+
{
937+
NamedPipeMessages.DownloadObject.Response response;
938+
Stopwatch downloadTime = Stopwatch.StartNew();
933939

934-
if (objectType == Native.ObjectTypes.Commit
935-
&& !this.context.Repository.CommitAndRootTreeExists(objectSha, out var treeSha)
936-
&& !string.IsNullOrEmpty(treeSha))
937-
{
938-
/* If a commit is downloaded, it wasn't prefetched.
939-
* The trees for the commit may be needed soon depending on the context.
940-
* e.g. git log (without a pathspec) doesn't need trees, but git checkout does.
941-
*
942-
* If any prefetch has been done there is probably a similar commit/tree in the graph,
943-
* but in case there isn't (such as if the cache server repack maintenance job is failing)
944-
* we should still try to avoid downloading an excessive number of loose trees for a commit.
945-
*
946-
* Save the tree/commit so if more trees are requested we can download all the trees for the commit in a batch.
947-
*/
948-
this.missingTreeTracker.AddMissingRootTree(treeSha: treeSha, commitSha: objectSha);
949-
}
950-
}
940+
/* If this is the root tree for a commit that was was just downloaded, assume that more
941+
* trees will be needed soon and download them as well by using the download commit API.
942+
*
943+
* Otherwise, or as a fallback if the commit download fails, download the object directly.
944+
*/
945+
if (this.ShouldDownloadCommitPack(objectSha, out string commitSha)
946+
&& this.gitObjects.TryDownloadCommit(commitSha))
947+
{
948+
this.DownloadedCommitPack(commitSha);
949+
response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.SuccessResult);
950+
// FUTURE: Should the stats be updated to reflect all the trees in the pack?
951+
// FUTURE: Should we try to clean up duplicate trees or increase depth of the commit download?
952+
}
953+
else if (this.gitObjects.TryDownloadAndSaveObject(objectSha, GVFSGitObjects.RequestSource.NamedPipeMessage) == GitObjects.DownloadAndSaveObjectResult.Success)
954+
{
955+
this.UpdateTreesForDownloadedCommits(objectSha);
956+
response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.SuccessResult);
957+
}
958+
else
959+
{
960+
response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.DownloadFailed);
951961
}
952962

953-
connection.TrySendResponse(response.CreateMessage());
963+
Native.ObjectTypes? objectType;
964+
this.context.Repository.TryGetObjectType(objectSha, out objectType);
965+
this.context.Repository.GVFSLock.Stats.RecordObjectDownload(objectType == Native.ObjectTypes.Blob, downloadTime.ElapsedMilliseconds);
966+
967+
if (objectType == Native.ObjectTypes.Commit
968+
&& !this.context.Repository.CommitAndRootTreeExists(objectSha, out var treeSha)
969+
&& !string.IsNullOrEmpty(treeSha))
970+
{
971+
/* If a commit is downloaded, it wasn't prefetched.
972+
* The trees for the commit may be needed soon depending on the context.
973+
* e.g. git log (without a pathspec) doesn't need trees, but git checkout does.
974+
*
975+
* If any prefetch has been done there is probably a similar commit/tree in the graph,
976+
* but in case there isn't (such as if the cache server repack maintenance job is failing)
977+
* we should still try to avoid downloading an excessive number of loose trees for a commit.
978+
*
979+
* Save the tree/commit so if more trees are requested we can download all the trees for the commit in a batch.
980+
*/
981+
this.missingTreeTracker.AddMissingRootTree(treeSha: treeSha, commitSha: objectSha);
982+
}
983+
984+
return response;
954985
}
955986

956987
private bool ShouldDownloadCommitPack(string objectSha, out string commitSha)

0 commit comments

Comments
 (0)