Retry committing locked files on error - #2211

Thanks for your comments! I moved the code into p_rename and added an error code check.

The PR itself looks correct. Now I just want @ethomson or @phkelley to tell me whether this is a good idea at all.

I think it's a good idea, for sure. Windows operates in this disappointing place where files are implicitly locked and if not, then they change in the middle of you reading them. This sort of technique is a requirement for many scenarios.

Out of curiosity, how was the 50ms figure arrived at?

I want a sanity-check from @phkelley as well, when he has a moment, since he knows better than I about all things Win32.

Out of curiosity, how was the 50ms figure arrived at?

It's the first number I picked. As I wrote above, I saw up to three Access Denied errors in a row, which equals to 15-20 ms. 50ms gives a safety margin. But I don't know about other systems.

It's the first number I picked.

Legit. My gut says that we might want to wait a little longer.

This is a common pattern that we use, let me see if I can find some of our numbers. Some historic data might be interesting since some of them probably predate SSDs and other fast type hardware.

This seems like a good idea to me. I wonder if we should add a GIT_OPT_SET_BUSY_RETRIES option for git_libgit2_opts that allows you to globally set an amount of time to wait and possibly even a number of retries to make. We can default to 1 retry after 50ms or 2 retries after 25ms, but a conservative app or an app with knowledge of the hardware and software environment could override as needed. Just a thought...

BTW, sorry, I just reread the patch and realize this is currently doing 10 tries at 5ms each.

I agree that we should make the retry time configurable, so that users can balance performance vs. reliability. But I think exposing two options, one for time and one for step count is too complicated. What really matters is the total maximum wait time. This should be divided into reasonable time steps to enhance performance. And we need to provide a reasonable default maximum wait time. @ethomson Did you find out anything?

This function (p_rename) renames a file from source to target, overwriting the target if it already exists. Most spurious file locking problems on Windows arise because when you call DeleteFile, it merely marks a file for deletion when the last handle is closed. The caller often has no way to synchronize with antivirus software, for example, which may be running in the background. I haven't looked closely at how MoveFileEx works when specifying the MOVEFILE_REPLACE_EXISTING as we are here. But my intuition is that we ought to do this work ourselves as opposed to specifying this flag and letting Windows do the concatenation of these two operations. My ideal function would look like this:

1. Call MoveFileEx, optimistically assuming that the target path is clear. (Provide the flag that says copying the file is allowed.) If it works, we're done.
2. If we get an error from MoveFileEx indicating that the target path already exists, call DeleteFile to clear the target path, optimistically assuming that the target path is not read-only.
3. If DeleteFile indicates that the target path is read-only, clear the read-only bit with SetFileAttributes and call DeleteFile again.
4. Synchronize with the deletion by spinning, checking to see if the target path is clear. Use GetFileAttributes for this, and Sleep. There should be a maximum amount of time to sleep, of course.
5. Once the target path is clear, call MoveFileEx again to move the file from source to target.

(I would prefer to not retry errors from MoveFileEx that don't have to do with clearing the target path. That is to say, anything having to do with the source.)

I wrote an implementation of this mechanism for clar at one point. It was an important part of increasing our test reliability on Win32. (The tests would fail almost 100% of the time before this on XP, for example.) You can find it at: https://github.com/vmg/clar/blob/master/clar/fs.h The key functions to look at are fs_rm and fs_rm_wait.

I remember at one point rewriting p_rename to do the same thing as the clar implementation, but I never got time to finish it and push it. Sorry about that.

If I understand your proposal, it would lead to a situation where we would delete a file before putting the new one in place.

This would go against a pretty basic assumption we make. Namely that a rename will atomically replace the target file. Otherwise, we end up in a situation, where we've e.g. deleted HEAD but not put in its replacement yet, leading to some time (albeit short) where we've made the repository invalid; or made a branch disappear.

[...] it would lead to a situation where we would delete a file before putting the new one in place.

Maybe I got it wrong, but isn't that exactly what we're doing now?

In git_filebuf_commit:

p_unlink(file->path_original);
if (p_rename(file->path_lock, file->path_original) < 0) {
// ...

I don't want to end this discussion, but I think we should merge this PR as it is. It seems like it is a definite improvement to the existing behavior of the library.

Obviously there are further improvements to configurability or stable behavior that can be achieved, and the discussion can continue here even after a merge and any outcomes can be addressed in a separate PR.

Unless I hear someone saying a loud NO, I'm going to merge this later today.

Gonna apply a couple style changes manually.