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
Index: subversion/libsvn_fs_fs/cached_data.c
===================================================================
--- subversion/libsvn_fs_fs/cached_data.c (revision 1559108)
+++ subversion/libsvn_fs_fs/cached_data.c (working copy)
@@ -354,7 +354,6 @@ get_node_revision_body(node_revision_t **noderev_p
revision_file,
pool,
pool));
- SVN_ERR(svn_fs_fs__close_revision_file(revision_file));
}
else
{
@@ -373,6 +372,8 @@ get_node_revision_body(node_revision_t **noderev_p
*noderev_p,
pool));
}
+
+ SVN_ERR(svn_fs_fs__close_revision_file(revision_file));
}
return SVN_NO_ERROR;
@@ -884,6 +885,7 @@ svn_fs_fs__check_rep(representation_t *rep,
if (is_packed != rev_file.is_packed)
{
svn_error_clear(err);
+ SVN_ERR(svn_fs_fs__close_revision_file(&rev_file));
return svn_error_trace(svn_fs_fs__check_rep(rep, fs, hint, pool));
}
else
@@ -900,6 +902,8 @@ svn_fs_fs__check_rep(representation_t *rep,
" in revision %ld"),
apr_off_t_toa(pool, entry->offset),
rep->item_index, rep->revision);
+
+ SVN_ERR(svn_fs_fs__close_revision_file(&rev_file));
}
else
{
Index: subversion/libsvn_fs_fs/pack.c
===================================================================
--- subversion/libsvn_fs_fs/pack.c (revision 1559108)
+++ subversion/libsvn_fs_fs/pack.c (working copy)
@@ -1426,6 +1426,8 @@ append_revision(pack_context_t *context,
svn_pool_destroy(iterpool);
context->pack_offset += finfo.size;
+ SVN_ERR(svn_fs_fs__close_revision_file(rev_file));
+
return SVN_NO_ERROR;
}
Index: subversion/libsvn_fs_fs/verify.c
===================================================================
--- subversion/libsvn_fs_fs/verify.c (revision 1559108)
+++ subversion/libsvn_fs_fs/verify.c (working copy)
@@ -233,6 +233,8 @@ compare_l2p_to_p2l_index(svn_fs_t *fs,
svn_pool_destroy(iterpool);
+ SVN_ERR(svn_fs_fs__close_revision_file(&rev_file));
+
return SVN_NO_ERROR;
}
@@ -321,6 +323,8 @@ compare_p2l_to_l2p_index(svn_fs_t *fs,
svn_pool_destroy(iterpool);
+ SVN_ERR(svn_fs_fs__close_revision_file(&rev_file));
+
return SVN_NO_ERROR;
}
@@ -576,6 +580,8 @@ compare_p2l_to_rev(svn_fs_t *fs,
svn_pool_destroy(iterpool);
+ SVN_ERR(svn_fs_fs__close_revision_file(rev_file));
+
return SVN_NO_ERROR;
}
Reduce the lifetime of open revision files within core FSFS routines.
Whenever these files are only required in the scope of the routine itself,
there is no reason not to close them before exiting that routine. Moreover,
leaving them open can lead to various problems when attempting to reorganize
the repository structure on Windows (e.g., ENOTEMPTY error when removing
folders during 'svnadmin pack').
This changeset restores the 1.8.x behavior of these routines and unbreaks the
upgrade_new_txns_to_log_addressing / upgrade_old_txns_to_log_addressing tests
(fs-fs-pack-tests) on Windows.
* subversion/libsvn_fs_fs/cached_data.c
(get_node_revision_body, svn_fs_fs__check_rep):
Call svn_fs_fs__close_revision_file().
* subversion/libsvn_fs_fs/pack.c
(append_revision): Call svn_fs_fs__close_revision_file().
* subversion/libsvn_fs_fs/verify.c
(compare_l2p_to_p2l_index, compare_p2l_to_l2p_index, compare_p2l_to_rev):
Call svn_fs_fs__close_revision_file().
Patch by: Evgeny Kotkov <evgeny.kotkov{_AT_}visualsvn.com>
