On Mon, Jan 20, 2014 at 3:12 PM, Evgeny Kotkov <evgeny.kot...@visualsvn.com>wrote:
> Hi, > > I've noticed that the new fs-fs-pack-tests (#13 and #14) fail on my machine > around trunk@r1559108. This happens with different Windows versions (I've > tried Windows XP SP3 32-bit / Vista SP2 32-bit / Server 2008 R2 64-bit / > Windows 7 64-bit / Windows 8 64-bit) and does not happen with Ubuntu > 64-bit: > [[[ > svn_tests: E720145: Can't remove directory > 'upgrade_new_txns_to_log_addressing\revs\2': The directory is not empty. > FAIL: fs-fs-pack-test.exe 13: upgrade txns to log addressing in shared > FSFS > > svn_tests: E720145: Can't remove directory > 'upgrade_old_txns_to_log_addressing\revs\2': The directory is not empty. > FAIL: fs-fs-pack-test.exe 14: upgrade txns started before svnadmin > upgrade > ]]] > > This Windows-specific error is usually caused by an attempt to remove a > folder > while still having open file handles to some of its children. So, I > grabbed > the Process Monitor [1] and dumped the CreateFile and CloseFile events that > happen within the 'upgrade_new_txns_to_log_addressing' test. It is easy to > spot that the 'revs/2/8' file handle is only closed during the test cleanup > (when the per-function TEST_POOL is being cleared). As a consequence, an > attempt to pack the repository fails to delete the 'revs/2' folder with an > ENOTEMPTY error. I've tracked down the origin of this file handle and here > is the corresponding stacktrace: > [[[ > file_open + 0x20, libsvn_subr\io.c(350) > svn_io_file_open + 0x4a, libsvn_subr\io.c(3427) > open_pack_or_rev_file + 0x48, libsvn_fs_fs\rev_file.c(67) > svn_fs_fs__open_pack_or_rev_file + 0x45, libsvn_fs_fs\rev_file.c(124) > open_and_seek_revision + 0x52, libsvn_fs_fs\cached_data.c(217) > get_node_revision_body + 0x1a7, libsvn_fs_fs\cached_data.c(338) > svn_fs_fs__get_node_revision + 0x2a, libsvn_fs_fs\cached_data.c(387) > shards_spanned + 0x7a, libsvn_fs_fs\transaction.c(1858) > choose_delta_base + 0x8e, libsvn_fs_fs\transaction.c(1925) > write_container_delta_rep + 0xd5, libsvn_fs_fs\transaction.c(2655) > write_final_rev + 0x22b, libsvn_fs_fs\transaction.c(2923) > commit_body + 0x344, libsvn_fs_fs\transaction.c(3843) > ... > ]]] > > Now, if you examine the core FSFS routines like 'get_node_revision_body', > you > can notice that some of them close the temporary > 'svn_fs_fs__revision_file_t' > objects, but some of them do not, e.g., svn_fs_fs__get_changes does, but > get_node_revision_body does not (for the physical addressing mode). In the > latter case, this can lead to various sorts of problems, one of which was > caught by the fs-fs-pack-tests. > > This behavior looks like a regression from 1.8.x, where these routines did > close the temporary file handles via explicit 'svn_io_file_close' calls or > via > less explicit 'svn_stream_close' calls for the streams with DISOWN=FALSE. > I've attached a patch that restores this behavior for FSFS7. The invariant > is quite simple: on any non-erroneous flow, a routine should not leak the > file > handle(s) in the POOL that come from the temporary > 'svn_fs_fs__revision_file_t' > object(s). > > NOTE: It looks like FSX suffers from the same kind of problem. There are > 77 > failing tests from the standalone suite (client-test.exe, fs-test.exe, > ...) and > any attempt to run the Python tests (basic_tests.py, ...) immediately fails > during the greek tree initialization phase. I guess I might be able to > come up > with a patch for this problem in the nearby future (in case someone does > not > fix it earlier): > > [[[ > (client-test.exe --fs-type=fsx) > > svn_tests: E720145: Can't remove directory > 'BUILDPATH\subversion\tests\cmdline\svn-test-work\libsvn_client\ > test-wc-add-repos\db\transactions\0-0.txn': The directory is not empty. > FAIL: client-test.exe 3: test svn_wc_add3 scenarios > > (win-tests.py --fs-type=fsx --test=basic_tests.py --log-to-stdout) > > Testing Release configuration on local repository. > START: basic_tests.py > E: import did not succeed, while creating greek repos. > E: The final line from 'svn import' was: > E: Can't remove directory > 'BUILDPATH\subversion\tests\cmdline\svn-test-work\local-tmp\ > repos\db\transactions\0-0.txn': The directory is not empty. > ]]] > > [1] http://technet.microsoft.com/sysinternals/bb896645 > > > Thanks and regards, > Evgeny Kotkov > Hi Evgeny, Thanks for the patch! Reviewed and committed as r1559777. -- Stefan^2. P.S.: Ugh, Windows! ;)
