Skip to content

Mac: Fix missing files due to auth cache#270

Merged
sanoursa merged 2 commits intomicrosoft:masterfrom
sanoursa:authTTL
Sep 20, 2018
Merged

Mac: Fix missing files due to auth cache#270
sanoursa merged 2 commits intomicrosoft:masterfrom
sanoursa:authTTL

Conversation

@sanoursa
Copy link
Contributor

@sanoursa sanoursa commented Sep 13, 2018

The first commit re-enables all the tests that were hitting bugs with auth cache poison issues. The second commit fixes those issues by setting the auth cache TTL to 0.

With this fix, we no longer hit any missing file issues. Initial perf testing also demonstrates that there is very minimal overhead on non-virtualized paths, i.e. anything outside of a VFS for Git repo. For paths inside of a virtualization root, there is a bigger perf hit, but that is because our kext is now getting called on every kauth request, and any of our own perf issues are getting magnified. We will do more work to analyze that overhead and optimize it, but for now this fix makes our kext much more reliable.

Fixes #249

@sanoursa sanoursa added the WIP label Sep 13, 2018
@sanoursa
Copy link
Contributor Author

sanoursa commented Sep 13, 2018

Confirmed at least one test failure without a fix.

1) Failed : GVFS.FunctionalTests.Tests.EnlistmentPerFixture.MultithreadedReadWriteTests.CanReadVirtualFileInParallel()
  At least one of the reads failed
  Expected: null
  But was:  <System.IO.FileNotFoundException: Could not find file '/GVFS.FT/test/72a37bd229894efcb1ef/src/GVFS/GVFS.FunctionalTests/Tests/LongRunningEnlistment/GitMoveRenameTests.cs'.

The rest of the tests pass both on my machine and on the server, but @wilbaker has confirmed that they fail on his machine, so we'll confirm the fix there once I push it up.

https://dev.azure.com/mseng/VSOnline/_build/results?buildId=7238862&view=logs

@sanoursa
Copy link
Contributor Author

sanoursa commented Sep 13, 2018

With the fix, the multithreaded read test now passes on the server:

=> GVFS.FunctionalTests.Tests.EnlistmentPerFixture.MultithreadedReadWriteTests.CanReadVirtualFileInParallel()
Test GVFS.FunctionalTests.Tests.EnlistmentPerFixture.MultithreadedReadWriteTests.CanReadVirtualFileInParallel()
Passed at 10:38:19 PM taking 00:00:00.8785380

https://dev.azure.com/mseng/VSOnline/_build/results?buildId=7241672&view=logs

@wilbaker
Copy link
Member

I ran all of the CheckoutTests twice on my machine, and all passed each time.

Exception readException = null;

Thread[] threads = new Thread[32];
Thread[] threads = new Thread[128];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we upping the number of threads here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally anecdotal - but I've just seen on my own machine that the higher number of threads makes any races more likely to get hit. And the more temporary/partial fixes I put in place in my other branch, the more threads I needed to reliably hit any remaining issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks.

Exception readException = null;

Thread[] threads = new Thread[32];
Thread[] threads = new Thread[128];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks.

Copy link
Member

@nickgra nickgra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a delightful PR to see after coming back from vacation :)

vnode_put(virtualizationRootVNode);
}

if (rootIndex >= 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we reset the auth cache TTL back to the default upon virtualization root unmount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we will certainly want to do that, but right now unmount doesn't call through to the kext at all so we're currently not doing any cleanup. I filed #284 to cover all of that.

@sanoursa sanoursa removed the WIP label Sep 20, 2018
@sanoursa sanoursa merged commit f2e933a into microsoft:master Sep 20, 2018
@sanoursa sanoursa deleted the authTTL branch September 20, 2018 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mac: stat reports "No such file or directory" if a file\folder's parent has not yet been enumerated

4 participants