On Fri, Jul 10, 2020 at 9:55 AM Ruediger Pluem <[email protected]> wrote: > > On 7/1/20 6:35 PM, [email protected] wrote: > > Author: ylavic > > Date: Wed Jul 1 16:35:48 2020 > > New Revision: 1879401 > > > > for (i = 0; i < nresults; i++) { > > - const apr_pollfd_t *cur = &results[i]; > > - int revents = cur->rtnevents; > > + const apr_pollfd_t *pfd = &results[i]; > > + struct proxy_tunnel_conn *tc = pfd->client_data; > > + > > + ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r, > > + "proxy: %s: #%i: %s: %hx/%hx", scheme, i, > > + tc->name, pfd->rtnevents, tc->pfd->reqevents); > > > > /* sanity check */ > > - if (cur->desc.s != client->pfd->desc.s > > - && cur->desc.s != origin->pfd->desc.s) { > > + if (pfd->desc.s != client->pfd->desc.s > > + && pfd->desc.s != origin->pfd->desc.s) { > > ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10222) > > "proxy: %s: unknown socket in pollset", > > scheme); > > rc = HTTP_INTERNAL_SERVER_ERROR; > > goto cleanup; > > } > > - > > - in = cur->client_data; > > - if (revents & APR_POLLOUT) { > > - in = in->other; > > - } > > - else if (!(revents & (APR_POLLIN | APR_POLLHUP))) { > > + if (!(pfd->rtnevents & (APR_POLLIN | APR_POLLHUP | > > APR_POLLOUT))) { > > /* this catches POLLERR/POLLNVAL etc.. */ > > ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10220) > > "proxy: %s: polling events error (%x)", > > - scheme, revents); > > + scheme, pfd->rtnevents); > > rc = HTTP_INTERNAL_SERVER_ERROR; > > goto cleanup; > > } > > - out = in->other; > > > > - ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r, > > - "proxy: %s: #%i: %s/%hx => %s/%hx: %x", > > - scheme, i, in->name, in->pfd->reqevents, > > - out->name, out->pfd->reqevents, revents); > > + if (pfd->rtnevents & APR_POLLOUT) { > > + struct proxy_tunnel_conn *out = tc, *in = tc->other; > > + > > + ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r, > > + "proxy: %s: %s output ready", > > + scheme, out->name); > > + > > + rc = ap_filter_output_pending(out->c); > > + if (rc == OK) { > > + /* Keep polling out (only) */ > > + continue; > > + } > > + if (rc != DECLINED) { > > + /* Real failure, bail out */ > > + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, > > APLOGNO(10221) > > + "proxy: %s: %s flushing failed (%i)", > > + scheme, out->name, rc); > > + goto cleanup; > > + } > > + rc = OK; > > + > > + /* No more pending data. If the input side is not readable > > + * anymore it's time to shutdown for write (this direction > > + * is over). Otherwise back to normal business. > > + */ > > + del_pollset(pollset, out->pfd, APR_POLLOUT); > > + if (in->readable) { > > + ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r, > > + "proxy: %s: %s resume writable", > > + scheme, out->name); > > + add_pollset(pollset, in->pfd, APR_POLLIN); > > + out->writable = 1; > > + } > > + else { > > + ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r, > > + "proxy: %s: %s write shutdown", > > + scheme, out->name); > > + apr_socket_shutdown(out->pfd->desc.s, 1); > > + } > > + } > > > > - if (in->readable && (in->drain || !(revents & APR_POLLOUT))) { > > + if (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP) > > Why do we check for APR_POLLHUP here as well? I noticed a case where we > request only APR_POLLOUT but get back APR_POLLOUT and > APR_POLLHUB which causes an endless loop.
Hmm, POLLHUP and POLLOUT are mutually exclusive. When requesting for POLLOUT only, I don't think we can get POLLHUP alone either, because even if the peer shut down (FIN) the connection we should still be able to write, and a RST would cause POLLERR (presumably since a read() would return an error, as opposed to POLLHUP's read() would return EOF). Is the loop you are thinking about when we get POLLHUP (alone, so don't enter "flushing" in POLLOUT code) and then ap_proxy_transfer_between_connections() does nothing (because of pending output data)? As explained above, I don't think it's a possible state, but maybe we should handle it as POLLERR just in case.. Regards; Yann.
