Re: [PATCH] Updating exclusively opened files locks working copy

["D.J. Heap" <dj...@gmail.com>]

On 7/30/06, Philip Martin <ph...@codematters.co.uk> wrote:
> Eric Hanchrow <of...@blarg.net> writes:
>
> > I know precisely nothing about APR programming, so forgive me if this
> > is stupid, but: doesn't this leak a file handle?
> >
> >     +  /* Make sure the file is not exclusively opened by someone else. */
> >     +  if (!adding && entry && entry->schedule != svn_wc_schedule_delete)
> >     +  {
> >     +    apr_file_t* file;
> >     +    SVN_ERR(svn_io_file_open(&file, fb->path, APR_READ, APR_OS_DEFAULT,
> >     +                             subpool));
> >     +  }
>
> The file will be closed when the subpool is destroyed and that happens
> immediately after this code.  Once the file has been closed it might
> then get locked and so the problem has not been eliminated but simply
> reduced.  It's a bit like doing stat() before open() to check whether
> the file exists, it's not a good solution since open() still has to
> handle errors.
>
> In this case it would be better to improve the error handling that is
> going wrong, although the original report is short on details so it's
> hard to know what to suggest and log handling was mentioned which can
> make things tricky.  If it turns out that the patch is the best way to
> do things, then it needs a much better comment explaining what is
> going on.  One further disadvantage of the patch is that the code for
> the "solution" is a long way from the code that causes the "problem"
> and there is no regression test.
>
> --
> Philip Martin
>


Yes, there may be a better way to handle the problem, but I'm not
terribly familiar with the working copy and log running code.  And
that is correct -- this is just attempting to detect the situation
where other applications already have files open and locked that we
need to update, preventing us from updating them.  In the distant
past, failed updates like this didn't cause the working copy to end up
locked -- but now it does.

As you said, if an application were to grab the file between this
check and the log running code, it would still fail -- but what
happens 95% of the time (in my experience) is that people have an
application (editor of some kind, especially with binary files)
running that is holding the file locked when they run update.  The
update fails, they remember they need to close the editor/application,
but now their working copy is locked and they have to cleanup and then
update again.

In the current implementation, when the update editor is running, it
doesn't actually do much with the real file (just a stat, I think)
until close_dir time when it is processing the log file and tries to
really update the file.  It fails at this point, and I didn't see any
way to gracefully recover -- all error handling just seems to leave
the directory locked and force a 'cleanup' run later.  Is there
something better we could do at that point?

Even this patch should probably really be tweaked to open the file in
read-write mode (to be a truer check) and that would require setting
the file to read-write -- but we need to do that later, anyway, so I
guess that's not too much of an issue.

I'd be happy to handle it better in the log running code, though, if
we can.  Or maybe update should just run cleanup automatically or
something?

DJ

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Updating exclusively opened files locks working copy

[Philip Martin <ph...@codematters.co.uk>]

"D.J. Heap" <dj...@gmail.com> writes:

> Could you elaborate on this a bit more?

It might be possible to do all the necessary operations on the working
file while creating the log file, generating log file commands as
necessary, thus creating a log file that does not need to access the
working file in the way that fails.  In that case any exclusive file
lock would not cause an error while running the log file.  I don't
know whether such an approach is feasible.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Updating exclusively opened files locks working copy

["D.J. Heap" <dj...@gmail.com>]

On 8/1/06, Philip Martin <ph...@codematters.co.uk> wrote:
[snip]
> >
> > I'd be happy to handle it better in the log running code, though, if
> > we can.  Or maybe update should just run cleanup automatically or
> > something?
>
> The whole point of cleanup is that it's not supposed to be run
> automatically, something has gone wrong that requires user
> intervention.  In this case running cleanup automatically would surely
> hit the same file locked problem?


Well, presumably the update would fail the first time and they would
get the clue and shut down the offending application.  Then they
update again and cleanup would get automatically run without the user
manually doing it.  But I agree that that is not a great solution --
just something I threw out (I think I saw it proposed by someone else
a while ago, actually, but I'm not sure).


>
> Your solution might be the best way to do it, although if you care
> about the behaviour you should write a regression test as it's easy to
> break corner cases like this.


Yes, I will do that.


>
> Alternatively perhaps you could extend the log file processing so that
> it does all the file handling while writing the log file, then it will
> fail before running the log file.
>


Could you elaborate on this a bit more?  Do you mean open and hang
onto the files in add_or_open_file (or around there) and then process
them using the already opened objects (rather than re-opening) in
close_directory/run_log?  Wouldn't that cause unacceptable memory
usage (or open file limit) problems with large directories?  It would
be perfect from the standpoint of solving this type of problem,
though...

Or were you thinking of something else?

DJ

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Updating exclusively opened files locks working copy

[Jared Hardy <ja...@gmail.com>]

On 8/1/06, D.J. Heap <dj...@gmail.com> wrote:
> In what circumstances is your tool used?  During a normal user driven
> update or during an automatic process of some kind?

Both. It's usually faster than normal updates, and it detects
immediately if the WC and HEAD are equal, and stops right away, rather
than going through the directory-walk motions. People usually do a
regular full WC update in the morning and during lunch break, a
scheduled update late at night, and do a quick log-list-only update
every other time. It has become our default update method.

> Are you saying you never want the update to stop -- just skip
> problematic files?  That seems like a departure from Subversions
> current 'normal' behavior...

I realize that. I guess we don't usually care about that "normal"
behavior in OS-local-lock situations, because they usually arise when
the locked file is being viewed or edited. When the user realizes
this, they either choose to continue what they're doing and update
that part later,   or close it and update immediately. In either case,
the subsequent update is always a little faster, so they're happy.
    I agree underlying Subversion API should probably just prevent all
updates until all local-lock issues are fixed, for better safety. I
guess, in this one case, our office isn't as concerned about this kind
of atomic version safety.

:) Jared

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Updating exclusively opened files locks working copy

["D.J. Heap" <dj...@gmail.com>]

On 8/1/06, Jared Hardy <ja...@gmail.com> wrote:
[snip]
> Also, files and parent directories that fail to update wouldn't have
> their WC version number changed, and thus continue to reflect
> recursive version status correctly. Prevention here is probably more
> efficient than trying to correct status data afterwards.
>


In what circumstances is your tool used?  During a normal user driven
update or during an automatic process of some kind?

Are you saying you never want the update to stop -- just skip
problematic files?  That seems like a departure from Subversions
current 'normal' behavior...

DJ

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Updating exclusively opened files locks working copy

[Jared Hardy <ja...@gmail.com>]

> "D.J. Heap" <dj...@gmail.com> writes:
>
> > Yes, there may be a better way to handle the problem, but I'm not
> > terribly familiar with the working copy and log running code.  And
> > that is correct -- this is just attempting to detect the situation
> > where other applications already have files open and locked that we
> > need to update, preventing us from updating them.  In the distant
> > past, failed updates like this didn't cause the working copy to end up
> > locked -- but now it does.

Pardon me for butting in -- I don't understand everything being said
here, because I'm not familiar with the code involved, so this may
have already been suggested...
     I have a wrapper program that goes through the "svn log -v"
output for any path, between revisions WC+1 and HEAD, and gets the
list of updated sub-paths. It then goes through all these paths
locally, renames them, updates only "unversioned" local files, renames
them back, and then updates the rest. Files that fail to rename are
marked as probably having some local-lock issue, and get skipped at
update. It then updates the parent directories, but only iff those
directories dont have any local-lock issues underneath them. This
prevents the need for cleanup, which takes a really long time on our
systems (NTFS, 16GB repo, ~190K files, ~85K folders). This also
prevents the annoying "unversioned file exists" update issue. This
tool also has a flag where the parent directories are not updated at
all, so the slow directory-walk update pass doesn't take place at all.
This saves us a lot of time.
    Anyway, maybe this patch can use a similar trick, to check
local-lock status, and just skip updates on parent directories where
local-lock issues exist, and report the problem to the user. Other
directories and files could be updated normally. This would prevent
the need for cleanup, which is the biggest slow-down issue for us.
Also, files and parent directories that fail to update wouldn't have
their WC version number changed, and thus continue to reflect
recursive version status correctly. Prevention here is probably more
efficient than trying to correct status data afterwards.

Jared

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Updating exclusively opened files locks working copy

[Philip Martin <ph...@codematters.co.uk>]

"D.J. Heap" <dj...@gmail.com> writes:

> Yes, there may be a better way to handle the problem, but I'm not
> terribly familiar with the working copy and log running code.  And
> that is correct -- this is just attempting to detect the situation
> where other applications already have files open and locked that we
> need to update, preventing us from updating them.  In the distant
> past, failed updates like this didn't cause the working copy to end up
> locked -- but now it does.

If log files fail then the working copy remains locked.  If it wasn't
locked in the past it probably means that some operation that wasn't
loggy has become loggy.  (I seem to remember something about the first
command in a log file being special, i.e. if the log file had done
nothing then it could be removed, but on first glance that doesn't
seem to be present any more).

> As you said, if an application were to grab the file between this
> check and the log running code, it would still fail -- but what
> happens 95% of the time (in my experience) is that people have an
> application (editor of some kind, especially with binary files)
> running that is holding the file locked when they run update.  The
> update fails, they remember they need to close the editor/application,
> but now their working copy is locked and they have to cleanup and then
> update again.
>
> In the current implementation, when the update editor is running, it
> doesn't actually do much with the real file (just a stat, I think)
> until close_dir time when it is processing the log file and tries to
> really update the file.  It fails at this point, and I didn't see any
> way to gracefully recover -- all error handling just seems to leave
> the directory locked and force a 'cleanup' run later.  Is there
> something better we could do at that point?

By design.

> Even this patch should probably really be tweaked to open the file in
> read-write mode (to be a truer check) and that would require setting
> the file to read-write -- but we need to do that later, anyway, so I
> guess that's not too much of an issue.
>
> I'd be happy to handle it better in the log running code, though, if
> we can.  Or maybe update should just run cleanup automatically or
> something?

The whole point of cleanup is that it's not supposed to be run
automatically, something has gone wrong that requires user
intervention.  In this case running cleanup automatically would surely
hit the same file locked problem?

Your solution might be the best way to do it, although if you care
about the behaviour you should write a regression test as it's easy to
break corner cases like this.

Alternatively perhaps you could extend the log file processing so that
it does all the file handling while writing the log file, then it will
fail before running the log file.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org