RE: svn commit: r1617926 -/subversion/trunk/subversion/libsvn_subr/io.c

I don't think a macro is the right thing to do here. The win32 retry macro already makes it too easy to reuse the existing but bad checks everywhere, causing more unneeded 12 second delays. Only specific errors should be retried in specific cases. Just retrying everything doesn't resolve the real problems. Only on some file lock cases this really makes sense.

Several months ago somebody suggested to just add such a retry around the recursive make directory...
... Great solution to a race condition in the Apr implementation which we eventually found.
(One that applied to multiple platforms if I remember correctly)

I was thinking about completely removing the retry from these mkdir cases, but I'm not completely convinced that there can never be relevant sharing violations.

In the far past many of these loops were just added as quick fixes by non Windows developers to 'fix some user reported Windows problems' which were not completely investigated.

Bert

-----Original Message-----
From: "Branko Čibej" <brane_at_wandisco.com>
Sent: ‎14/‎08/‎2014 20:19
To: "dev_at_subversion.apache.org" <dev_at_subversion.apache.org>
Subject: Re: svn commit: r1617926 -/subversion/trunk/subversion/libsvn_subr/io.c

On 14.08.2014 14:55, rhuijben_at_apache.org wrote:

Author: rhuijben
Date: Thu Aug 14 12:55:02 2014
New Revision: 1617926

URL: http://svn.apache.org/r1617926
Log:
Resolve a 12 second delay on every 'svn' invocation from Windows system
services that don't have access to a writable user profile.
(Try running a svn script from the task scheduler on Windows 2012 R2)

Without this fix we retry creating the %APPDATA%\Subversion directory again
and again, and then continue silently.

The retry loop prevents erroring out from virusscanners and indexers that keep
file handles open, but this can't happen in the create directory case as we
would have waited in the delete preceding the create in relevant cases.

(Found while rebuilding the Windows buildbot last week)

* subversion/libsvn_subr/io.c
  (svn_io_make_dir_recursively,
   dir_make): Don't retry for 12 seconds when creating a directory fails,
    because we are using a directory with an ACL that denies creating
    directories.

Modified:
    subversion/trunk/subversion/libsvn_subr/io.c

Modified: subversion/trunk/subversion/libsvn_subr/io.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1617926&r1=1617925&r2=1617926&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/io.c (original)
+++ subversion/trunk/subversion/libsvn_subr/io.c Thu Aug 14 12:55:02 2014
@@ -1154,8 +1154,13 @@ svn_io_make_dir_recursively(const char *
   SVN_ERR(cstring_from_utf8(&path_apr, path, pool));
 
   apr_err = apr_dir_make_recursive(path_apr, APR_OS_DEFAULT, pool);
- WIN32_RETRY_LOOP(apr_err, apr_dir_make_recursive(path_apr,
- APR_OS_DEFAULT, pool));
+#ifdef WIN32
+ /* Don't retry on ERROR_ACCESS_DENIED, as that typically signals a
+ permanent error */
+ if (apr_err == APR_FROM_OS_ERROR(ERROR_SHARING_VIOLATION))
+ WIN32_RETRY_LOOP(apr_err, apr_dir_make_recursive(path_apr,
+ APR_OS_DEFAULT, pool));
+#endif
Bert, would it not make sense to wrap this idiom into a Windows-specific retry macro, if only to avoid writing the same code twice in this file (and avoid the #ifdefs here, too)?

-- Brane

-- 
Branko Čibej | Director of Subversion 
WANdisco | Realising the impossibilities of Big Data 
e. brane_at_wandisco.com