missing close of rev file in fsfs - get_dir_contents?

[Johan Corveleyn <jc...@gmail.com>]

Hi devs,

I think I may have found a bug in fs_fs.c, while I was
examining/profiling the code. However, I am completely new at
subversion development, and quite unexperienced in c (I'm a java
developer actually, with some notions of c, but trying to learn fast),
so I may be imagining things. If so, feel free to let me know I
completely misunderstood things :-).

I think the rev files that are opened inside get_dir_contents (called
by svn_fs_fs__get_dir_contents) are not closed. At least I can't see
them being closed. I have instrumented the code with printf statements
(particularly inside svn_io_file_open, svn_io_file_close and in
apr_file_open, apr_file_close) to see what's happening. For the open
that happens inside get_dir_contents, I don't see svn_io_file_close
nor apr_file_close being hit later on.

Looking at the source code I see:
- get_dir_contents calls read_representation
- read_representation calls rep_read_get_baton, which creates a
rep_read_baton and opens the rev file inside build_rep_list ->
create_rep_state -> create_rep_state_body
- read_representation wraps it in a stream, and sets the close
function to rep_read_contents_close
- get_dir_contents reads stuff and closes the stream

Now, the close callback of the stream is rep_read_contents_close, which is:
[[[
static svn_error_t *
rep_read_contents_close(void *baton)
{
  struct rep_read_baton *rb = baton;

  svn_pool_destroy(rb->pool);
  svn_pool_destroy(rb->filehandle_pool);

  return SVN_NO_ERROR;
}
]]]

Apparently, this does not close the file. After this callback is run,
neither svn_io_file_close nor apr_file_close are being executed.

I read in the Community Guide (section "Exception handling") that "All
files opened with apr_file_open are closed at pool cleanup". If that's
correct, this closing either doesn't happen through apr_file_close, or
the above code doesn't destroy (cleanup?) the correct pools or the
filehandle is not associated with this pool or something.

If this is a bug, it's probably read_representation's fault (not
installing a good close callback on the stream). That function is also
used from other places besides get_dir_contents, so they may be
affected as well ...

This was tested with trunk of today (2010-02-23), on Windows (yes, I
actually managed to build on Windows :-)).

Johan

P.S.: I know littering the code with printf's is a quite primitive way
of debugging/profiling. But hey, I already have to learn c, apr and
the subversion source code, not to mention reading HACKING and the
subversion design doc, so give me a break :-).
P.P.S.: I'm actually looking at this code to see if I can do something
to improve fsfs' I/O behavior, opening and closing the same files all
the time (really, really costly if your repo is on a NAS mounted via
NFS). Maybe this is a hopeless or stupid mission, but I'm trying
anyway :-).

Re: missing close of rev file in fsfs - get_dir_contents?

[Johan Corveleyn <jc...@gmail.com>]

On Wed, Feb 24, 2010 at 7:10 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Johan Corveleyn <jc...@gmail.com> writes:
>
>> Looking at the source code I see:
>> - get_dir_contents calls read_representation
>> - read_representation calls rep_read_get_baton, which creates a
>> rep_read_baton and opens the rev file inside build_rep_list ->
>> create_rep_state -> create_rep_state_body
>> - read_representation wraps it in a stream, and sets the close
>> function to rep_read_contents_close
>> - get_dir_contents reads stuff and closes the stream
>>
>> Now, the close callback of the stream is rep_read_contents_close, which is:
>> [[[
>> static svn_error_t *
>> rep_read_contents_close(void *baton)
>> {
>>   struct rep_read_baton *rb = baton;
>>
>>   svn_pool_destroy(rb->pool);
>>   svn_pool_destroy(rb->filehandle_pool);
>>
>>   return SVN_NO_ERROR;
>> }
>> ]]]
>>
>> Apparently, this does not close the file. After this callback is run,
>> neither svn_io_file_close nor apr_file_close are being executed.
>
> You have the wrong function name.  Look at apr_file_open in
>
> http://svn.apache.org/repos/asf/apr/apr/trunk/file_io/unix/open.c
>
> to see the pool cleanup handler, it's on the pool passed in (unless
> the flag APR_FILE_NOCLEANUP is used), so that should be result_pool in
> svn_stream_open_readonly which ends up being rb->pool.  You should be
> able to set a breakpoint on the function apr_unix_file_cleanup (or
> file_cleanup on Windows) to catch the close.

Oh I see. Thanks for the explanation, Philip.

Johan

Re: missing close of rev file in fsfs - get_dir_contents?

[Philip Martin <ph...@wandisco.com>]

Johan Corveleyn <jc...@gmail.com> writes:

> Looking at the source code I see:
> - get_dir_contents calls read_representation
> - read_representation calls rep_read_get_baton, which creates a
> rep_read_baton and opens the rev file inside build_rep_list ->
> create_rep_state -> create_rep_state_body
> - read_representation wraps it in a stream, and sets the close
> function to rep_read_contents_close
> - get_dir_contents reads stuff and closes the stream
>
> Now, the close callback of the stream is rep_read_contents_close, which is:
> [[[
> static svn_error_t *
> rep_read_contents_close(void *baton)
> {
>   struct rep_read_baton *rb = baton;
>
>   svn_pool_destroy(rb->pool);
>   svn_pool_destroy(rb->filehandle_pool);
>
>   return SVN_NO_ERROR;
> }
> ]]]
>
> Apparently, this does not close the file. After this callback is run,
> neither svn_io_file_close nor apr_file_close are being executed.

You have the wrong function name.  Look at apr_file_open in

http://svn.apache.org/repos/asf/apr/apr/trunk/file_io/unix/open.c

to see the pool cleanup handler, it's on the pool passed in (unless
the flag APR_FILE_NOCLEANUP is used), so that should be result_pool in
svn_stream_open_readonly which ends up being rb->pool.  You should be
able to set a breakpoint on the function apr_unix_file_cleanup (or
file_cleanup on Windows) to catch the close.

-- 
Philip