Hi
On Mon, Nov 3, 2008 at 5:46 AM, Hyrum K. Wright
<hyrum_wright_at_mail.utexas.edu> wrote:
> Joe Swatosh wrote:
>> Hi
>>
>> On Wed, Oct 29, 2008 at 9:54 PM, Joe Swatosh <joe.swatosh_at_gmail.com> wrote:
>>> Howdy Hyrum, kou,
>>>
>>> On Mon, Oct 27, 2008 at 8:08 AM, Hyrum K. Wright
>>> <hyrum_wright_at_mail.utexas.edu> wrote:
>>>> Joe Swatosh wrote:
>>>>> Hi Hyrum,
>>>>>
>>>>> On Sat, Oct 18, 2008 at 6:12 AM, <hwright_at_tigris.org> wrote:
>>>>>> Author: hwright
>>>>>> Date: Sat Oct 18 06:11:59 2008
>>>>>> New Revision: 33730
>>>>>>
>>>>>> Log:
>>>>>> Merge the fs-rep-sharing branch to trunk.
>>>>>>
>>>>>> This gives us significant space savings on both bdb and fsfs. See the
>>>>>> branch for individual log messages.
>>>>>>
>>>>> Obviously, this commit broke the windows build, but when it was fixed
>>>>> in r33743 none of the Ruby bindings tests would even run anymore. I
>>>>> instrumented the calls to sqlite3_open_v2() and sqlite3_close() and
>>>>> many of the calls from the bindings into svn. Below is the output
>>>>> along with some comments by me marked by #'s. I hope I'm talking
>>>>> sense. This was run against trunk at r33894.
>>>>>
>>>>>
>>
>> snip
>>
>>>>> I'm just a bystander watching the development of svn proper, but I wonder
>>>>> if there is a better pool to put the clean up of the cache opened by the
>>>>> commit call above? It seems like ideally, it should be released before
>>>>> the invocation of commit completes (like checkout).
>>>> The rep-sharing sqlite database currently closes itself when the pool provided
>>>> to svn_fs_new() or svn_fs_open() is destroyed. I suppose that if a caller
>>>> doesn't destroy the pool, but instead uses it on a subsequent call to
>>>> svn_fs_open(), then we'll have two open handles to the sqlite database. That in
>>>> and of itself shouldn't be too much of a problem, but I'm wondering if the
>>>> cleanup function we register with APR is getting confused.
>>>>
>>>> Thoughts?
>>>>
>>> Yup, the sqlite database is closed, but for whatever reason
>>> svn_client_commit4 and svn_client_list2 (and others) are just passing
>>> along the pool that was passed to them. I don't think the cleanup
>>> function is confused, I just think the lifetime of the pool we are
>>> passing in is too long. This might not even be problem on non-Windows
>>> OSes that can delete open files.
>>>
>>> The apparent reason that svn_client_checkout3 was working for me was
>>> that it calls svn_client__checkout_internal, which creates a temporary
>>> "session_pool" that is passed to svn_ra_open3 and so the sqlite
>>> database is closed before returning to Ruby.
>
> This part is odd, because the SQLite database should be created in the same pool
> that the filesystem is, and should be closed only when the filesystem is closed.
> If there's a difference in there somewhere, that's a bug.
I haven't really looked at the code much, just did my experiments, so
I couldn't say that it isn't a problem with the bindings keeping the
filesystem open too long. It seems unlikely though since the symptom
we're getting now is that there is an open file that can't be deleted
during test cleanup.
Further, this isn't a likely pattern of use, although it doesn't seem
like it should be prohibited, either. We create repository during
setup, manipulate it during the test then delete it during teardown.
With my patch, I can now pass the first few tests, but eventually one
of the tests does something that causes the cache file to not be
closed, that test errors teardown then the error cascades to all
subsequent tests.
>
>>> I started down the path of creating a patch for the svn_client library
>>> mimicking that pattern of creating session_pools, but when I got to
>>> svn_client_open_ra_session, I started doubting the approach. It seems
>>> to me that if we are returning an RA session object that perhaps the
>>
>> Trying to make this a little more concrete, I've attached the patch in
>> progress. The more I think about it the less I like it. It seems
>> like there may be synchronization issues with having different
>> functions accessing the cache with different file handles.
>
> We've got some validation to do, to be sure (and I believe that's part of the
> notes that Dave made about FSFS in TODO-1.6). However, SQLite handles the
> multiple access issues for us. We're not working with different file handles,
> we're working with different database handles, and the SQLite library ensures
> that we can do that.
>
> Perchance, are experiencing this problem on any kind of network file system?
>
No, its all on a disk.
--
Joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-03 16:11:29 CET