On 7/2/20 11:17 AM, Yann Ylavic wrote:
> On Thu, Jul 2, 2020 at 10:17 AM Ruediger Pluem <rpl...@apache.org> wrote:
>>
>> On 7/2/20 2:14 AM, yla...@apache.org wrote:
>>>
>>> +/* Invoked by the event loop when data is ready on either end.
>>> + * We don't need the invoke_mtx, since we never put multiple callback
>>> events
>>> + * in the queue.
>>> + */
>>> +static void proxy_http_async_cb(void *baton)
>>> +{
>>> + proxy_http_req_t *req = (proxy_http_req_t *)baton;
>>> + int status;
>>> +
>>> + if (req->pfds) {
>>> + apr_pool_clear(req->pfds->pool);
>>> + }
>>> +
>>> + switch (req->state) {
>>> + case PROXY_HTTP_TUNNELING:
>>> + /* Pump both ends until they'd block and then start over again */
>>> + status = ap_proxy_tunnel_run(req->tunnel);
>>> + if (status == HTTP_GATEWAY_TIME_OUT) {
>>> + if (req->pfds) {
>>> + apr_pollfd_t *async_pfds = (void *)req->pfds->elts;
>>> + apr_pollfd_t *tunnel_pfds = (void
>>> *)req->tunnel->pfds->elts;
>>> + async_pfds[0].reqevents = tunnel_pfds[0].reqevents;
>>> + async_pfds[1].reqevents = tunnel_pfds[1].reqevents;
>>
>> What is the purpose of this?
>> async_pfds and tunnel_pfds are local to this block and cannot be used
>> outside this block.
>
> Here and in mod_proxy_wstunnel, the goal is that we have a dedicated
> pfds array for ap_mpm_register_poll_callback_timeout() which modifies
> the array in-place and uses pfds->pool for its own allocations (see
> event_register_poll_callback_ex() in MPM event).
>
> async_pfds and tunnel_pfds are local but point to req->pfds and
> req->tunnel->pfd, this is just to avoid ugly one-line casting.
> I could use the APR_ARRAY_IDX() macro though..
Thanks for explaining. I guess a comment about this in the code could prevent
confusion for future readers :-)
>
>>
>>> + }
>>> + else {
>>> + req->pfds = apr_array_copy(req->p, req->tunnel->pfds);
>>> + apr_pool_create(&req->pfds->pool, req->p);
>>
>> Why first using baton->r->pool to create the copy and then setting the pool
>> of the array to the new pool?
>
> Since event_register_poll_callback_ex() uses pfds->pool for mpm_event
> "internal" allocations, creating and clearing subpool req->pfds->pool
> avoids leaks when proxy_http_async_cb() (and thus
> ap_mpm_register_poll_callback_timeout() below) is called multiple
> times, each time connections are idle and need rescheduling through
> the MPM.
I understand why you do this, but I think it is a dangerous approach. If the
array would do resize operations it would allocate
from a different pool then. I think it is a design flaw of
event_register_poll_callback_ex to allocate stuff from pfds->pool.
It should have a separate pool argument and should use this pool for these
allocations. If callers want to have the same
lifetime of these objects as the array they could simply supply pfds->pool. I
would be even fine if
event_register_poll_callback_ex would accept NULL for this parameter and use
pfds->pool in this case.
Regards
Rüdiger
