[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

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



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.