On Thu, Jul 2, 2020 at 6:25 AM Ruediger Pluem <[email protected]> wrote: > > > > On 7/2/20 11:17 AM, Yann Ylavic wrote: > > On Thu, Jul 2, 2020 at 10:17 AM Ruediger Pluem <[email protected]> wrote: > >> > >> On 7/2/20 2:14 AM, [email protected] 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.
Since these API's never made it out of trunk, I think we can break/fix them without too much worry, especially if it simplifies callers. (sorry Yann for leaving it subtly hosed this way)
