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

[Haseeb Abdul Qadir <haseebq@xxxxxxxxxxxxxxx>]

What you guys mean when you say 'build correct infrastructure'? Does it mean the ability to schedule callbacks and guarantee that callbacks will called in the order they've been queued? For example a timer callback with a timeout of 0 is usually enough to enough to defer processing to the next iteration of the event loop on other platforms (i.e Windows' message loop and Cocoa runloops).

On Tuesday, 20 December, 2011 at 7:57 AM, Nick Mathewson wrote:

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.