Re: mod_ssl: why do we flush on EOS in ssl_io_filter_output()?

[Graham Leggett <mi...@sharp.fm>]

On 28 Oct 2013, at 6:23 PM, Eric Covener <co...@gmail.com> wrote:

>>> It would seem at the very least in order for any kind of write completion to be possible we would need to stop mod_ssl from trying to flush on EOS. Is there a specific problem that mod_ssl tries to solve by doing this?
>> 
>> If you go back to e.g. r91413 and look at the entire file, it seems
>> that the EOS used to flush out some data buffered in openssl:
> 
> Sorry, stray gmail keystrongs, meant to include
> 
> ssl_io_filter_Output()
> ...
>    APR_BRIGADE_FOREACH(bucket, bb) {
>        const char *data;
>        apr_size_t len, n;
> 
>        if (APR_BUCKET_IS_EOS(bucket)) {
>            if ((ret = churn_output(ctx)) != APR_SUCCESS) {
> 
> 
> churn_output()
> ...
>  if (BIO_pending(ctx->pbioWrite)) {
>   ... flush

Looking at the code today, on EOS we call bio_filter_out_flush(), which creates a flush bucket and then gives it to bio_filter_out_pass(), which just ap_pass_brigade()'s it down to the network filter. I am not seeing any openssl functionality being called to flush any openssl state of any kind.

At the very least flush-on-eos is harmful, as it breaks write completion by definition, so it looks like this behaviour needs to go.

Looking a little bit deeper, we find the following:

- The event MPM seems to want to perform write completion on the very last filter in the chain only, which seems completely arbitrary - why should another filter (like mod_ssl) be prevented from running within write completion? Doesn't make sense to me. Ideally we need to teach the event MPM to run write completion on a class of filters, such as AP_FTYPE_NETWORK, and make it a rule that network filters must be aware of and are not allowed to block async MPMs. Given that in theory there is only one AP_FTYPE_NETWORK filter (to my knowledge), this should be backportable to v2.4.
- mod_ssl defines itself as "AP_FTYPE_CONNECTION + 5", which looks to me like mod_ssl really wants to be a network filter, not a connection filter. If we were to teach the event MPM above to trigger network filters, I propose that mod_ssl becomes a network filter following the "not allowed to block async MPMs" rule (which it should do already anyway).

Thoughts?

Regards,
Graham
--

Re: mod_ssl: why do we flush on EOS in ssl_io_filter_output()?

[Graham Leggett <mi...@sharp.fm>]

On 11 Nov 2013, at 12:29 PM, Stefan Fritsch <sf...@sfritsch.de> wrote:

> The filter calls during write completion are done in the worker threads. 
> There is no strict requirement that they must not block.

I had an idea in my head that write completion took place in the listening thread not the worker thread, and I see now that the worker thread handles this, meaning that we can block without a problem.

> Of course, if 
> they block, the advantage of write completion is lost. I think we should 
> stay with this mode for the time being, because it will save a lot of 
> trouble in case of badly behaved filters. Also, if encryption would be 
> done in the async thread, the CPU processing that thread could become a 
> bottleneck.

Agreed.

> In the medium term, we don't only want network filters to become eligible 
> for write completion, but all filters. Think of a large text file being 
> compressed with mod_deflate. Or a large file being processed with 
> mod_include. I had the idea of inserting another buffering filter at the 
> very start (let's call it edge_filter for now), which could then be called 
> on write completion. It could somehow communicate with the 
> core_output_filter to balance how much data is buffered on which end of 
> the filter chain. For example, as long as core_output_filter has data 
> buffered, it would write that with async write completion. When its buffer 
> gets emtpy, it would call edge_filter to send some more data through the 
> whole filter chain, including mod_ssl. If any in-between filter module 
> blocks, then it would no worse than now, without write completion. But I 
> think in many cases, none of the in-between filters would block, and we 
> get write completion for many filters. Of course, so far this is only an 
> idea and I haven't digged into the code to see if it's actually feasible. 
> Do you see any immediate reason why it wouldn't work?

I'm not seeing any immediate reason why it wouldn't work, no.

From what I can see, we could just change "write to the last filter in the stack" with "write to all filters in the stack", and it should work but for one issue: we need to know reliably when all filters have no more buffered data, and so are ready to leave write completion and enter linger.

Right now the core_output_filter keeps a flag to say c->data_in_output_filters, which I changed to a counter when I tried to get mod_ssl involved. I am thinking about misbehaving filters that don't reliably tell us they buffer data, if this flag/counter got out of sync we could see weird errors where data is never sent to the client because it gets clogged in the filters, but the client never sends the next request because the previous response never arrived. I think this problem inspired the "clogging filters" flag that was added to mod_ssl.

Hmmm… thinks.

I notice that the core_output_filter takes no notice of the EOS bucket (which makes sense for a filter that doesn't necessarily care about requests or streams, just data).

Imagine if your edge_filter at the very start flipped the switch and said "from now on, data in the output filters", or in other words it would set c->data_in_output_filters to true and stepped out the way[1].

At the same time, if c->data_in_output_filters was true, the core filter at the very end keeps a lookout for the EOS bucket, and when that EOS bucket is processed the core filter now knows there is no more data in the output filters (if there was, the EOS would not be seen), and the core filter can flip c->data_in_output_filters back to false again.

The event MPM would then react to to change in c->data_in_output_filters as it does now and flip out of WRITE_COMPLETION into LINGER as it should.

This doesn't place any requirements on the filters in between, they can block (we hope they don't, but the wheels don't fall off if they do), they can buffer data, the only requirement is that when the filters are done and the EOS bucket is physically passed upstream there is no buffered data (which is the case now as I understand).

Do you see any reason this wouldn't work?

[1] The edge filter could also perform flow control as you describe, I am thinking specifically about entering and leaving write_completion.

Regards,
Graham
--

Re: mod_ssl: why do we flush on EOS in ssl_io_filter_output()?

[Stefan Fritsch <sf...@sfritsch.de>]

On Mon, 4 Nov 2013, Graham Leggett wrote:
> Looking a little bit deeper, we find the following:
> 
> - The event MPM seems to want to perform write completion on the very 
> last filter in the chain only, which seems completely arbitrary - why 
> should another filter (like mod_ssl) be prevented from running within 
> write completion? Doesn't make sense to me. Ideally we need to teach the 
> event MPM to run write completion on a class of filters, such as 
> AP_FTYPE_NETWORK, and make it a rule that network filters must be aware 
> of and are not allowed to block async MPMs. Given that in theory there 
> is only one AP_FTYPE_NETWORK filter (to my knowledge), this should be 
> backportable to v2.4.

The filter calls during write completion are done in the worker threads. 
There is no strict requirement that they must not block. Of course, if 
they block, the advantage of write completion is lost. I think we should 
stay with this mode for the time being, because it will save a lot of 
trouble in case of badly behaved filters. Also, if encryption would be 
done in the async thread, the CPU processing that thread could become a 
bottleneck.

In the medium term, we don't only want network filters to become eligible 
for write completion, but all filters. Think of a large text file being 
compressed with mod_deflate. Or a large file being processed with 
mod_include. I had the idea of inserting another buffering filter at the 
very start (let's call it edge_filter for now), which could then be called 
on write completion. It could somehow communicate with the 
core_output_filter to balance how much data is buffered on which end of 
the filter chain. For example, as long as core_output_filter has data 
buffered, it would write that with async write completion. When its buffer 
gets emtpy, it would call edge_filter to send some more data through the 
whole filter chain, including mod_ssl. If any in-between filter module 
blocks, then it would no worse than now, without write completion. But I 
think in many cases, none of the in-between filters would block, and we 
get write completion for many filters. Of course, so far this is only an 
idea and I haven't digged into the code to see if it's actually feasible. 
Do you see any immediate reason why it wouldn't work?

> - mod_ssl defines itself as "AP_FTYPE_CONNECTION + 5", which looks to me 
> like mod_ssl really wants to be a network filter, not a connection 
> filter. If we were to teach the event MPM above to trigger network 
> filters, I propose that mod_ssl becomes a network filter following the 
> "not allowed to block async MPMs" rule (which it should do already 
> anyway).

I have no idea ATM about the implications of changing that.

BTW, mod_reqtimeout registers as AP_FTYPE_CONNECTION + 8. But it may make 
sense to integrate that into the core_output_filter.