On 1/22/2012 5:12 AM, Stefan Fritsch wrote: > 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)?
As long as we are clear that winnt_mpm manages the socket (because it is responsible for recycling them) and must be able to pass the initial network read heap bucket, it doesn't matter to me how we accomplish this.
