Evgeny Kotkov wrote on Wed, Jan 13, 2016 at 17:33:36 +0300: > Daniel Shahaf <d...@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@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
