On 2/2/21 2:58 PM, Yann Ylavic wrote:
> 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 agree that it should fix it. Another nitpick I came across when reviewing
this patch:
Shouldn't should_send_brigade return an int instead of apr_status_t?
Regards
Rüdiger
