Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:
> 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?
[...]
> 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.
I believe that (1) is much harder than (2).
The rewrite of mod_dav pool's usage is a recipe for rarely reproducible
crashes caused by lifetime issues. The functions and callbacks often modify
or attach allocated data to other objects. We'd also need to deal with
per-object subpools like propdb->p, ensure that errors survive up to the
proper point of time, provide different API versions compatible in terms of
the objects' lifetimes, hope that nobody — API users and mod_dav itself —
accidentally relies on something to be allocated in a long-living pool, etc.
And we do have a history of breaking mod_dav (and then reverting changes)
with much smaller fixes.
Frankly, I don't think that this is a realistic approach to the problem.
> 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.
This one-line patch doesn't resolve the root cause of the problem.
As long as we allocate in the request pool on every iteration, the memory
usage would always be unbounded, irrespectively of how we tweak the absolute
values. And even with these tweaks, I still see heavy memory consumption in
my tests (details are included below).
I also think that if we had the necessary aspects of PROPFIND in our control,
we probably wouldn't even get to the point where we find ourselves removing
subpools, saying that they are the root cause of the problem or inspecting
the intrinsics of the APR allocator.
The last thing worth mentioning is that there was no reaction even to this
patch; mod_dav isn't being actively maintained these days. I only counted
about 35 dav-specific commits to modules/dav/main since 2005, and several
of them were reverted afterwards.
Stefan Fuhrmann <stefan2_at_apache.org> writes:
> The "designed" memory usage of mod_dav as in "what gets allocated via palloc"
> is O(total size of all properties). That is unbounded and if you attach
> large properties to each node in a large folder, you may end up with GB
> sized pools. This is a real problem and needs to fixed eventually.
>
> The current situation, however, is that we have O(N^2) allocation *overhead*,
> i.e. memory provided by the APR pool allocator that will never be used.
>
> So, it is not that anybody allocates large structures in long-living pools
> (except for large properties). Instead APR reuses blocks from large,
> short-lived allocations for small long-lived allocations. Preventing
> that already goes a long way mitigating the problems with any reasonable
> setup.
If I replace apr_palloc() with malloc() / free() and patch mod_dav to avoid
creating the propdb->p subpools, I still see inadequate memory consumption.
The environment is close to the one reported by the user, and preparing a
4.5 MB response causes the heap commit size to slowly increase up to 70 MB.
This is a lot, considering a possible amount of parallel requests and *can*
cause problems in any reasonable setup. It could as well be much worse with
other possible environments and data.
> Assuming that your analysis for (1) is correct, I think (2) is a viable
> option. However, I would very much like to see mod_dav fixed as well and
> to have (2) only as a temporary workaround. "Temporary" being a quite
> euphemistic term here ...
[...]
> Does the patch already include the improvements you suggested above?
> Is it feasible to #ifdef the usage of that code? I think if we have
> an easy and clean opt-in / opt-out, people could be more accepting of
> that workaround.
The patch doesn't include these improvements. I can drive the patch up to
a committable state, commit it, and we can enhance these aspects later in
trunk. I attached an updated version of the patch without the regression
tests that were committed in r1724103.
I don't really like the idea of surrounding this code with #ifdef-s, since
I can't see a reason to conditionally turn it off, and not enabling it by
default would leave us with undertested code or code that's never executed.
Regards,
Evgeny Kotkov
Received on 2016-01-13 15:34:19 CET