httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fritsch ...@sfritsch.de>
Subject Re: trunk/2.4 core output filter is broken
Date Sun, 22 Jan 2012 11:12:09 GMT
On Friday 20 January 2012, Joe Orton wrote:
> On Fri, Jan 20, 2012 at 09:04:30PM +0100, Stefan Fritsch wrote:
> > This is a bigger problem. With the attached patch, the core
> > output filter will flush data to the client when it has read
> > more than 64K from the cgi bucket. Then it will setaside the
> > remaining part of the passed brigade for later write completion.
> > Here it hits the second part of the problem: ap_save_brigade()
> > will call apr_bucket_read() on bucket types that don't implement
> > the setaside method. And the cgi bucket doesn't.
> > 
> > To me, there seem to be two immediate solutions: Either require
> > setaside being implemented for all bucket types or disable async
> > write completion for requests involving such buckets. Or am I
> > missing something?
> 
> I think you are correct, and I don't see how it is feasible to
> require that setaside "works" for all bucket types, it would be a
> fundamental API change.  Maybe 6 years ago...
> 
> I am struggling to understand this code, I'm sure I am missing
> something too.
> 
> The big loop aims to segregrate the brigade into two parts,
> "buckets which must be written before returning" and "buckets
> which can be buffered".  (Right?)

Yes.

> If we assume that morphing buckets cannot be buffered, the code
> could be adjusted to always place them in the "to flush" segment,
> and then there is no need to read the buckets until they need to
> be sent, as in the patch below.  This seems to fix the memory
> consumption behaviour without obviously breaking anything else
> (cough cough).

I think that your patch is correct. However, as an optimization, one 
could try reading the morphing bucket until there are 
THRESHOLD_MAX_BUFFER bytes in memory. If all morphing buckets in the 
brigade disappear before reaching that limit, one could still do async 
write completion.

FWIW, I don't think it is really necessary to setaside the CGI and 
pipe buckets. They should only depend on the request pool and we 
control the lifetime of that pool via the EOR bucket. However, I don't 
think we can depend on all morphing buckets only depending on the 
request pool. So one would need a custom bucket attribute or a list of 
ok bucket types that don't need to be setaside. This has great 
potential to break things in interesting ways and should be left for 
2.5.x, if it is a good idea at all.

> send_brigade_nonblocking() also needs to be fixed to use a
> non-blocking bucket read, but that is a separate issue.

I don't think so. AFAIK, there is no way for an async mpm to poll on 
more bytes from the producer bucket. If the network to the client is 
faster than the producing cgi, a non blocking read would only put a 
few (or even zero) bytes in the output buffer, the output socket would 
immediately become writable again, and the output filter would be 
called again. This would cause a busy loop, wasting CPU cycles. If my 
assessment is correct, this should go as a comment into 
send_brigade_nonblocking().

> 
> Good catch on ctx->bytes_in. I'd add: why is
> core_output_filter_ctx_t in a public header?

There is no good reason other than that other core filter structs like 
core_filter_ctx and core_net_rec are exposed, too. And those are 
actually used by the winnt MPM. I would prefer if all these structs 
were private and core_filters.c would provide an API to allocate them 
from the winnt MPM and add additional buckets to the input brigade.
Is this something we should still do for 2.4.x (iff 2.4.0 is not 
released)?

Mime
View raw message