Re: branching over mod_dav 2.4.6 is O(tree)

On Fri, Apr 3, 2015 at 8:23 PM, Ben Reser <ben_at_reser.org> wrote:
> On 4/1/15 5:28 AM, Johan Corveleyn wrote:
>> On Wed, Apr 1, 2015 at 1:21 PM, Philip Martin
>> <philip.martin_at_wandisco.com> wrote:
>>> Yes, I believe so. mod_dav should avoid the walk when the callback
>>> result for every path in the walk is known in advance. I spoke to Ben
>>> yesterday and he said he has an httpd patch that is just about ready.

Hi Ben, thanks for looking into this. I have a few questions / thoughts below.

> So first of all this DAV code has two somewhat overlapping purposes. It
> handles If headers and it handles locks. There is some logic to this since
> lock tokens can be encoded in If headers. However, you can also encode ETags
> as conditions in If headers. For example a DAV client could use an If header
> to only copy something if it had not been modified since it was last aware.
> Until r1476604 (released in 2.2.25 and 2.4.5) mod_dav didn't handle these
> properly for the copy source.
>
> It is necessary to do the walk to handle locks properly since we have to look
> for all the locks and then make sure we received all the lock tokens for them.
> It is not necessary to walk the tree to handle ETag conditions since we are
> given the paths and can just handle them one by one.
>
> So what I've been working on is trying to separate these two things out from
> each other. So that locks and ETags can be handled separately and so that we
> only walk the tree if necessary to check locks.
>
> I'm not quite done with this work.
>
> I'm considering committing the following to httpd today given the attention
> this has gotten:
> [[[
> Index: modules/dav/main/util.c
> ===================================================================
> --- modules/dav/main/util.c (revision 1671097)
> +++ modules/dav/main/util.c (working copy)
> @@ -1595,8 +1595,10 @@
> }
> }
>
> - /* (1) Validate the specified resource, at the specified depth */
> - if (resource->exists && depth > 0) {
> + /* (1) Validate the specified resource, at the specified depth.
> + * Avoid the walk there is no if_header and we aren't planning
> + * to modify this resource. */
> + if (resource->exists && depth > 0 && !(!if_header && flags &
> DAV_VALIDATE_NO_MODIFY)) {
> dav_walker_ctx ctx = { { 0 } };
> dav_response *multi_status;
>
> ]]]
>
> This is worse than the workarounds you have already come up with from an SVN
> only perspective. The workarounds you have come up with would prevent the use
> of ETag If headers on the copy source, but SVN clients don't use those anyway.
> The above patch would not break ETag If headers but SVN would still have the
> penalty of the walk if the client is sending lock tokens for the destination
> (since SVN sends those as If headers). But I think it would at least hide the
> issue from more users in the meantime.

I only know SVN, not much of httpd and certainly not much of DAV. So
I'm reading all this from behind "SVN glasses". In what cases would
SVN send lock tokens (in the form of If headers) when executing a
(server-side) copy?

(I'm wondering: when executing a wc-repos copy ('svn copy $WC $URL')
this issue is also relevant, isn't it? Because such a copy is
internally executed as a copy on the server combined with
modifications from the WC, right? Maybe that's the case where the
client could be sending lock tokens together with the copy?)

Side-question: if the COPY request contains 1 If header with 1 token,
can't DAV locate where that lock token applies in the tree (so as not
to have to crawl it)? This may be a dumb question, I don't really
understand much of all of this :-).

> For SVN only use you can remove the test for the if_header (which I'm not
> comfortable committing to httpd since it just moves the breakage to mod_dav_fs):
> if (resource->exists && depth > 0 && !(flags & DAV_VALIDATE_NO_MODIFY))

This sounds a bit like: from dav we have to do this expensive stuff,
but if you know you're dav_svn, you don't really have to do all of
this, and can decide to take shortcuts, or implement the same
semantics in another way. But you're suggesting modifying the dav
logic in a hard-coded way to do this. Can't this better be done via
some configuration, or a callback?

1) Configuration: directive
DAVDontCheckLocksOnCopyBecauseIHaveItCovered :-) to be used in
combination with DAV svn. Or maybe mod_dav_svn can set this
configuration itself immediately in the right scope?

2) Callback: boolean dav_want_lock_validation_on_copy() or something
like that ...

Also, as hinted by someone else: how strict do we in mod_dav_svn have
to follow the dav spec (to act like a law-abiding dav implementation)?
Didn't we already take steps away from that by implementing our http
v2 protocol?

>> That's great news.
>>
>> Just to be clear: the subject says "mod_dav 2.4.6", but be advised
>> that 2.2.25 and higher are also affected, so I hope that an httpd fix
>> will be backported to 2.2.x as well.
>
> That's my intent. Whatever is done will be backported to 2.2.x and 2.4.x.
> I'll keep an eye open and make sure that the above patch gets committed and
> backported to 2.2.x and 2.4.x if I don't have the more complete refactoring
> done to at least try and limit the situations this comes up in

Great, thanks in any case.

-- 
Johan