Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
MIME-Version: 1.0
Date: Sat, 19 Mar 2016 22:05:38 +0100
Message-ID: 
 <CAKQ1sVNb0Rp2r-bph-mQxLMiTkOkn4c+utu5HpdtSrdcW+En5A@mail.gmail.com>
Subject: mod_proxy_hcheck review..
From: Yann Ylavic <ylavic.dev@gmail.com>
To: httpd-dev <dev@httpd.apache.org>
Content-Type: text/plain; charset=UTF-8

The proposed backport patch (v2) does not contain mod_proxy_hcheck.c
itself, so I took the one from trunk, easy enough :)
The (recent) changes around the call to
[ap_proxy_]is_socket_connected() in ap_proxy_connect_backend()) broke
the patch too (minor change).

Regarding the implementation, IIUC, there is a scheduled hc thread per
BalancerMember, each running hc_check().
The threads are somehow run one child at a time, thanks to
mod_watchdog, but I'm not too familiar with it, so I may have
overlooked that...

However, it seems that there are some paths where the worker threads
can allocate on (or use) the server config pool (ctx->p, a subpool of
pconf).
This (AFAICT) concerns hc_get_hcworker(), hc_determine_connection(),
hc_get_backend() (where backend->p == ctx->p ??) and hc_check_http().
Shouldn't we either use a lock or create a subpool there?
Or maybe use ptemp but it looks more like a thread pool, whereas
ctx->p usages seem more related to process wide initialization (once?
via balancer-manager only?)...

Thanks for clarifiying.

Anyway, this is really cool stuff!

Regards,
Yann.