You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@subversion.apache.org by Uroš Jovanović <ur...@gmail.com> on 2020/09/07 17:25:46 UTC
[ISSUE] Cancelling svn checkout leaves an open file handle
Hello,
Recently I came across this behaviour where if I use the Cancel API to
cancel a long running checkout operation, the process running the SVN code
leaves an open file handle in the .svn\tmp folder inside the working copy.
Initially, I thought it was a SharpSvn (1.9 x64) issue, but I observed the
same behaviour using TortoiseSVN with the latest 1.14 client.
It can be easily reproduced, especially when downloading large file, the
.svn\tmp folder holds a file named "svn-XYZ123" and when the operation is
cancelled, this file still has an open handle on it, which means that the
calling process needs to be shutdown before doing anything else with that
WC since cleanup and folder deletion aren't possible. In case of
TortoiseSVN it simply means the client window needs to be closed to perform
the next operation, but when using SharpSvn in context of a larger
application, this would mean the user needs to close the entire application
because one unmanaged file handle was left alive.
I searched through reported issues on Jira and couldn't find anything
similar.
SVN client version: 1.9 and 1.14.
Platform: Windows 10 (x64)
Best regards
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ Moving to dev@ since earlier replies in the thread moved there.
Please un-Cc users@ from replies. ]
Uroš Jovanović wrote on Tue, 08 Sep 2020 11:31 +0200:
> Hi Daniel,
>
> Not sure what are "relevant pools", I am not really too familiar with SVN :)
>
The C API manages lifetime of various resources — primarily memory, but
also file handles — via the apr_pool_t abstraction. Nearly every API
function takes at least one pool parameter.
Given that you mentioned closing the TortoiseSVN window is a valid
workaround for that version of the issue, I guess the file handle would
be closed once the pools passed to the checkout API are
cleared/destroyed, without requiring the process to be exited or
low-level system calls to be resorted to.
However, I don't know what the SharpSvn equivalent of
clearing/destroying a pool or destroying a client context is. That'd
be a question for the SharpSvn maintainers. (SharpSvn is a third party
project, not part of the core Subversion project.)
Cheers,
Daniel
> Best regards,
> Uros
>
>
> On Tue, Sep 8, 2020 at 12:51 AM Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
>
> > Uroš Jovanović wrote on Mon, 07 Sep 2020 19:25 +0200:
> > > when using SharpSvn in context of a larger
> > > application, this would mean the user needs to close the entire
> > application
> > > because one unmanaged file handle was left alive.
> >
> > Have you tried clearing the relevant pools?
> >
> > Could you help debugging this? (E.g., write a reproduction recipe or
> > a test case or track down where in the code the file gets opened.)
> >
> > Cheers,
> >
> > Daniel
> >
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ Moving to dev@ since earlier replies in the thread moved there.
Please un-Cc users@ from replies. ]
Uroš Jovanović wrote on Tue, 08 Sep 2020 11:31 +0200:
> Hi Daniel,
>
> Not sure what are "relevant pools", I am not really too familiar with SVN :)
>
The C API manages lifetime of various resources — primarily memory, but
also file handles — via the apr_pool_t abstraction. Nearly every API
function takes at least one pool parameter.
Given that you mentioned closing the TortoiseSVN window is a valid
workaround for that version of the issue, I guess the file handle would
be closed once the pools passed to the checkout API are
cleared/destroyed, without requiring the process to be exited or
low-level system calls to be resorted to.
However, I don't know what the SharpSvn equivalent of
clearing/destroying a pool or destroying a client context is. That'd
be a question for the SharpSvn maintainers. (SharpSvn is a third party
project, not part of the core Subversion project.)
Cheers,
Daniel
> Best regards,
> Uros
>
>
> On Tue, Sep 8, 2020 at 12:51 AM Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
>
> > Uroš Jovanović wrote on Mon, 07 Sep 2020 19:25 +0200:
> > > when using SharpSvn in context of a larger
> > > application, this would mean the user needs to close the entire
> > application
> > > because one unmanaged file handle was left alive.
> >
> > Have you tried clearing the relevant pools?
> >
> > Could you help debugging this? (E.g., write a reproduction recipe or
> > a test case or track down where in the code the file gets opened.)
> >
> > Cheers,
> >
> > Daniel
> >
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Wed, 09 Sep 2020 10:35 +0200:
> Would it be a better approach to enumerate all open fd:s before and after
> the call to 'checkout' and compare the list of open fd:s (after any pool
> cleanup required). Of course enumerating open fd:s most probably require
> platform specific code in the regression test.
Well, of course a platform-specific test is better than no test at all,
but let's try to minimize the platform dependencies. For instance,
instead of using /proc, which would work only on Linux, we could use
POSIX API promises to enumerate open fd's in a more portable way. (Off
the top of my head, open(2) and fcntl(2) both seem like candidates.)
Or maybe APR has already solved this problem.
Cheers,
Daniel
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Daniel Sahlberg <da...@gmail.com>.
Den ons 9 sep. 2020 kl 09:39 skrev Branko Čibej <br...@apache.org>:
> On 09.09.2020 08:18, Daniel Sahlberg wrote:
>
> Den ons 9 sep. 2020 kl 06:44 skrev Nathan Hartman <
> hartman.nathan@gmail.com>:
>
>> On Tue, Sep 8, 2020 at 6:08 AM Uroš Jovanović <ur...@gmail.com> wrote:
>> >
>> > Then, mid downloading some of the larger files a temp file will appear
>> in .svn\tmp. Once that happens, hit the Cancel button.
>> > It will signal the cancellation to the svn client and it will throw the
>> SvnOperationCanceledException, the SvnClient gets disposed BUT an open file
>> handle remains on ".svn\tmp\svn-XYZ123" file.
>> > If you try to delete it, Windows will complain that it is used by our
>> test app. :(
>>
>> Moving this to the dev@ list...
>>
>> Potentially long-running APIs such as 'checkout' allow the client to
>> provide a 'cancel_func' callback, which is called at various strategic
>> places to ask the client whether the operation should be canceled.
>>
>> It sounds to me like one of those places sees a cancel request and
>> returns to its caller, forgetting to do some cleanup.
>>
>> Last night I tried to find such a place by reading code.
>>
>> The 'checkout' command sets up a working copy (if necessary) and then
>> calls the 'update' logic to do the heavy lifting.
>>
>> The 'update' logic is quite involved as it handles all sorts of
>> possibilities, which means the number of branches of the call tree
>> that need to be checked are too numerous for my code reading approach
>> to be sensible.
>>
>> My thoughts for an automated approach, provided there is a way for a
>> process to inquire how many open file handles it has (I assume there
>> is a way; I've just never had to do this): The idea is to write a
>> minimal client that does the following (on a ramdrive):
>>
>> 1. Check out a working copy of a repository, giving a cancel_func 'A'
>> that increments a global variable 'n' each time it is called and
>> always returns "don't cancel."
>>
>> 2. Loop n times, the loop counter being a global variable 'x':
>>
>> 2.1: Delete the working copy.
>>
>> 2.2: Check out a working copy of the same repository, giving a
>> different cancel_func 'B' that returns "don't cancel" the
>> first (x - 1) times it is called, and returns "cancel" the
>> x-th time it is called.
>>
>> 2.3: Test whether there are open file handles. If there are, we
>> know at which iteration the cleanup is not done, and we break
>> out of the loop.
>>
>> 3. If x >= n, quit; we didn't find the problem.
>>
>> 4. Delete the working copy.
>>
>> 5. Check out a working copy of the same repository, giving a different
>> cancel_func 'C' that returns "don't cancel" the first (x - 1) times
>> it is called, and traps the x-th time it is called, allowing the
>> call stack to be examined.
>>
>> Notes and caveats:
>>
>> 1. This could run for days (or years).
>>
>> 2. Then again, if it can be exposed pretty reliably by a user hitting
>> a Cancel button in a GUI, that means cancel_func is called
>> frequently enough from the offending location that it should
>> (hopefully) be caught relatively soon in the process.
>>
>> 3. I think a huge repository isn't needed. The Greek Tree used by the
>> test suite may suffice. If it doesn't expose the bug, I'd retry
>> with a larger file thrown in. If that doesn't expose it, add
>> increasing complexity such as externals, etc.
>>
>> 4. This relies on the logic being executed identically for each
>> checkout (i.e., cancel_func is called the same number of times from
>> the same call sites).
>>
>> 5. No idea how this could be turned into a regression test.
>>
>> 6. If there's a better way, I'd love to hear it!
>>
>
> For a regression test (as well as trying to pinpoint what goes wrong),
> wouldn't it be enough if the cancel_func check for the presence of a file
> in .svn/tmp (maybe even checking if it is open - in Linux that should be
> easy enough to check in /proc/$PID/fd) and then signal to cancel. That
> would "only" need a repository/file that is large enough to trigger calling
> the cancel_func.
>
>
> Not even that. The cancel check function is provided by the caller (in the
> client context) and can return 'true' when the conditions are right.
> Finding the correct temp directory is a bit more involved though because
> it's platform-dependent (i.e., I'm not sure if APR can reliably tell us
> that).
>
>
> I checked quickly and I also see the open file when checking out using
> TortoiseSVN and cancelling and it seems to occur all the time.
>
>
> It could be a but in our library, or it could be a bug in TortoiseSVN,
> since the API caller controls the lifetime of pools. The easiest way to
> check would be to run a checkout from the command line client built with
> APR pool debugging enabled (that means a custom build of APR is needed,
> too) and examining the debug output.
>
Thanks for your input! If it is in TortoiseSVN, then the same bug also
exists in SharpSVN, thus I'm leaning towards a bug in Subversion itself.
I'll try to find some time to check this later this week.
Would it be a better approach to enumerate all open fd:s before and after
the call to 'checkout' and compare the list of open fd:s (after any pool
cleanup required). Of course enumerating open fd:s most probably require
platform specific code in the regression test.
>
Daniel
>
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Wed, 09 Sep 2020 09:39 +0200:
> On 09.09.2020 08:18, Daniel Sahlberg wrote:
> > For a regression test (as well as trying to pinpoint what goes wrong),
> > wouldn't it be enough if the cancel_func check for the presence of a
> > file in .svn/tmp (maybe even checking if it is open - in Linux that
> > should be easy enough to check in /proc/$PID/fd) and then signal to
> > cancel. That would "only" need a repository/file that is large enough
> > to trigger calling the cancel_func.
>
> Not even that. The cancel check function is provided by the caller (in
> the client context) and can return 'true' when the conditions are right.
> Finding the correct temp directory is a bit more involved though because
> it's platform-dependent (i.e., I'm not sure if APR can reliably tell us
> that).
Why would finding the correct temp directory be a problem? The file in
question isn't created in the system temp dir but in .svn/tmp/.
Wouldn't it always be created there?
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Branko Čibej <br...@apache.org>.
On 09.09.2020 08:18, Daniel Sahlberg wrote:
> Den ons 9 sep. 2020 kl 06:44 skrev Nathan Hartman
> <hartman.nathan@gmail.com <ma...@gmail.com>>:
>
> On Tue, Sep 8, 2020 at 6:08 AM Uroš Jovanović <urosh3d@gmail.com
> <ma...@gmail.com>> wrote:
> >
> > Then, mid downloading some of the larger files a temp file will
> appear in .svn\tmp. Once that happens, hit the Cancel button.
> > It will signal the cancellation to the svn client and it will
> throw the SvnOperationCanceledException, the SvnClient gets
> disposed BUT an open file handle remains on ".svn\tmp\svn-XYZ123"
> file.
> > If you try to delete it, Windows will complain that it is used
> by our test app. :(
>
> Moving this to the dev@ list...
>
> Potentially long-running APIs such as 'checkout' allow the client to
> provide a 'cancel_func' callback, which is called at various strategic
> places to ask the client whether the operation should be canceled.
>
> It sounds to me like one of those places sees a cancel request and
> returns to its caller, forgetting to do some cleanup.
>
> Last night I tried to find such a place by reading code.
>
> The 'checkout' command sets up a working copy (if necessary) and then
> calls the 'update' logic to do the heavy lifting.
>
> The 'update' logic is quite involved as it handles all sorts of
> possibilities, which means the number of branches of the call tree
> that need to be checked are too numerous for my code reading approach
> to be sensible.
>
> My thoughts for an automated approach, provided there is a way for a
> process to inquire how many open file handles it has (I assume there
> is a way; I've just never had to do this): The idea is to write a
> minimal client that does the following (on a ramdrive):
>
> 1. Check out a working copy of a repository, giving a cancel_func 'A'
> that increments a global variable 'n' each time it is called and
> always returns "don't cancel."
>
> 2. Loop n times, the loop counter being a global variable 'x':
>
> 2.1: Delete the working copy.
>
> 2.2: Check out a working copy of the same repository, giving a
> different cancel_func 'B' that returns "don't cancel" the
> first (x - 1) times it is called, and returns "cancel" the
> x-th time it is called.
>
> 2.3: Test whether there are open file handles. If there are, we
> know at which iteration the cleanup is not done, and we break
> out of the loop.
>
> 3. If x >= n, quit; we didn't find the problem.
>
> 4. Delete the working copy.
>
> 5. Check out a working copy of the same repository, giving a different
> cancel_func 'C' that returns "don't cancel" the first (x - 1) times
> it is called, and traps the x-th time it is called, allowing the
> call stack to be examined.
>
> Notes and caveats:
>
> 1. This could run for days (or years).
>
> 2. Then again, if it can be exposed pretty reliably by a user hitting
> a Cancel button in a GUI, that means cancel_func is called
> frequently enough from the offending location that it should
> (hopefully) be caught relatively soon in the process.
>
> 3. I think a huge repository isn't needed. The Greek Tree used by the
> test suite may suffice. If it doesn't expose the bug, I'd retry
> with a larger file thrown in. If that doesn't expose it, add
> increasing complexity such as externals, etc.
>
> 4. This relies on the logic being executed identically for each
> checkout (i.e., cancel_func is called the same number of times from
> the same call sites).
>
> 5. No idea how this could be turned into a regression test.
>
> 6. If there's a better way, I'd love to hear it!
>
>
> For a regression test (as well as trying to pinpoint what goes wrong),
> wouldn't it be enough if the cancel_func check for the presence of a
> file in .svn/tmp (maybe even checking if it is open - in Linux that
> should be easy enough to check in /proc/$PID/fd) and then signal to
> cancel. That would "only" need a repository/file that is large enough
> to trigger calling the cancel_func.
Not even that. The cancel check function is provided by the caller (in
the client context) and can return 'true' when the conditions are right.
Finding the correct temp directory is a bit more involved though because
it's platform-dependent (i.e., I'm not sure if APR can reliably tell us
that).
> I checked quickly and I also see the open file when checking out using
> TortoiseSVN and cancelling and it seems to occur all the time.
It could be a but in our library, or it could be a bug in TortoiseSVN,
since the API caller controls the lifetime of pools. The easiest way to
check would be to run a checkout from the command line client built with
APR pool debugging enabled (that means a custom build of APR is needed,
too) and examining the debug output.
-- Brane
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Uroš Jovanović <ur...@gmail.com>.
>
> Do you know if SharpSvn is maintained? When I checked your previous test
> code it seemed the NuGet package of SharpSvn is based on Subversion 1.9 and
> any bugfix will (at maximum) be applied to 1.10 and 1.14. So maybe you are
> out of luck even if we find it is a bug in Subversion and fix it.
I glanced through their forums, they have plans on updating, but obviously
with no hurry since the last nuget was published in 2017.
Regarding test cases I suppose these should be plain C to make sure they
> are executable on any platform.
>
Unfortunately, I don't think I'll manage to provide the test case in plain
C but I hope the link I provided can help you with Win API.
Will there be a Jira issue made, so there is a way to monitor the status?
If you need any further assistance, please let me know.
Regards,
Uros
On Wed, Sep 9, 2020 at 10:48 AM Daniel Sahlberg <da...@gmail.com>
wrote:
> Den ons 9 sep. 2020 kl 10:38 skrev Uroš Jovanović <ur...@gmail.com>:
>
>> Hi Nathan,
>> Hi Daniel,
>>
>> Thank you both for your replies.
>>
>> As a workaround I ended up doing exactly the things you mentioned. After
>> the cancellation of the operation I call additional code to "cleanup".
>> This code goes through open file handles held by the current process and
>> closes the ones left over in .svn\tmp.
>>
>> That being said, I don't know how easy/hard this is to do on Linux, but
>> doing this kind of stuff on Windows is a giant pain in the a**.
>> It involves calling a bunch of poorly documented, low-level native
>> methods from Windows' internal api.
>>
>> Luckily, I found some code online and modified it in order to work on
>> both 32 and 64 bit apps.
>> For reference, you can find this code on my github
>> <https://github.com/urosjovanovic/MceController/blob/master/VmcServices/DetectOpenFiles.cs>
>> .
>>
>
> GREAT, you just saved me a couple of hours of work to dig up the APIs.
>
>
>>
>> So the final process is as follows:
>>
>> 1. Perform checkout
>> 2. Cancel checkout
>> 3. If present, force-close the open file handle in .svn\tmp
>> 4. Perform a regular svn cleanup to break locks etc.
>>
>> It works, but now I have an additional ~700 LOC to maintain just in order
>> to detect and close those rogue file handles, hoping it will not crash the
>> app. :)
>>
>> If you want, I can provide a test case written in C# using SharpSvn which
>> will work exactly as Daniel wrote, but on Windows.
>> I can also use the repo of your choice, if you have a preferred one for
>> testing.
>>
>
> Do you know if SharpSvn is maintained? When I checked your previous test
> code it seemed the NuGet package of SharpSvn is based on Subversion 1.9 and
> any bugfix will (at maximum) be applied to 1.10 and 1.14. So maybe you are
> out of luck even if we find it is a bug in Subversion and fix it.
>
> Regarding test cases I suppose these should be plain C to make sure they
> are executable on any platform.
>
> Daniel
>
>>
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Daniel Sahlberg <da...@gmail.com>.
Den ons 9 sep. 2020 kl 10:38 skrev Uroš Jovanović <ur...@gmail.com>:
> Hi Nathan,
> Hi Daniel,
>
> Thank you both for your replies.
>
> As a workaround I ended up doing exactly the things you mentioned. After
> the cancellation of the operation I call additional code to "cleanup".
> This code goes through open file handles held by the current process and
> closes the ones left over in .svn\tmp.
>
> That being said, I don't know how easy/hard this is to do on Linux, but
> doing this kind of stuff on Windows is a giant pain in the a**.
> It involves calling a bunch of poorly documented, low-level native methods
> from Windows' internal api.
>
> Luckily, I found some code online and modified it in order to work on both
> 32 and 64 bit apps.
> For reference, you can find this code on my github
> <https://github.com/urosjovanovic/MceController/blob/master/VmcServices/DetectOpenFiles.cs>
> .
>
GREAT, you just saved me a couple of hours of work to dig up the APIs.
>
> So the final process is as follows:
>
> 1. Perform checkout
> 2. Cancel checkout
> 3. If present, force-close the open file handle in .svn\tmp
> 4. Perform a regular svn cleanup to break locks etc.
>
> It works, but now I have an additional ~700 LOC to maintain just in order
> to detect and close those rogue file handles, hoping it will not crash the
> app. :)
>
> If you want, I can provide a test case written in C# using SharpSvn which
> will work exactly as Daniel wrote, but on Windows.
> I can also use the repo of your choice, if you have a preferred one for
> testing.
>
Do you know if SharpSvn is maintained? When I checked your previous test
code it seemed the NuGet package of SharpSvn is based on Subversion 1.9 and
any bugfix will (at maximum) be applied to 1.10 and 1.14. So maybe you are
out of luck even if we find it is a bug in Subversion and fix it.
Regarding test cases I suppose these should be plain C to make sure they
are executable on any platform.
Daniel
>
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Uroš Jovanović <ur...@gmail.com>.
Hi Nathan,
Hi Daniel,
Thank you both for your replies.
As a workaround I ended up doing exactly the things you mentioned. After
the cancellation of the operation I call additional code to "cleanup".
This code goes through open file handles held by the current process and
closes the ones left over in .svn\tmp.
That being said, I don't know how easy/hard this is to do on Linux, but
doing this kind of stuff on Windows is a giant pain in the a**.
It involves calling a bunch of poorly documented, low-level native methods
from Windows' internal api.
Luckily, I found some code online and modified it in order to work on both
32 and 64 bit apps.
For reference, you can find this code on my github
<https://github.com/urosjovanovic/MceController/blob/master/VmcServices/DetectOpenFiles.cs>
.
So the final process is as follows:
1. Perform checkout
2. Cancel checkout
3. If present, force-close the open file handle in .svn\tmp
4. Perform a regular svn cleanup to break locks etc.
It works, but now I have an additional ~700 LOC to maintain just in order
to detect and close those rogue file handles, hoping it will not crash the
app. :)
If you want, I can provide a test case written in C# using SharpSvn which
will work exactly as Daniel wrote, but on Windows.
I can also use the repo of your choice, if you have a preferred one for
testing.
Regards,
Uros
On Wed, Sep 9, 2020 at 8:18 AM Daniel Sahlberg <da...@gmail.com>
wrote:
> Den ons 9 sep. 2020 kl 06:44 skrev Nathan Hartman <
> hartman.nathan@gmail.com>:
>
>> On Tue, Sep 8, 2020 at 6:08 AM Uroš Jovanović <ur...@gmail.com> wrote:
>> >
>> > Then, mid downloading some of the larger files a temp file will appear
>> in .svn\tmp. Once that happens, hit the Cancel button.
>> > It will signal the cancellation to the svn client and it will throw the
>> SvnOperationCanceledException, the SvnClient gets disposed BUT an open file
>> handle remains on ".svn\tmp\svn-XYZ123" file.
>> > If you try to delete it, Windows will complain that it is used by our
>> test app. :(
>>
>> Moving this to the dev@ list...
>>
>> Potentially long-running APIs such as 'checkout' allow the client to
>> provide a 'cancel_func' callback, which is called at various strategic
>> places to ask the client whether the operation should be canceled.
>>
>> It sounds to me like one of those places sees a cancel request and
>> returns to its caller, forgetting to do some cleanup.
>>
>> Last night I tried to find such a place by reading code.
>>
>> The 'checkout' command sets up a working copy (if necessary) and then
>> calls the 'update' logic to do the heavy lifting.
>>
>> The 'update' logic is quite involved as it handles all sorts of
>> possibilities, which means the number of branches of the call tree
>> that need to be checked are too numerous for my code reading approach
>> to be sensible.
>>
>> My thoughts for an automated approach, provided there is a way for a
>> process to inquire how many open file handles it has (I assume there
>> is a way; I've just never had to do this): The idea is to write a
>> minimal client that does the following (on a ramdrive):
>>
>> 1. Check out a working copy of a repository, giving a cancel_func 'A'
>> that increments a global variable 'n' each time it is called and
>> always returns "don't cancel."
>>
>> 2. Loop n times, the loop counter being a global variable 'x':
>>
>> 2.1: Delete the working copy.
>>
>> 2.2: Check out a working copy of the same repository, giving a
>> different cancel_func 'B' that returns "don't cancel" the
>> first (x - 1) times it is called, and returns "cancel" the
>> x-th time it is called.
>>
>> 2.3: Test whether there are open file handles. If there are, we
>> know at which iteration the cleanup is not done, and we break
>> out of the loop.
>>
>> 3. If x >= n, quit; we didn't find the problem.
>>
>> 4. Delete the working copy.
>>
>> 5. Check out a working copy of the same repository, giving a different
>> cancel_func 'C' that returns "don't cancel" the first (x - 1) times
>> it is called, and traps the x-th time it is called, allowing the
>> call stack to be examined.
>>
>> Notes and caveats:
>>
>> 1. This could run for days (or years).
>>
>> 2. Then again, if it can be exposed pretty reliably by a user hitting
>> a Cancel button in a GUI, that means cancel_func is called
>> frequently enough from the offending location that it should
>> (hopefully) be caught relatively soon in the process.
>>
>> 3. I think a huge repository isn't needed. The Greek Tree used by the
>> test suite may suffice. If it doesn't expose the bug, I'd retry
>> with a larger file thrown in. If that doesn't expose it, add
>> increasing complexity such as externals, etc.
>>
>> 4. This relies on the logic being executed identically for each
>> checkout (i.e., cancel_func is called the same number of times from
>> the same call sites).
>>
>> 5. No idea how this could be turned into a regression test.
>>
>> 6. If there's a better way, I'd love to hear it!
>>
>
> For a regression test (as well as trying to pinpoint what goes wrong),
> wouldn't it be enough if the cancel_func check for the presence of a file
> in .svn/tmp (maybe even checking if it is open - in Linux that should be
> easy enough to check in /proc/$PID/fd) and then signal to cancel. That
> would "only" need a repository/file that is large enough to trigger calling
> the cancel_func.
>
> I checked quickly and I also see the open file when checking out using
> TortoiseSVN and cancelling and it seems to occur all the time.
>
> Kind regards
> Daniel
>
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Daniel Sahlberg <da...@gmail.com>.
Den ons 9 sep. 2020 kl 06:44 skrev Nathan Hartman <hartman.nathan@gmail.com
>:
> On Tue, Sep 8, 2020 at 6:08 AM Uroš Jovanović <ur...@gmail.com> wrote:
> >
> > Then, mid downloading some of the larger files a temp file will appear
> in .svn\tmp. Once that happens, hit the Cancel button.
> > It will signal the cancellation to the svn client and it will throw the
> SvnOperationCanceledException, the SvnClient gets disposed BUT an open file
> handle remains on ".svn\tmp\svn-XYZ123" file.
> > If you try to delete it, Windows will complain that it is used by our
> test app. :(
>
> Moving this to the dev@ list...
>
> Potentially long-running APIs such as 'checkout' allow the client to
> provide a 'cancel_func' callback, which is called at various strategic
> places to ask the client whether the operation should be canceled.
>
> It sounds to me like one of those places sees a cancel request and
> returns to its caller, forgetting to do some cleanup.
>
> Last night I tried to find such a place by reading code.
>
> The 'checkout' command sets up a working copy (if necessary) and then
> calls the 'update' logic to do the heavy lifting.
>
> The 'update' logic is quite involved as it handles all sorts of
> possibilities, which means the number of branches of the call tree
> that need to be checked are too numerous for my code reading approach
> to be sensible.
>
> My thoughts for an automated approach, provided there is a way for a
> process to inquire how many open file handles it has (I assume there
> is a way; I've just never had to do this): The idea is to write a
> minimal client that does the following (on a ramdrive):
>
> 1. Check out a working copy of a repository, giving a cancel_func 'A'
> that increments a global variable 'n' each time it is called and
> always returns "don't cancel."
>
> 2. Loop n times, the loop counter being a global variable 'x':
>
> 2.1: Delete the working copy.
>
> 2.2: Check out a working copy of the same repository, giving a
> different cancel_func 'B' that returns "don't cancel" the
> first (x - 1) times it is called, and returns "cancel" the
> x-th time it is called.
>
> 2.3: Test whether there are open file handles. If there are, we
> know at which iteration the cleanup is not done, and we break
> out of the loop.
>
> 3. If x >= n, quit; we didn't find the problem.
>
> 4. Delete the working copy.
>
> 5. Check out a working copy of the same repository, giving a different
> cancel_func 'C' that returns "don't cancel" the first (x - 1) times
> it is called, and traps the x-th time it is called, allowing the
> call stack to be examined.
>
> Notes and caveats:
>
> 1. This could run for days (or years).
>
> 2. Then again, if it can be exposed pretty reliably by a user hitting
> a Cancel button in a GUI, that means cancel_func is called
> frequently enough from the offending location that it should
> (hopefully) be caught relatively soon in the process.
>
> 3. I think a huge repository isn't needed. The Greek Tree used by the
> test suite may suffice. If it doesn't expose the bug, I'd retry
> with a larger file thrown in. If that doesn't expose it, add
> increasing complexity such as externals, etc.
>
> 4. This relies on the logic being executed identically for each
> checkout (i.e., cancel_func is called the same number of times from
> the same call sites).
>
> 5. No idea how this could be turned into a regression test.
>
> 6. If there's a better way, I'd love to hear it!
>
For a regression test (as well as trying to pinpoint what goes wrong),
wouldn't it be enough if the cancel_func check for the presence of a file
in .svn/tmp (maybe even checking if it is open - in Linux that should be
easy enough to check in /proc/$PID/fd) and then signal to cancel. That
would "only" need a repository/file that is large enough to trigger calling
the cancel_func.
I checked quickly and I also see the open file when checking out using
TortoiseSVN and cancelling and it seems to occur all the time.
Kind regards
Daniel
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Sep 8, 2020 at 6:08 AM Uroš Jovanović <ur...@gmail.com> wrote:
>
> Then, mid downloading some of the larger files a temp file will appear in .svn\tmp. Once that happens, hit the Cancel button.
> It will signal the cancellation to the svn client and it will throw the SvnOperationCanceledException, the SvnClient gets disposed BUT an open file handle remains on ".svn\tmp\svn-XYZ123" file.
> If you try to delete it, Windows will complain that it is used by our test app. :(
Moving this to the dev@ list...
Potentially long-running APIs such as 'checkout' allow the client to
provide a 'cancel_func' callback, which is called at various strategic
places to ask the client whether the operation should be canceled.
It sounds to me like one of those places sees a cancel request and
returns to its caller, forgetting to do some cleanup.
Last night I tried to find such a place by reading code.
The 'checkout' command sets up a working copy (if necessary) and then
calls the 'update' logic to do the heavy lifting.
The 'update' logic is quite involved as it handles all sorts of
possibilities, which means the number of branches of the call tree
that need to be checked are too numerous for my code reading approach
to be sensible.
My thoughts for an automated approach, provided there is a way for a
process to inquire how many open file handles it has (I assume there
is a way; I've just never had to do this): The idea is to write a
minimal client that does the following (on a ramdrive):
1. Check out a working copy of a repository, giving a cancel_func 'A'
that increments a global variable 'n' each time it is called and
always returns "don't cancel."
2. Loop n times, the loop counter being a global variable 'x':
2.1: Delete the working copy.
2.2: Check out a working copy of the same repository, giving a
different cancel_func 'B' that returns "don't cancel" the
first (x - 1) times it is called, and returns "cancel" the
x-th time it is called.
2.3: Test whether there are open file handles. If there are, we
know at which iteration the cleanup is not done, and we break
out of the loop.
3. If x >= n, quit; we didn't find the problem.
4. Delete the working copy.
5. Check out a working copy of the same repository, giving a different
cancel_func 'C' that returns "don't cancel" the first (x - 1) times
it is called, and traps the x-th time it is called, allowing the
call stack to be examined.
Notes and caveats:
1. This could run for days (or years).
2. Then again, if it can be exposed pretty reliably by a user hitting
a Cancel button in a GUI, that means cancel_func is called
frequently enough from the offending location that it should
(hopefully) be caught relatively soon in the process.
3. I think a huge repository isn't needed. The Greek Tree used by the
test suite may suffice. If it doesn't expose the bug, I'd retry
with a larger file thrown in. If that doesn't expose it, add
increasing complexity such as externals, etc.
4. This relies on the logic being executed identically for each
checkout (i.e., cancel_func is called the same number of times from
the same call sites).
5. No idea how this could be turned into a regression test.
6. If there's a better way, I'd love to hear it!
Nathan
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Uroš Jovanović <ur...@gmail.com>.
Hi Daniel,
Not sure what are "relevant pools", I am not really too familiar with SVN :)
Attached below is a minimal reproduction case.
It is a C# winforms app with Check and Cancel button (.NET 4.8 with
SharpSvn 1.9 x64 nuget)
You would just need to type in some real svn repo address in Form1.cs in
order to start the checkout process.
I advise choosing the repo with large files which take time to download.
Then, mid downloading some of the larger files a temp file will appear in
.svn\tmp. Once that happens, hit the Cancel button.
It will signal the cancellation to the svn client and it will throw the
SvnOperationCanceledException, the SvnClient gets disposed BUT an open file
handle remains on ".svn\tmp\svn-XYZ123" file.
If you try to delete it, Windows will complain that it is used by our test
app. :(
Let me know if you need any further assistance.
Best regards,
Uros
On Tue, Sep 8, 2020 at 12:51 AM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Uroš Jovanović wrote on Mon, 07 Sep 2020 19:25 +0200:
> > when using SharpSvn in context of a larger
> > application, this would mean the user needs to close the entire
> application
> > because one unmanaged file handle was left alive.
>
> Have you tried clearing the relevant pools?
>
> Could you help debugging this? (E.g., write a reproduction recipe or
> a test case or track down where in the code the file gets opened.)
>
> Cheers,
>
> Daniel
>
Re: [ISSUE] Cancelling svn checkout leaves an open file handle
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Uroš Jovanović wrote on Mon, 07 Sep 2020 19:25 +0200:
> when using SharpSvn in context of a larger
> application, this would mean the user needs to close the entire application
> because one unmanaged file handle was left alive.
Have you tried clearing the relevant pools?
Could you help debugging this? (E.g., write a reproduction recipe or
a test case or track down where in the code the file gets opened.)
Cheers,
Daniel