Evgeny Kotkov wrote on Tue, Jan 05, 2016 at 19:08:41 +0300:
> Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:
>
> >> I'm aware of two possible ways of solving the problem:
> >>
> >> (1) Fix mod_dav, adjust mod_dav_svn accordingly
> >>
> >> (2) Reimplement PROPFIND in mod_dav_svn
> >>
>
> [...]
>
> > I'm not too happy at basically saying I prefer (1) over (2) when you've
> > already written a 2000-line patch for (2)... but maybe (2) _is_ the
> > better way to go and I just don't see it yet. Enlighten me ☺
>
> I did explore option (1) prior to (2), and spent a reasonable amount of time
> trying to prepare the mod_dav's part of the patch.
>
> Option (1) requires a major overhauling of how mod_dav works with pools, and
> there are just too many things that we'd need to deal with. Basically, every
> relevant pool usage needs to be examined, updated and propagated to the
> mod_dav's public API.
>
I still do not see the problem. mod_dav is not a huge amount of code,
and the work you describe does not sound too hard. Why didn't your
attempt to patch mod_dav succeed?
> Then, even if we pulled that off in httpd, I don't think that these changes
> would become a part of 2.2.x and 2.4.x. And the necessity of waiting or
> upgrading to httpd 2.6.x is a major setback, since I think that, for example,
> the 2.2.x series is still widely adopted.
>
Why wouldn't they become part of 2.2.x?
Stefan's patch is a one-line bugfix patch that applies cleanly, so
anyone supporting httpd-2.2 can easily apply it.
If you're talking about a wider-scale refactoring, mod_dav has had
little change between 2.2.x and 2.6.x (mostly just the additional
argument for ap_log_error/dav_new_error), so backporting a mod_dav patch
to 2.2 would be... at worst, a lot of trivially-resolvable
conflicts; a SMOP, but not a reimplementation.
Even if such a wider-scale refactoring patch would be unfit in
upstream's opinion for an official 2.2.x patch release, it could be
maintained as a fork of httpd-2.2 that has the mod_dav-2.6 changes
backported to it; for example, it could live in
/repos/asf/httpd/httpd/branches/2.2.x-with-mod_dav-extensions, akin to
the javahl-1.8-extensions branch in our tree.
Either way, once the patch has been backported either to 2.2.x proper or
to 2.2.x-with-mod_dav-extensions, users can upgrade their mod_dav to
that (they won't even have to upgrade httpd proper, only mod_dav), and
we won't have to maintain a fork of mod_dav's code in our tree.
[ The above reads "2.2" but refers to both 2.2 and 2.4. ]
> Consuming unbounded amounts of memory or crashing is a serious issue,
> and people report it to us. There's quite a history of plugging PROPFIND
> memory leaks in mod_dav_svn in response to such reports — see [1, 2], for
> instance.
>
> [1] https://issues.apache.org/jira/browse/SVN-2000
> [2] https://svn.apache.org/r859602
Those two links are the same bug, which was fixed by having mod_dav_svn
clear a pool it allocated from. How does this bug support your argument
that we should fork mod_dav's PROPFIND code in mod_dav_svn?
I do see that the only reason dav_resource_private::pool exists is that
mod_dav doesn't pass a scratch_pool argument its hooks.
> However, while the root cause is still there, the problem can be
> re-triggered by a variety of factors. In this particular case, the trigger is
> the replacement of the dynamically-sized caches with a fixed-size membuffer
> cache in 1.7, because now series of cache misses or uncacheable objects can
> lead to performing allocations in the long-living request pool.
So you're saying a mod_dav backend can trigger suboptimal memory
allocation in mod_dav. That should be fixed in mod_dav then, shouldn't
it? Either by allocating memory in a better manner or by forbidding
backends from behaving in that way.
> Compared with (1), option (2) has known complexity and should fix the problem
> irrespectively of the httpd version, and that's why I proposed we do it.
Option (1) does not have "unknown complexity". According to what you
wrote above, it involves converting mod_dav to dual pools. That is
a finite task: for each of the 90 functions and 73 callbacks, at each
callsite, determine what lifetime the object it returns or constructs
needs, find a pool of that lifetime, and pass that pool to the function
or callback.
Option (1) also fixes the problem for users of all httpd versions. It
has a higher bootstrap cost (backporting the patch to 2.2.x will involve
resolving trivial conflicts); however, it also has a lower ongoing
maintenance cost and a smaller chance of introducing bugs (because the
work would be done on httpd's list rather than on our lists).
I think your patch basically forks httpd's code, reindents it, and
improves it. We should instead work with dev_at_httpd to seek a solution
that improves mod_dav in situ.
Daniel
P.S. Your patch probably shouldn't reindent the forked code from httpd's
indent style to svn's indent style, since that'll make merging bugfixes
between the fork and its upstream needlessly harder.
>
> Regards,
> Evgeny Kotkov
>
Received on 2016-01-10 01:37:59 CET