[PATCH] Reduce the lifetime of open files within core FSFS7 routines

Hi,

I've noticed that the new fs-fs-pack-tests (#13 and #14) fail on my machine
around trunk_at_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