On Mon, Feb 1, 2021 at 11:24 AM Ruediger Pluem <rpl...@apache.org> wrote:
> > +
> > + should_send_brigade(bb, c, &flush);
> > + if (flush) {
> > + apr_int32_t nfd;
> > + apr_pollfd_t pfd;
> > + memset(&pfd, 0, sizeof(pfd));
> > + pfd.reqevents = APR_POLLOUT;
> > + pfd.desc_type = APR_POLL_SOCKET;
> > + pfd.desc.s = sock;
> > + pfd.p = c->pool;
> > + do {
> > + rv = apr_poll(&pfd, 1, &nfd, sock_timeout);
> > + } while (APR_STATUS_IS_EINTR(rv));
> > }
> > - /*
> > - * Defer the actual blocking write to avoid doing many writes.
> > - */
> > - flush_upto = next;
> > + } while (flush);
>
> Hm, doesn't that loop forever in case the socket does not become writable
> again?
> We don't check the result of the above poll call whether we had an event or
> if we hit the timeout.
> Shouldn't we leave the outer while loop (the while(flush)) if apr_poll
> returns APR_TIMEUP?
> Otherwise I assume that send_brigade_nonblocking will just return
> APR_STATUS_IS_EAGAIN.
Yes correct, good catch.
The attached patch aligns with what we do in trunk (which does not
have this issue), and should fix it.
I should have done it like this from the start rather than diverging
2.4.x more than necessary, since should_send_brigade() is meant to
align with trunk's ap_filter_reinstate_brigade(), with the notable
difference of THRESHOLD_MIN_WRITE (removed from trunk somehow).
With the attached patch, we'd have the below diff on this code
(whitespace changes ignored/mangled):
$ diff -u -w ~trunk/server/core_filters.c ~2.4.x/server/core_filters.c
...
+ if (!new_bb || should_send_brigade(bb, c, NULL)) {
+ apr_socket_t *sock = net->client_socket;
+ apr_interval_time_t sock_timeout = 0;
+
/* Non-blocking writes on the socket in any case. */
apr_socket_timeout_get(sock, &sock_timeout);
apr_socket_timeout_set(sock, 0);
do {
rv = send_brigade_nonblocking(sock, bb, ctx, c);
- if (APR_STATUS_IS_EAGAIN(rv)) {
+ if (new_bb && APR_STATUS_IS_EAGAIN(rv)) {
/* Scan through the brigade and decide whether we must absolutely
- * flush the remaining data, based on ap_filter_reinstate_brigade()
+ * flush the remaining data, based on should_send_brigade()
* rules. If so, wait for writability and retry, otherwise we did
* our best already and can wait for the next call.
*/
- apr_bucket *flush_upto;
- ap_filter_reinstate_brigade(f, bb, &flush_upto);
- if (flush_upto) {
+ int flush;
+ should_send_brigade(bb, c, &flush);
+ if (flush) {
apr_int32_t nfd;
apr_pollfd_t pfd;
memset(&pfd, 0, sizeof(pfd));
@@ -422,6 +534,7 @@
/* Restore original socket timeout before leaving. */
apr_socket_timeout_set(sock, sock_timeout);
+ }
...
>
> Furthermore doesn't that open a way for a Dos if the client only reads a
> single byte shortly
> before the timeout is hit? But I think we had the same problem with the old
> code and we would need
> to have a mod_reqtimeout like check for a minimum rate and probably a maximum
> timeout.
Another story.. reqtimeout can hardly work as an output filter here
since the core_output_filter is the very last one.
Possibly at AP_FTYPE_NETWORK - 1 and with some
ap_filter_should_yield() alike probes we can cook something, but some
optional function(s) called directly by the core may be easier.
Anyway, we miss the whole reqtimeout "outgoing" configuration for now..
Thanks for the review Rüdiger, better late than shipped :)
Regards;
Yann.
Index: server/core_filters.c
===================================================================
--- server/core_filters.c (revision 1886100)
+++ server/core_filters.c (working copy)
@@ -502,7 +502,6 @@ apr_status_t ap_core_output_filter(ap_filter_t *f,
if (!new_bb || should_send_brigade(bb, c, NULL)) {
apr_socket_t *sock = net->client_socket;
apr_interval_time_t sock_timeout = 0;
- int flush;
/* Non-blocking writes on the socket in any case. */
apr_socket_timeout_get(sock, &sock_timeout);
@@ -510,25 +509,29 @@ apr_status_t ap_core_output_filter(ap_filter_t *f,
do {
rv = send_brigade_nonblocking(sock, bb, ctx, c);
- if (!new_bb || !APR_STATUS_IS_EAGAIN(rv)) {
- break;
+ if (new_bb && APR_STATUS_IS_EAGAIN(rv)) {
+ /* Scan through the brigade and decide whether we must absolutely
+ * flush the remaining data, based on should_send_brigade()
+ * rules. If so, wait for writability and retry, otherwise we did
+ * our best already and can wait for the next call.
+ */
+ int flush;
+ should_send_brigade(bb, c, &flush);
+ if (flush) {
+ apr_int32_t nfd;
+ apr_pollfd_t pfd;
+ memset(&pfd, 0, sizeof(pfd));
+ pfd.reqevents = APR_POLLOUT;
+ pfd.desc_type = APR_POLL_SOCKET;
+ pfd.desc.s = sock;
+ pfd.p = c->pool;
+ do {
+ rv = apr_poll(&pfd, 1, &nfd, sock_timeout);
+ } while (APR_STATUS_IS_EINTR(rv));
+ }
}
+ } while (rv == APR_SUCCESS && !APR_BRIGADE_EMPTY(bb));
- should_send_brigade(bb, c, &flush);
- if (flush) {
- apr_int32_t nfd;
- apr_pollfd_t pfd;
- memset(&pfd, 0, sizeof(pfd));
- pfd.reqevents = APR_POLLOUT;
- pfd.desc_type = APR_POLL_SOCKET;
- pfd.desc.s = sock;
- pfd.p = c->pool;
- do {
- rv = apr_poll(&pfd, 1, &nfd, sock_timeout);
- } while (APR_STATUS_IS_EINTR(rv));
- }
- } while (flush);
-
/* Restore original socket timeout before leaving. */
apr_socket_timeout_set(sock, sock_timeout);
}
