Re: trunk/2.4 core output filter is broken

2012-01-24 Thread Joe Orton
On Mon, Jan 23, 2012 at 09:39:38PM +0100, Stefan Fritsch wrote: This patch allows us to later add members to core_ctx_t without breaking binary compatibility to mod_ftp. Without such a patch, the size of core_ctx_t is part of the ABI, which is bad. Opinions? After thinking about it more:

Re: trunk/2.4 core output filter is broken

2012-01-24 Thread Stefan Fritsch
On Tue, 24 Jan 2012, Joe Orton wrote: On Mon, Jan 23, 2012 at 09:39:38PM +0100, Stefan Fritsch wrote: This patch allows us to later add members to core_ctx_t without breaking binary compatibility to mod_ftp. Without such a patch, the size of core_ctx_t is part of the ABI, which is bad.

Re: trunk/2.4 core output filter is broken

2012-01-24 Thread Jim Jagielski
Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) On Jan 24, 2012, at 4:08 PM, Stefan Fritsch wrote: On Tue, 24 Jan 2012, Joe Orton wrote: On Mon, Jan 23, 2012 at 09:39:38PM

Re: trunk/2.4 core output filter is broken

2012-01-23 Thread Joe Orton
On Sun, Jan 22, 2012 at 12:12:09PM +0100, Stefan Fritsch wrote: On Friday 20 January 2012, Joe Orton wrote: 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

Re: trunk/2.4 core output filter is broken

2012-01-23 Thread Jim Jagielski
On Jan 23, 2012, at 8:32 AM, Joe Orton wrote: Is this something we should still do for 2.4.x (iff 2.4.0 is not released)? Don't know. :) I'm +1 !

Re: trunk/2.4 core output filter is broken

2012-01-23 Thread Stefan Fritsch
On Monday 23 January 2012, William A. Rowe Jr. wrote: On 1/22/2012 10:34 PM, William A. Rowe Jr. wrote: On 1/22/2012 5:12 AM, Stefan Fritsch wrote: On Friday 20 January 2012, Joe Orton wrote: Good catch on ctx-bytes_in. I'd add: why is core_output_filter_ctx_t in a public header?

Re: trunk/2.4 core output filter is broken

2012-01-23 Thread Stefan Fritsch
On Monday 23 January 2012, Joe Orton wrote: On Sun, Jan 22, 2012 at 12:12:09PM +0100, Stefan Fritsch wrote: On Friday 20 January 2012, Joe Orton wrote: If we assume that morphing buckets cannot be buffered, the code could be adjusted to always place them in the to flush segment, and

Re: trunk/2.4 core output filter is broken

2012-01-23 Thread Joe Orton
On Mon, Jan 23, 2012 at 05:15:08PM +0100, Stefan Fritsch wrote: On Monday 23 January 2012, Joe Orton wrote: I think I was not clear enough here: yes, the non-blocking read must be followed by blocking reads. Right, that makes sense. Great. Many eyes on r1234848 and r1234899 rather

Re: trunk/2.4 core output filter is broken

2012-01-23 Thread Stefan Fritsch
On Monday 23 January 2012, Joe Orton wrote: 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

Re: trunk/2.4 core output filter is broken

2012-01-23 Thread William A. Rowe Jr.
On 1/23/2012 2:39 PM, Stefan Fritsch wrote: Opinions? No cycles for review today, but +1 on principal. If you do commit, I'll pick it up into some other testing I'm doing.

Re: trunk/2.4 core output filter is broken

2012-01-23 Thread Stefan Fritsch
On Monday 23 January 2012, William A. Rowe Jr. wrote: On 1/23/2012 2:39 PM, Stefan Fritsch wrote: Opinions? No cycles for review today, but +1 on principal. If you do commit, I'll pick it up into some other testing I'm doing. OK, thanks. It's in trunk now.

Re: trunk/2.4 core output filter is broken

2012-01-23 Thread William A. Rowe Jr.
On 1/23/2012 2:49 PM, William A. Rowe Jr. wrote: On 1/23/2012 2:39 PM, Stefan Fritsch wrote: Opinions? No cycles for review today, but +1 on principal. If you do commit, I'll pick it up into some other testing I'm doing. Thanks to both you, and Joe. I'll check on a sufficiently slow link

Re: trunk/2.4 core output filter is broken

2012-01-22 Thread Stefan Fritsch
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

Re: trunk/2.4 core output filter is broken

2012-01-22 Thread William A. Rowe Jr.
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

Re: trunk/2.4 core output filter is broken

2012-01-22 Thread Stefan Fritsch
On Sunday 22 January 2012, Stefan Fritsch wrote: 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

Re: trunk/2.4 core output filter is broken

2012-01-22 Thread William A. Rowe Jr.
On 1/22/2012 5:12 AM, Stefan Fritsch wrote: On Friday 20 January 2012, Joe Orton wrote: 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

Re: trunk/2.4 core output filter is broken

2012-01-22 Thread William A. Rowe Jr.
On 1/22/2012 10:34 PM, William A. Rowe Jr. wrote: On 1/22/2012 5:12 AM, Stefan Fritsch wrote: On Friday 20 January 2012, Joe Orton wrote: 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

Re: trunk/2.4 core output filter is broken

2012-01-20 Thread Stefan Fritsch
On Friday 20 January 2012, Joe Orton wrote: The main loop in the core output filter (rewritten since 2.2) will try to read the entire passed-in brigade into RAM for CGI/PIPE-like mutating bucket types. :( :( We have trying to bash this kind of bug since 2.0.x days, and now the *core output