You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Hyrum K. Wright" <hy...@mail.utexas.edu> on 2007/08/08 19:25:45 UTC
serf property caching
Hi all.
I've been trying to get copy tests 56, 57, 58, 59, the multiple copy
tests, passing over ra_serf, and here's what I've discovered. As part
of the copy logic, and we use the svn_ra_get_log2() interface to
determine the old revision at a given path. The problem is an abort()
not on the first, but subsequent calls to svn_ra_serf__get_log().
svn_ra_serf__get_log() uses svn_ra_serf__retrieve_props(() to get a
resource's properties from the server, which can then be fetched using
svn_ra_serf__get_ver_prop(). On the first call to retrieve_props(),
everything works as expected, and the correct values are interested into
the props cache, and can retrieved from the props hash. On subsequent
calls, the props are are already cached, but aren't making it back to
get_log(), which leads to the abort().
The property we are interested in here is DAV:href, which gets returned
correctly the first time. The second time it doesn't, because we aren't
explicitly asking for it, but rather the set of checked_in_props. (This
set does *not* contain DAV:href, and I don't know why the initial call
to retrieve_props() succeeds.) When I use all_props instead of
checked_in_props, the DAV:href gets pulled from the cache correctly, and
we don't hit the abort().
Is my analysis correct? Is this the right way to solve this problem, or
does it just treat the symptom, and not the disease? Is there a
performance issue with using all_props?
The attached patch is my change. If nobody objects, I'll commit this in
the next couple of days.
-Hyrum
[[[
Fix copy tests 56, 57, 58, and 59 over ra_serf. The DAV:href property
is not part of the check_in_props group, so we need to use all_props to
ensure that it is properly fetched from the cache during multiple calls
to svn_ra_serf__retrieve_props()
* subversion/libsvn_ra_serf/log.c
(svn_ra_serf__get_log): Use retrieve all_props when retrieving
properties of the vcc_url.
]]]
Re: serf property caching
Posted by Lieven Govaerts <sv...@mobsol.be>.
Hyrum K. Wright wrote:
> Justin Erenkrantz wrote:
>
>> On 8/8/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
>>
> ...
>
>>> explicitly asking for it, but rather the set of checked_in_props. (This
>>> set does *not* contain DAV:href, and I don't know why the initial call
>>> to retrieve_props() succeeds.) When I use all_props instead of
>>> checked_in_props, the DAV:href gets pulled from the cache correctly, and
>>> we don't hit the abort().
>>>
>> Instead of using all_props, I'd just add a new structure to ra_serf.h:
>>
>> static const svn_ra_serf__dav_props_t href_props[] =
>> {
>> { "DAV:", "href" },
>> { NULL }
>> };
>>
>> and use href_props rather than all_props.
>>
>
> I made this change, and it works. However, other calls to
> svn_ra_serf__retrieve_props() are failing with a different message:
> subversion/libsvn_ra_serf/util.c:329: (apr_err=20014)
> svn: Error running context: Internal error
>
> This looks like some kind of error internal to serf itself. In doing
> more digging, it looks to be coming from context.c, around line 870.
> Given that information, I think that this is a bug in serf, not in our
> usage of it. My serf-fu isn't that great, though, so I'm not sure. I'm
> going to commit my patch with Justin's recommended change above, but it
> will generate quite a number of new failures. Anybody with a little bit
> more knowledge care to debug this new problem?
>
Hyrum,
the problem is this:
127.0.0.1 - - [11/Aug/2007:23:28:25 +0200] "PROPFIND HTTP/1.1" 400 326 "-"
The client is doing a PROPFIND request without specifying a URI, hence
it fails with 400 Bad Request. I traced this back to
libsvn_ra_serf/log.c svn_ra_serf__get_log() line 490. The
svn_ra_serf__retrieve_props is called with baseline_url = "". Note
baseline_url is not NULL, that's why svn isn't aborting here.
I didn't look into it any further since it seems to be related to what
you were trying to solve. If you need more info let me know.
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: serf property caching
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 8/14/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
> Unfortunately, I also have other things on my plate, and r26087 is about
> the best I can do at this point. The solution seems to require a bit
> more knowledge about DAV than I have at the moment. Anybody want to
> pick it up from here?
As said on IRC, r26097 should fix this up - the log and copy tests now pass.
Thanks! -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: serf property caching
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Justin Erenkrantz wrote:
> On 8/14/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
>> I committed this in r26087.
>
> Sorry I didn't get to it sooner (well, you only let it sit for just
> around 24 hours...so...) - but I really feel that this patch is
> covering up the symptom rather than the actual root cause. Hence, I
> doubt it's the right solution.
Thanks for the feedback, and sorry for the itchy trigger finger. :) If
the solution isn't appropriate, then I'll be happy to revert.
> My initial suggestion is the right thing - if you are looking for
> property X, you need to pass it in to the retrieve_props call. Doing
> otherwise is wrong.
>
> The root cause here is that baseline_url is being set to NULL. That's
> wrong and needs to be further investigated. I'm too swamped right now
> to commit much time to looking deeper, but I really suspect something
> else is going on. Sorry I can't be of more immediate assistance right
> now... -- justin
Unfortunately, I also have other things on my plate, and r26087 is about
the best I can do at this point. The solution seems to require a bit
more knowledge about DAV than I have at the moment. Anybody want to
pick it up from here?
-Hyrum
Re: serf property caching
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 8/14/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
> I committed this in r26087.
Sorry I didn't get to it sooner (well, you only let it sit for just
around 24 hours...so...) - but I really feel that this patch is
covering up the symptom rather than the actual root cause. Hence, I
doubt it's the right solution.
My initial suggestion is the right thing - if you are looking for
property X, you need to pass it in to the retrieve_props call. Doing
otherwise is wrong.
The root cause here is that baseline_url is being set to NULL. That's
wrong and needs to be further investigated. I'm too swamped right now
to commit much time to looking deeper, but I really suspect something
else is going on. Sorry I can't be of more immediate assistance right
now... -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: serf property caching
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Hyrum K. Wright wrote:
> Hyrum K. Wright wrote:
>> Justin Erenkrantz wrote:
>>> On 8/8/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
>> ...
>>>> explicitly asking for it, but rather the set of checked_in_props. (This
>>>> set does *not* contain DAV:href, and I don't know why the initial call
>>>> to retrieve_props() succeeds.) When I use all_props instead of
>>>> checked_in_props, the DAV:href gets pulled from the cache correctly, and
>>>> we don't hit the abort().
>>> Instead of using all_props, I'd just add a new structure to ra_serf.h:
>>>
>>> static const svn_ra_serf__dav_props_t href_props[] =
>>> {
>>> { "DAV:", "href" },
>>> { NULL }
>>> };
>>>
>>> and use href_props rather than all_props.
>> I made this change, and it works. However, other calls to
>> svn_ra_serf__retrieve_props() are failing with a different message:
>> subversion/libsvn_ra_serf/util.c:329: (apr_err=20014)
>> svn: Error running context: Internal error
>
> On second thought, I don't think that this is the correct solution.
> Even though it solves some problems, it After some further digging, and
> thanks to Lieven's analysis elsethread, this is what I've discovered.
>
> Using check_in_props, retrieve_props() caches the correct value for
> DAV:href, but doesn't cache anything for DAV:checked-in. On the second
> call to retrieve_props(), the value for DAV:href doesn't get copied out
> of the cache, which is the reason for the failure. This much we've
> already established.
>
> However, using href_props, we don't get *anything* cached in the first
> place, because the server doesn't recognize the request. So, it returns
> a 404, and subsequent attempts to use the href value fail, leading to
> the serf internal error. The solution to this problem would require
> modifying the server to recognize the new property request.
>
> So, my hacky, and very inelegant solution is attached. It first tries
> to use checked_in_props, and failing that, uses href_props.
>
> -Hyrum
>
> [[[
> Don't fail on multiple calls to svn_ra_serf__retrieve_props() when
> fetching the DAV:href property. Followup to r26036.
>
> * subversion/libsvn_ra_serf/log.c
> (svn_ra_serf__get_log): If retrieving the DAV:href property fails
> when using the checked_in property set, attempt to do so by using the
> href property set.
> ]]]
I committed this in r26087.
-Hyrum
Re: serf property caching
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Hyrum K. Wright wrote:
> Justin Erenkrantz wrote:
>> On 8/8/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
> ...
>>> explicitly asking for it, but rather the set of checked_in_props. (This
>>> set does *not* contain DAV:href, and I don't know why the initial call
>>> to retrieve_props() succeeds.) When I use all_props instead of
>>> checked_in_props, the DAV:href gets pulled from the cache correctly, and
>>> we don't hit the abort().
>> Instead of using all_props, I'd just add a new structure to ra_serf.h:
>>
>> static const svn_ra_serf__dav_props_t href_props[] =
>> {
>> { "DAV:", "href" },
>> { NULL }
>> };
>>
>> and use href_props rather than all_props.
>
> I made this change, and it works. However, other calls to
> svn_ra_serf__retrieve_props() are failing with a different message:
> subversion/libsvn_ra_serf/util.c:329: (apr_err=20014)
> svn: Error running context: Internal error
On second thought, I don't think that this is the correct solution.
Even though it solves some problems, it After some further digging, and
thanks to Lieven's analysis elsethread, this is what I've discovered.
Using check_in_props, retrieve_props() caches the correct value for
DAV:href, but doesn't cache anything for DAV:checked-in. On the second
call to retrieve_props(), the value for DAV:href doesn't get copied out
of the cache, which is the reason for the failure. This much we've
already established.
However, using href_props, we don't get *anything* cached in the first
place, because the server doesn't recognize the request. So, it returns
a 404, and subsequent attempts to use the href value fail, leading to
the serf internal error. The solution to this problem would require
modifying the server to recognize the new property request.
So, my hacky, and very inelegant solution is attached. It first tries
to use checked_in_props, and failing that, uses href_props.
-Hyrum
[[[
Don't fail on multiple calls to svn_ra_serf__retrieve_props() when
fetching the DAV:href property. Followup to r26036.
* subversion/libsvn_ra_serf/log.c
(svn_ra_serf__get_log): If retrieving the DAV:href property fails
when using the checked_in property set, attempt to do so by using the
href property set.
]]]
Re: serf property caching
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Justin Erenkrantz wrote:
> On 8/8/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
...
>> explicitly asking for it, but rather the set of checked_in_props. (This
>> set does *not* contain DAV:href, and I don't know why the initial call
>> to retrieve_props() succeeds.) When I use all_props instead of
>> checked_in_props, the DAV:href gets pulled from the cache correctly, and
>> we don't hit the abort().
>
> Instead of using all_props, I'd just add a new structure to ra_serf.h:
>
> static const svn_ra_serf__dav_props_t href_props[] =
> {
> { "DAV:", "href" },
> { NULL }
> };
>
> and use href_props rather than all_props.
I made this change, and it works. However, other calls to
svn_ra_serf__retrieve_props() are failing with a different message:
subversion/libsvn_ra_serf/util.c:329: (apr_err=20014)
svn: Error running context: Internal error
This looks like some kind of error internal to serf itself. In doing
more digging, it looks to be coming from context.c, around line 870.
Given that information, I think that this is a bug in serf, not in our
usage of it. My serf-fu isn't that great, though, so I'm not sure. I'm
going to commit my patch with Justin's recommended change above, but it
will generate quite a number of new failures. Anybody with a little bit
more knowledge care to debug this new problem?
(FWIW, log_tests 3 is a simple test which easily generates this error.)
-Hyrum
Re: serf property caching
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 8/8/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
> The property we are interested in here is DAV:href, which gets returned
> correctly the first time. The second time it doesn't, because we aren't
The initial property retrieval is not tied to a peg rev, so we can't
necessarily reuse the DAV:href property as the DAV:href may change
between peg revs.
> explicitly asking for it, but rather the set of checked_in_props. (This
> set does *not* contain DAV:href, and I don't know why the initial call
> to retrieve_props() succeeds.) When I use all_props instead of
> checked_in_props, the DAV:href gets pulled from the cache correctly, and
> we don't hit the abort().
Instead of using all_props, I'd just add a new structure to ra_serf.h:
static const svn_ra_serf__dav_props_t href_props[] =
{
{ "DAV:", "href" },
{ NULL }
};
and use href_props rather than all_props.
Other than that minor nit, good catch! -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org