AW: svn commit: r1841225 - /httpd/httpd/trunk/modules/dav/main/props.c
C2 General > -Ursprüngliche Nachricht- > Von: Joe Orton > Gesendet: Dienstag, 25. Juni 2019 13:59 > An: dev@httpd.apache.org > Betreff: Re: svn commit: r1841225 - > /httpd/httpd/trunk/modules/dav/main/props.c > > On Tue, Jun 25, 2019 at 09:06:42AM +, Plüm, Rüdiger, Vodafone Group > wrote: > > > On Tue, Jun 25, 2019 at 08:49:08AM +0100, Joe Orton wrote: > > > > Unless I am missing something the apr_dir_open/apr_dir_read API > design > > > > is always going to have memory use proportional to directory size > > > > because apr_dir_read() duplicates the filename into the > directory's > > > > pool. We need an apr_dir_pread() or whatever which takes a pool > > > > argument. :( > > > > > > OK, yes, this plus an iterpool in dav_fs_walker() fixes it. > > > > Only the iterpool in dav_fs_walker or plus the changes in APR > (apr_dir_pread)? > > Yes, both - PoC attached but I'm not at all confident this is safe, esp > in the recursion case. > > The test I was running before was only a "core" liveprop provided by > mod_dav itself, so I also tested the deadprops case and that still seems > to leak. sbrk() to increase the heap is getting triggered by > apr_sdbm_open() every time I saw it, but apr_dbm_open() is being pass > the scratchpool so I think it's not the cause. I would agree, but can you please provide the stacktrace up to sbrk? That would ease it for me to dive in :-). > > Running "dump_all_pools propdb->p" at this point gives me: > >Pool 'transaction' [0x24e1d88]: 7264/8192 free (1 blocks) allocator: > 0x24d2b90 free blocks in allocator: 0 kiB > Pool 'master_conn' [0x2504e28]: 1224/8192 free (1 blocks) > allocator: 0x24d2b90 free blocks in allocator: 0 kiB >Pool 'deferred_pool' [0x263ef18]: 8032/8192 free (1 blocks) > allocator: 0x24d2b90 free blocks in allocator: 0 kiB > Pool 'request' [0x2506e38]: 13672/1646592 free (201 blocks) > allocator: 0x24d2b90 free blocks in allocator: 0 kiB >Pool 'mod_dav_fs-walker' [0x2524648]: 8016/8192 free (1 > blocks) allocator: 0x24d2b90 free blocks in allocator: 0 kiB >Pool 'mod_dav-scratch' [0x2508e48]: 7224/8192 free (1 blocks) > allocator: 0x24d2b90 free blocks in allocator: 0 kiB >Pool 'mod_lua-vm' [0x250ce68]: 6360/16384 free (2 blocks) > allocator: 0x24d2b90 free blocks in allocator: 0 kiB > > [trimmed some whitespace only] > > I think this is implicating misuse of r->pool, that's grown rather > large? > I would think so as well. Regards Rüdiger
Re: svn commit: r1862014 - /httpd/httpd/trunk/modules/proxy/mod_proxy_hcheck.c
On 24/06/2019 21:13, Christophe JAILLET wrote:
> Le 24/06/2019 à 18:29, jfcl...@apache.org a écrit :
>> Author: jfclere
>> Date: Mon Jun 24 16:29:22 2019
>> New Revision: 1862014
>>
>> URL: http://svn.apache.org/viewvc?rev=1862014=rev
>> Log:
>> Set connectiontimeout for mod_proxy_hcheck.
>> Fix for https://issues.jboss.org/browse/JBCS-448
>>
>> Modified:
>> httpd/httpd/trunk/modules/proxy/mod_proxy_hcheck.c
>>
>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_hcheck.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_hcheck.c?rev=1862014=1862013=1862014=diff
>>
>> ==
>>
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_hcheck.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_hcheck.c Mon Jun 24
>> 16:29:22 2019
>> @@ -487,6 +487,10 @@ static proxy_worker *hc_get_hcworker(sct
>> hc->hash.def = hc->s->hash.def =
>> ap_proxy_hashfunc(hc->s->name, PROXY_HASHFUNC_DEFAULT);
>> hc->hash.fnv = hc->s->hash.fnv =
>> ap_proxy_hashfunc(hc->s->name, PROXY_HASHFUNC_FNV);
>> hc->s->port = port;
>> + if (worker->s->conn_timeout_set) {
>> + hc->s->conn_timeout_set = worker->s->conn_timeout_set;
>> + hc->s->conn_timeout = worker->s->conn_timeout;
>> + }
>> /* Do not disable worker in case of errors */
>> hc->s->status |= PROXY_WORKER_IGNORE_ERRORS;
>> /* Mark as the "generic" worker */
>
>
> Hi, a similar approach is available in
> https://bz.apache.org/bugzilla/show_bug.cgi?id=60948.
>
> This BZ also extends templates and makes this new connection time-out
> configurable.
>
> CJ
>
>
>
>
Thanks I missed it... Now trying to understand why we need 2 connection
timeouts? We are checking the connection to the backend , the backend
should behave the same way for connection... Did I miss something?
--
Cheers
Jean-Frederic
Re: svn commit: r1841225 - /httpd/httpd/trunk/modules/dav/main/props.c
On Tue, Jun 25, 2019 at 09:06:42AM +, Plüm, Rüdiger, Vodafone Group wrote:
> > On Tue, Jun 25, 2019 at 08:49:08AM +0100, Joe Orton wrote:
> > > Unless I am missing something the apr_dir_open/apr_dir_read API design
> > > is always going to have memory use proportional to directory size
> > > because apr_dir_read() duplicates the filename into the directory's
> > > pool. We need an apr_dir_pread() or whatever which takes a pool
> > > argument. :(
> >
> > OK, yes, this plus an iterpool in dav_fs_walker() fixes it.
>
> Only the iterpool in dav_fs_walker or plus the changes in APR (apr_dir_pread)?
Yes, both - PoC attached but I'm not at all confident this is safe, esp
in the recursion case.
The test I was running before was only a "core" liveprop provided by
mod_dav itself, so I also tested the deadprops case and that still seems
to leak. sbrk() to increase the heap is getting triggered by
apr_sdbm_open() every time I saw it, but apr_dbm_open() is being pass
the scratchpool so I think it's not the cause.
Running "dump_all_pools propdb->p" at this point gives me:
Pool 'transaction' [0x24e1d88]: 7264/8192 free (1 blocks) allocator:
0x24d2b90 free blocks in allocator: 0 kiB
Pool 'master_conn' [0x2504e28]: 1224/8192 free (1 blocks) allocator:
0x24d2b90 free blocks in allocator: 0 kiB
Pool 'deferred_pool' [0x263ef18]: 8032/8192 free (1 blocks) allocator:
0x24d2b90 free blocks in allocator: 0 kiB
Pool 'request' [0x2506e38]: 13672/1646592 free (201 blocks) allocator:
0x24d2b90 free blocks in allocator: 0 kiB
Pool 'mod_dav_fs-walker' [0x2524648]: 8016/8192 free (1 blocks)
allocator: 0x24d2b90 free blocks in allocator: 0 kiB
Pool 'mod_dav-scratch' [0x2508e48]: 7224/8192 free (1 blocks)
allocator: 0x24d2b90 free blocks in allocator: 0 kiB
Pool 'mod_lua-vm' [0x250ce68]: 6360/16384 free (2 blocks) allocator:
0x24d2b90 free blocks in allocator: 0 kiB
[trimmed some whitespace only]
I think this is implicating misuse of r->pool, that's grown rather
large?
Regards, Joe
Index: include/apr_file_info.h
===
--- include/apr_file_info.h (revision 1862036)
+++ include/apr_file_info.h (working copy)
@@ -268,6 +268,24 @@
apr_dir_t *thedir);
/**
+ * Read the next entry from the specified directory.
+ * @param finfo the file info structure and filled in by apr_dir_read
+ * @param wanted The desired apr_finfo_t fields, as a bit flag of APR_FINFO_
+ values
+ * @param thedir the directory descriptor returned from apr_dir_open
+ * @param pool the pool to use for allocations
+ * @remark No ordering is guaranteed for the entries read.
+ *
+ * @note If @c APR_INCOMPLETE is returned all the fields in @a finfo may
+ * not be filled in, and you need to check the @c finfo->valid bitmask
+ * to verify that what you're looking for is there. When no more
+ * entries are available, APR_ENOENT is returned.
+ */
+APR_DECLARE(apr_status_t) apr_dir_pread(apr_finfo_t *finfo, apr_int32_t wanted,
+apr_dir_t *thedir, apr_pool_t *pool);
+
+
+/**
* Rewind the directory to the first entry.
* @param thedir the directory descriptor to rewind.
*/
Index: file_io/unix/dir.c
===
--- file_io/unix/dir.c (revision 1862036)
+++ file_io/unix/dir.c (working copy)
@@ -133,6 +133,12 @@
apr_status_t apr_dir_read(apr_finfo_t *finfo, apr_int32_t wanted,
apr_dir_t *thedir)
{
+return apr_dir_pread(finfo, wanted, thedir, thedir->pool);
+}
+
+apr_status_t apr_dir_pread(apr_finfo_t *finfo, apr_int32_t wanted,
+ apr_dir_t *thedir, apr_pool_t *pool)
+{
apr_status_t ret = 0;
#ifdef DIRENT_TYPE
apr_filetype_e type;
@@ -242,7 +248,7 @@
apr_cpystrn(end, thedir->entry->d_name,
sizeof fspec - (end - fspec));
-ret = apr_stat(finfo, fspec, APR_FINFO_LINK | wanted, thedir->pool);
+ret = apr_stat(finfo, fspec, APR_FINFO_LINK | wanted, pool);
/* We passed a stack name that will disappear */
finfo->fname = NULL;
}
@@ -254,7 +260,7 @@
/* We don't bail because we fail to stat, when we are only -required-
* to readdir... but the result will be APR_INCOMPLETE
*/
-finfo->pool = thedir->pool;
+finfo->pool = pool;
finfo->valid = 0;
#ifdef DIRENT_TYPE
if (type != APR_UNKFILE) {
@@ -270,7 +276,7 @@
#endif
}
-finfo->name = apr_pstrdup(thedir->pool, thedir->entry->d_name);
+finfo->name = apr_pstrdup(pool, thedir->entry->d_name);
finfo->valid |= APR_FINFO_NAME;
if (wanted)
Index: modules/dav/fs/repos.c
===
---
AW: svn commit: r1841225 - /httpd/httpd/trunk/modules/dav/main/props.c
C2 General > -Ursprüngliche Nachricht- > Von: Joe Orton > Gesendet: Dienstag, 25. Juni 2019 10:45 > An: dev@httpd.apache.org > Betreff: Re: svn commit: r1841225 - > /httpd/httpd/trunk/modules/dav/main/props.c > > On Tue, Jun 25, 2019 at 08:49:08AM +0100, Joe Orton wrote: > > Unless I am missing something the apr_dir_open/apr_dir_read API design > > is always going to have memory use proportional to directory size > > because apr_dir_read() duplicates the filename into the directory's > > pool. We need an apr_dir_pread() or whatever which takes a pool > > argument. :( > > OK, yes, this plus an iterpool in dav_fs_walker() fixes it. Only the iterpool in dav_fs_walker or plus the changes in APR (apr_dir_pread)? > > I've been using the python "psrecord" (from pip) to check this rather > than my traditional method of "eyeballing top", here are the charts for > running 10x a PROPFIND with ~100K files, for Great plots. Thanks for pointing to the tool. > > a) trunk - steep gradient, ~20mb consumed > b) trunk plus the patch from my previous post, ~5mb? > c) (b) plus iterpool in dav_fs_walker - flat > > so I think (b) is fine to commit but with the recursion in dav_fs_walker > I am not at all sure this stuff is safe yet, will need some more > testing. The recursion stuff could be tricky. Probably a separate (sub-)pool for apr_dir_open on each recursion level and separate iterpools on each recursion level? Regards Rüdiger
cmake help wanted
I added the cmake definitions for mod_md in r1862041. If someone could find the time to run this on Windows in the coming weeks, that'd be great. - Stefan
Re: svn commit: r1841225 - /httpd/httpd/trunk/modules/dav/main/props.c
Nice work! > Am 25.06.2019 um 10:44 schrieb Joe Orton : > >
Re: svn commit: r1841225 - /httpd/httpd/trunk/modules/dav/main/props.c
On Tue, Jun 25, 2019 at 08:49:08AM +0100, Joe Orton wrote: > Unless I am missing something the apr_dir_open/apr_dir_read API design > is always going to have memory use proportional to directory size > because apr_dir_read() duplicates the filename into the directory's > pool. We need an apr_dir_pread() or whatever which takes a pool > argument. :( OK, yes, this plus an iterpool in dav_fs_walker() fixes it. I've been using the python "psrecord" (from pip) to check this rather than my traditional method of "eyeballing top", here are the charts for running 10x a PROPFIND with ~100K files, for a) trunk - steep gradient, ~20mb consumed b) trunk plus the patch from my previous post, ~5mb? c) (b) plus iterpool in dav_fs_walker - flat so I think (b) is fine to commit but with the recursion in dav_fs_walker I am not at all sure this stuff is safe yet, will need some more testing.
AW: svn commit: r1841225 - /httpd/httpd/trunk/modules/dav/main/props.c
C2 General > -Ursprüngliche Nachricht- > Von: Joe Orton > Gesendet: Dienstag, 25. Juni 2019 09:49 > An: dev@httpd.apache.org > Betreff: Re: svn commit: r1841225 - > /httpd/httpd/trunk/modules/dav/main/props.c > > On Mon, Jun 24, 2019 at 09:54:21PM +0200, Ruediger Pluem wrote: > > On 06/24/2019 07:04 PM, Joe Orton wrote: > > > #11 0x7f8110ce6448 in dav_do_prop_subreq (propdb=0x100cfc0) at > props.c:331 > > > > What if you go here and do a > > > > dump_all_pools propdb->p > > > > and do it again when you hit the next sbrk and the next one and so on? > Something suspicious? > > Well I did this and immediately noticed something suspicious on the > previous line, e_uri is allocated from resource->pool (=r->pool) rather > than propbb->p, so, yet again the precision accuracy of your insights is Very good catch. Great that you found it. > beyond belief Ruediger :) :) > > This fixed a significant leak, but r->pool is still growing. I think > the remaining problem is use of r->pool in dav_fs_walker, i.e. is > mod_dav_fs specific. Do you think it is now good enough to commit at least as a first step? > > Unless I am missing something the apr_dir_open/apr_dir_read API design > is always going to have memory use proportional to directory size > because apr_dir_read() duplicates the filename into the directory's > pool. We need an apr_dir_pread() or whatever which takes a pool > argument. :( In order to fix this leak, I agree that we would need this. The pool is also used for apr_stat inside apr_dir_read, but I fail to see how it consumes memory. Another related question: Do you think it would be beneficial if we replace the apr_psprintf with apr_pstrcat in props.c as long as they only concat strings without further formatting? Regards Rüdiger
Re: svn commit: r1841225 - /httpd/httpd/trunk/modules/dav/main/props.c
On Mon, Jun 24, 2019 at 09:54:21PM +0200, Ruediger Pluem wrote:
> On 06/24/2019 07:04 PM, Joe Orton wrote:
> > #11 0x7f8110ce6448 in dav_do_prop_subreq (propdb=0x100cfc0) at
> > props.c:331
>
> What if you go here and do a
>
> dump_all_pools propdb->p
>
> and do it again when you hit the next sbrk and the next one and so on?
> Something suspicious?
Well I did this and immediately noticed something suspicious on the
previous line, e_uri is allocated from resource->pool (=r->pool) rather
than propbb->p, so, yet again the precision accuracy of your insights is
beyond belief Ruediger :) :)
This fixed a significant leak, but r->pool is still growing. I think
the remaining problem is use of r->pool in dav_fs_walker, i.e. is
mod_dav_fs specific.
Unless I am missing something the apr_dir_open/apr_dir_read API design
is always going to have memory use proportional to directory size
because apr_dir_read() duplicates the filename into the directory's
pool. We need an apr_dir_pread() or whatever which takes a pool
argument. :(
Index: modules/dav/main/mod_dav.c
===
--- modules/dav/main/mod_dav.c (revision 1861995)
+++ modules/dav/main/mod_dav.c (working copy)
@@ -563,6 +563,7 @@
dav_begin_multistatus(bb, r, status, namespaces);
apr_pool_create(, r->pool);
+apr_pool_tag(subpool, "mod_dav-multistatus");
for (; first != NULL; first = first->next) {
apr_pool_clear(subpool);
@@ -2027,8 +2028,9 @@
** Note: we cast to lose the "const". The propdb won't try to change
** the resource, however, since we are opening readonly.
*/
-err = dav_open_propdb(ctx->r, ctx->w.lockdb, wres->resource, 1,
- ctx->doc ? ctx->doc->namespaces : NULL, );
+err = dav_popen_propdb(ctx->scratchpool,
+ ctx->r, ctx->w.lockdb, wres->resource, 1,
+ ctx->doc ? ctx->doc->namespaces : NULL, );
if (err != NULL) {
/* ### do something with err! */
Index: modules/dav/main/mod_dav.h
===
--- modules/dav/main/mod_dav.h (revision 1861995)
+++ modules/dav/main/mod_dav.h (working copy)
@@ -1596,6 +1596,16 @@
apr_array_header_t *ns_xlate,
dav_propdb **propdb);
+DAV_DECLARE(dav_error *) dav_popen_propdb(
+apr_pool_t *p,
+request_rec *r,
+dav_lockdb *lockdb,
+const dav_resource *resource,
+int ro,
+apr_array_header_t *ns_xlate,
+dav_propdb **propdb);
+
+
DAV_DECLARE(void) dav_close_propdb(dav_propdb *db);
DAV_DECLARE(dav_get_props_result) dav_get_props(
Index: modules/dav/main/props.c
===
--- modules/dav/main/props.c(revision 1861995)
+++ modules/dav/main/props.c(working copy)
@@ -323,7 +323,7 @@
{
/* need to escape the uri that's in the resource struct because during
* the property walker it's not encoded. */
-const char *e_uri = ap_escape_uri(propdb->resource->pool,
+const char *e_uri = ap_escape_uri(propdb->p,
propdb->resource->uri);
/* perform a "GET" on the resource's URI (note that the resource
@@ -524,22 +524,21 @@
apr_array_header_t * ns_xlate,
dav_propdb **p_propdb)
{
+return dav_popen_propdb(r->pool, r, lockdb, resource, ro, ns_xlate,
p_propdb);
+}
+
+DAV_DECLARE(dav_error *)dav_popen_propdb(apr_pool_t *p,
+ request_rec *r, dav_lockdb *lockdb,
+ const dav_resource *resource,
+ int ro,
+ apr_array_header_t * ns_xlate,
+ dav_propdb **p_propdb)
+{
dav_propdb *propdb = NULL;
-/*
- * Check if we have tucked away a previous propdb and reuse it.
- * Otherwise create a new one and tuck it away
- */
-apr_pool_userdata_get((void **), "propdb", r->pool);
-if (!propdb) {
-propdb = apr_pcalloc(r->pool, sizeof(*propdb));
-apr_pool_userdata_setn(propdb, "propdb", NULL, r->pool);
-apr_pool_create(>p, r->pool);
-}
-else {
-/* Play safe and clear the pool of the reused probdb */
-apr_pool_clear(propdb->p);
-}
+propdb = apr_pcalloc(p, sizeof(*propdb));
+propdb->p = p;
+
*p_propdb = NULL;
#if DAV_DEBUG
@@ -579,8 +578,6 @@
ap_destroy_sub_req(propdb->subreq);
propdb->subreq = NULL;
}
-
-apr_pool_clear(propdb->p);
}
DAV_DECLARE(dav_get_props_result) dav_get_allprops(dav_propdb *propdb,
