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 :-).
