Evgeny Kotkov wrote on Wed, Jan 13, 2016 at 17:33:36 +0300:
> 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.
>
> Frankly, I don't think that this is a realistic approach to the problem.
>
All the problems you describe are generic problems that apply to *any*
rewrite of *any* C codebase to change its dynamic memory allocation
patterns. Your argument could be applied to *any* case where somebody
proposes to change some library's object lifetime patterns. It implies
that it is impossible or infeasible to patch a library to have tighter
lifetime promises.
> And we do have a history of breaking mod_dav (and then reverting changes)
> with much smaller fixes.
Would you be able to cite concrete examples?
> > 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.
>
The "number of commits" statistic of a project is not a sensitive test
of its actively-maintainedness: a project may have few commits but be
actively maintained. A better measure is whether "DEFECT"-kind bugs are
being addressed timely. To my recollection, every bug we found in
mod_dav has been addressed. (Admittedly, some of them were addressed by
an svn developer who earned httpd PMC membership in the process
(breser).)
>
>
> Regards,
> Evgeny Kotkov
So, to summarize:
My understanding is that you disqualified / ruled out the "patch mod_dav"
approach because mod_dav is a library. (I might have misunderstood, of
course, in which case I'd appreciate being corrected.)
I think forking mod_dav into mod_dav_svn would be a mistake.
I think you should patch mod_dav.
Can I help with the latter approach? E.g., by reaching out to the
dev_at_httpd folks, or by coding / testing / reviewing a patch? I will
have limited time for this since it's not part of my dayjob.
How do we move forward?
Daniel
Received on 2016-01-14 01:13:44 CET