Re: [Libevent-users] PATCH: bufferevent_openssl consider_read doesn't drain underlying BE in all cases

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Mon, Dec 19, 2011 at 8:41 PM, Mark Ellzey <mthomas@xxxxxxxxxx> wrote:
> On Mon, Dec 19, 2011 at 07:34:50PM -0600, Mark Ellzey wrote:
>> I think I have been able to reproduce this using ssl client + filters.
>> More on this in a bit.
>
>
> This seems to fix the issue:
>
> -               n_to_read = SSL_pending(bev_ssl->ssl);
> +               if (!(n_to_read = SSL_pending(bev_ssl->ssl)) && bev_ssl->underlying) {
> +                   n_to_read = evbuffer_get_length(bev_ssl->underlying->input);
> +               }
> +

I agree with the part of your fix where we don't ignore the extra data
whose presence is indicated by a nonzero  SSL_pending.

But I think these fixes all have some problem(s) in common:  They
ignore the reason for the logic that initially sets n_to_read, and
instead have the behavior for the filtering SSL bufferevent be "read
aggressively, as much as possible!"

That isn't correct behavior.  Because of rate-limiting[1],
watermarks[2], fairness/starvation[3], and loop-avoidance[4], we
*don't* want to read aggressively from a single underlying bufferevent
until it's out of data.

[1][2] This behavior can make the SSL bufferevent aggressively overrun
rate limits and watermarks.

[3] If a bunch of data has arrived on a the underlying bufferevent, we
probably don't want to aggressively do SSL on it until its gone
without giving other events a chance to fire.

[4] If the underlying bufferevent is a paired bufferevent, draining it
could cause it to refill itself from its partner, invoking callbacks
that might refill the partner, etc.

Instead, I think what we do want to do is make sure that
consider_reading gets called on the *next* iteration of the event
loop, as it would if this were a non-filtering bufferevent.  But I
don't know that there's a good way to do that in the 2.0.x framework.

I think instead our best bet is to try to return the
filtering-bufferevent behavior here as close as we can to the status
quo ante before 2.0.16 came out, and to work on a real correct
solution to this (and other tricky issues) for 2.1, where we can
afford to do stuff like "build new infrastructure" and "refactor all
the ugly code."

Thoughts?

yrs,
-- 
Nick
***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.