Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add retries to win32 p_unlink and p_open.#3790

Merged
carlosmn merged 1 commit intolibgit2:masterlibgit2/libgit2:masterfrom
tylerchurch:open-and-unlink-retriestylerchurch/libgit2:open-and-unlink-retriesCopy head branch name to clipboard
Apr 17, 2017
Merged

Add retries to win32 p_unlink and p_open.#3790
carlosmn merged 1 commit intolibgit2:masterlibgit2/libgit2:masterfrom
tylerchurch:open-and-unlink-retriestylerchurch/libgit2:open-and-unlink-retriesCopy head branch name to clipboard

Conversation

@tylerchurch
Copy link

This pull requests follows the pattern used in this previous pull request: #2211 to add retries to p_unlink and p_open.

When doing checkouts I was seeing Dropbox and/or anti-virus locking files while libgit2 was trying to call p_unlink or p_open. These changes seem to make checkout much more reliable for me on Windows.

break;
}

if (errno == EACCES) {
Copy link
Member

Choose a reason for hiding this comment

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

When will this be reached? MSDN says that errno is set to EACCES when _wopen returns -1, but the condition above would have caught that case.

Copy link
Author

Choose a reason for hiding this comment

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

My interpretation of the docs here: https://msdn.microsoft.com/en-us/library/z0kc8e3z.aspx was that it could be EACCES, EEXIST, EINVAL, EMFILE, or ENOENT. EACCES was the only one I wanted to retry on, but if you'd like I can kill the conditional and we can just retry no matter the errno.

@ethomson
Copy link
Member

I think use of gotos may simplify this code a bit. However, (and keep in mind that I use this phrase quite sparingly) a macro might be a decent solution here, too, since this seems like a common pattern in the Windows stuff.

I seem to remember that we do this retry dance elsewhere in the win32 compat layer. It would be nice to unify the style amongst them (give them a consistent timeout, with a #defined constant, etc).

Thanks for starting this, I think it will benefit win32 users quite a bit.

@tylerchurch
Copy link
Author

Yeah, a macro would be nice, though so far the usages of the different functions seem just slightly different enough that I'm not sure how useful it would actually be (e.g. p_open needs to return a handle, but p_unlink doesn't.) If you've got an idea for a macro you think would work though, I'm happy to integrate it.

I'm with you on adding some global timeout constants. My preference would be for there to be some library wide setting that could be changed by whatever application is loading libgit2, rather than a #defined constant. Are there any other similar settings defined anywhere? If you could point me in the right direction, I can take a look at adding it.

@ethomson
Copy link
Member

I'm not tied to this needing a nice configuration. I think that it's fine to #define these - I don't have any notion that this is something that somebody wants to tune. But if you would benefit from the ability to change it then 👍 .

Our globals are set in https://github.com/libgit2/libgit2/blob/master/src/global.c - let me know if you have questions about the patterns in there.

@carlosmn carlosmn merged commit 32269b1 into libgit2:master Apr 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.