You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2012/01/20 17:52:10 UTC
trunk/2.4 core output filter is broken
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 filter* itself is doing it, yegads.
The fix may be as simple as adding an appropriate "break" to the loop,
I'm not sure yet. It should be doing the non-block read, FLUSH & retry
thing as well, so that streaming from slow generators works properly.
prefork/mod_cgi, serve this as "nph-killme.pl" and wait for the
fireworks:
!/usr/bin/perl
print "HTTP/1.0 200 OK\r\n";
print "Transfer-Encoding: chunked\r\n";
print "\r\n";
print "5\r\nhello\r\n" while (1);
Re: trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
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
and fast box to see if we continue trigger this acceptfilter none
nonblocking exception to mod_ssl.
Re: trunk/2.4 core output filter is broken
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
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: [PATCH] trunk/2.4 core output filter is broken
Posted by Jim Jagielski <ji...@jaguNET.com>.
FanTAStic!!!
On Feb 4, 2012, at 5:11 AM, Stefan Fritsch wrote:
> On Saturday 04 February 2012, Gregg Smith wrote:
>
>> This fixes the crash, thanks.
>
> Thanks. Committed to trunk and 2.4
>
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Saturday 04 February 2012, Gregg Smith wrote:
> This fixes the crash, thanks.
Thanks. Committed to trunk and 2.4
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
On 2/3/2012 10:43 AM, Stefan Fritsch wrote:
> On Thursday 02 February 2012, William A. Rowe Jr. wrote:
>> On 2/2/2012 8:36 AM, Jim Jagielski wrote:
>>> bb == NULL ??
>> Looking at his attached screen scrape; no. Which leaves with
>> something like e == NULL or a broken bb.
> The former :-(
>
> Gregg, please add this or try the attached patch which is against
> current trunk and includes all fixes so far. Thanks in advance.
>
> --- a/server/mpm/winnt/child.c
> +++ b/server/mpm/winnt/child.c
> @@ -743,11 +743,10 @@ apr_status_t
> winnt_insert_network_bucket(conn_rec *c,
> apr_bucket *e;
> winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config,
> &mpm_winnt_module);
> - if (context == NULL)
> + if (context == NULL || (e = context->overlapped.Pointer) == NULL)
> return DECLINED;
>
> /* seed the brigade with AcceptEx read heap bucket */
> - e = context->overlapped.Pointer;
> APR_BRIGADE_INSERT_HEAD(bb, e);
> /* also seed the brigade with the client socket. */
> e = apr_bucket_socket_create(socket, c->bucket_alloc);
Stefan,
This fixes the crash, thanks.
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Thursday 02 February 2012, William A. Rowe Jr. wrote:
> On 2/2/2012 8:36 AM, Jim Jagielski wrote:
> > bb == NULL ??
>
> Looking at his attached screen scrape; no. Which leaves with
> something like e == NULL or a broken bb.
The former :-(
Gregg, please add this or try the attached patch which is against
current trunk and includes all fixes so far. Thanks in advance.
--- a/server/mpm/winnt/child.c
+++ b/server/mpm/winnt/child.c
@@ -743,11 +743,10 @@ apr_status_t
winnt_insert_network_bucket(conn_rec *c,
apr_bucket *e;
winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config,
&mpm_winnt_module);
- if (context == NULL)
+ if (context == NULL || (e = context->overlapped.Pointer) == NULL)
return DECLINED;
/* seed the brigade with AcceptEx read heap bucket */
- e = context->overlapped.Pointer;
APR_BRIGADE_INSERT_HEAD(bb, e);
/* also seed the brigade with the client socket. */
e = apr_bucket_socket_create(socket, c->bucket_alloc);
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 2/2/2012 8:36 AM, Jim Jagielski wrote:
> bb == NULL ??
Looking at his attached screen scrape; no. Which leaves with something
like e == NULL or a broken bb.
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Jim Jagielski <ji...@jaguNET.com>.
bb == NULL ??
On Feb 2, 2012, at 3:40 AM, Gregg Smith wrote:
> Stefan,
>
> On 2/1/2012 12:44 PM, Stefan Fritsch wrote:
>> Try removing the "static":
>>
>> --- a/server/mpm/winnt/child.c
>> +++ b/server/mpm/winnt/child.c
>> @@ -736,9 +736,9 @@ static winnt_conn_ctx_t
>> *winnt_get_connection(winnt_conn_ctx_t *context)
>> return context;
>> }
>>
>> -static apr_status_t winnt_insert_network_bucket(conn_rec *c,
>> - apr_bucket_brigade
>> *bb,
>> - apr_socket_t *socket)
>> +apr_status_t winnt_insert_network_bucket(conn_rec *c,
>> + apr_bucket_brigade *bb,
>> + apr_socket_t *socket)
>> {
>> apr_bucket *e;
>> winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config
>
> That builds now, but one step forward, two steps back.
>
> libhttpd crashes on any request with
>
> AcceptFilter http none
> or
> AcceptFilter http connect
>
> The default AcceptFilter http data works fine. I did not try https.
>
> Regards,
>
> Gregg
>
> <acfilter_crash.png>
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
Stefan,
On 2/1/2012 12:44 PM, Stefan Fritsch wrote:
> Try removing the "static":
>
> --- a/server/mpm/winnt/child.c
> +++ b/server/mpm/winnt/child.c
> @@ -736,9 +736,9 @@ static winnt_conn_ctx_t
> *winnt_get_connection(winnt_conn_ctx_t *context)
> return context;
> }
>
> -static apr_status_t winnt_insert_network_bucket(conn_rec *c,
> - apr_bucket_brigade
> *bb,
> - apr_socket_t *socket)
> +apr_status_t winnt_insert_network_bucket(conn_rec *c,
> + apr_bucket_brigade *bb,
> + apr_socket_t *socket)
> {
> apr_bucket *e;
> winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config
That builds now, but one step forward, two steps back.
libhttpd crashes on any request with
AcceptFilter http none
or
AcceptFilter http connect
The default AcceptFilter http data works fine. I did not try https.
Regards,
Gregg
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Wednesday 01 February 2012, Gregg Smith wrote:
> Getting closer
>
> Linking...
> Creating library .\Release/libhttpd.lib and object
> .\Release/libhttpd.exp
> mpm_winnt.obj : error LNK2019: unresolved external symbol
> _winnt_insert_network_bucket referenced in function _winnt_hooks
>
> I don't quite get this error though.
Having a Windows compiler myself would really help :-/
Try removing the "static":
--- a/server/mpm/winnt/child.c
+++ b/server/mpm/winnt/child.c
@@ -736,9 +736,9 @@ static winnt_conn_ctx_t
*winnt_get_connection(winnt_conn_ctx_t *context)
return context;
}
-static apr_status_t winnt_insert_network_bucket(conn_rec *c,
- apr_bucket_brigade
*bb,
- apr_socket_t *socket)
+apr_status_t winnt_insert_network_bucket(conn_rec *c,
+ apr_bucket_brigade *bb,
+ apr_socket_t *socket)
{
apr_bucket *e;
winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config,
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
On 1/31/2012 1:30 PM, Stefan Fritsch wrote:
> On Tuesday 31 January 2012, Gregg Smith wrote:
>> On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
>>> On Wednesday 25 January 2012, Jim Jagielski wrote:
>>>> 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?)
>>> OK, patch is attached. This needs review/testing for Windows.
>> Patched trunk at r1237447, saw same after patching 2.4-HEAD so I
>> tried trunk.
>>
>> Regards,
>> Gregg
>>
>> Compiling...
>> service.c
>> .\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module'
>> : redefinition; different storage class
>>
>> c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
>>
>> : see declaration of 'mpm_winnt_module'
> Oops. Please try this in addition. Thanks for the testing.
>
> --- a/server/mpm/winnt/mpm_winnt.h
> +++ b/server/mpm/winnt/mpm_winnt.h
> @@ -67,7 +67,7 @@ void mpm_nt_eventlog_stderr_flush(void);
>
> /* From mpm_winnt.c: */
>
> -extern module mpm_winnt_module;
> +extern module AP_MODULE_DECLARE_DATA mpm_winnt_module;
> extern int ap_threads_per_child;
>
> extern DWORD my_pid;
>
Getting closer
Linking...
Creating library .\Release/libhttpd.lib and object
.\Release/libhttpd.exp
mpm_winnt.obj : error LNK2019: unresolved external symbol
_winnt_insert_network_bucket referenced in function _winnt_hooks
I don't quite get this error though.
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Rainer Jung <ra...@kippdata.de>.
On 31.01.2012 22:30, Stefan Fritsch wrote:
> On Tuesday 31 January 2012, Gregg Smith wrote:
>> On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
>>> On Wednesday 25 January 2012, Jim Jagielski wrote:
>>>> 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?)
>>>
>>> OK, patch is attached. This needs review/testing for Windows.
>>
>> Patched trunk at r1237447, saw same after patching 2.4-HEAD so I
>> tried trunk.
>>
>> Regards,
>> Gregg
>>
>> Compiling...
>> service.c
>> .\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module'
>> : redefinition; different storage class
>>
>> c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
>>
>> : see declaration of 'mpm_winnt_module'
>
> Oops. Please try this in addition. Thanks for the testing.
No info about Windows, but: I applied the patches on trunk and ran the
test suite against a reallyall build on Solaris 10. No test failures.
Regards,
Rainer
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Tuesday 31 January 2012, Gregg Smith wrote:
> On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
> > On Wednesday 25 January 2012, Jim Jagielski wrote:
> >> 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?)
> >
> > OK, patch is attached. This needs review/testing for Windows.
>
> Patched trunk at r1237447, saw same after patching 2.4-HEAD so I
> tried trunk.
>
> Regards,
> Gregg
>
> Compiling...
> service.c
> .\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module'
> : redefinition; different storage class
>
> c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
>
> : see declaration of 'mpm_winnt_module'
Oops. Please try this in addition. Thanks for the testing.
--- a/server/mpm/winnt/mpm_winnt.h
+++ b/server/mpm/winnt/mpm_winnt.h
@@ -67,7 +67,7 @@ void mpm_nt_eventlog_stderr_flush(void);
/* From mpm_winnt.c: */
-extern module mpm_winnt_module;
+extern module AP_MODULE_DECLARE_DATA mpm_winnt_module;
extern int ap_threads_per_child;
extern DWORD my_pid;
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Thursday 02 February 2012, Joe Orton wrote:
> The combination of APR_SUCCESS and DECLINED is unusual; an int
> return value with OK/DECLINED?
Input and output filters should return an apr_status_t. So, if the
hook does not return an apr_status_t, core_input_filter() would have
to invent some apr_status_t value, which is bad. And I think the
general principle that return code 'int' means HTTP_* error code
should be kept.
But I agree that DECLINED == -1 is not a good idea, because it could
in theory collide with another APR_E* code. Should we define an
AP_STATUS_DECLINED or something in the APR_OS_START_USERERR range?
Or simply change the definition of DECLINED to be in the
APR_OS_START_USERERR range? APR_OS_START_USERERR is 120000, so there
should be no danger of a collision with HTTP_*. It would require a
major MMN bump, but making core_output_filter_ctx_t and core_ctx_t
private is an API change, anyway.
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Joe Orton <jo...@redhat.com>.
On Sun, Jan 29, 2012 at 08:53:09PM +0100, Stefan Fritsch wrote:
> + * Insert the network bucket into the core input filter's input brigade.
> + * This hook is intended for MPMs or protocol modules that need to do special
> + * socket setup.
> + * @param c The connection
> + * @param bb The brigade to insert the bucket into
> + * @param socket The socket to put into a bucket
> + * @return DECLINED if the current function does not handle this connection,
> + * APR_SUCCESS or an error otherwise.
> + */
> +AP_DECLARE_HOOK(apr_status_t, insert_network_bucket,
> + (conn_rec *c, apr_bucket_brigade *bb, apr_socket_t *socket))
The combination of APR_SUCCESS and DECLINED is unusual; an int return
value with OK/DECLINED? Looks great otherwise - thanks Stefan!
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Daniel Ruggeri <DR...@primary.net>.
On 1/31/2012 12:01 AM, Rainer Jung wrote:
> What's you runtime env then (Win version and 32 or 64 Bits)?
>
> Rainer
I have at my immediate disposal Server 2003 32-bit as well as Win7 and
Server 2008 64-bit. I may be able to scrounge up some more exotic
configurations if needed. My earlier tests to duplicate the problem were
done on the Win7 64 box.
--
Daniel Ruggeri
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Rainer Jung <ra...@kippdata.de>.
On 31.01.2012 00:36, Daniel Ruggeri wrote:
> On 1/30/2012 7:51 AM, Jim Jagielski wrote:
>> Anyone with Windows willing to sign up to review/test?
>
> I don't have a build environment to create something based on the diff,
> but if someone can create a build , I'll happily do the testing.
What's you runtime env then (Win version and 32 or 64 Bits)?
Rainer
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Daniel Ruggeri <DR...@primary.net>.
On 1/30/2012 7:51 AM, Jim Jagielski wrote:
> Anyone with Windows willing to sign up to review/test?
I don't have a build environment to create something based on the diff,
but if someone can create a build , I'll happily do the testing.
--
Daniel Ruggeri
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Jim Jagielski <ji...@jaguNET.com>.
Anyone with Windows willing to sign up to review/test?
On Jan 29, 2012, at 2:53 PM, Stefan Fritsch wrote:
> On Wednesday 25 January 2012, Jim Jagielski wrote:
>> 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?)
>
> OK, patch is attached. This needs review/testing for Windows.
> <insert_network_bucket.diff>
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
> On Wednesday 25 January 2012, Jim Jagielski wrote:
>> 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?)
> OK, patch is attached. This needs review/testing for Windows.
Patched trunk at r1237447, saw same after patching 2.4-HEAD so I tried
trunk.
Regards,
Gregg
Compiling...
service.c
.\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module' :
redefinition; different storage class
c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
: see declaration of 'mpm_winnt_module'
scoreboard.c
provider.c
nt_eventlog.c
mpm_winnt.c
.\server\mpm\winnt\mpm_winnt.c(1765) : error C2370: 'mpm_winnt_module' :
redefinition; different storage class
c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
: see declaration of 'mpm_winnt_module'
.\server\mpm\winnt\mpm_winnt.c(1765) : error C2370: 'mpm_winnt_module' :
redefinition; different storage class
c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
: see declaration of 'mpm_winnt_module'
mpm_common.c
listen.c
child.c
.\server\mpm\winnt\child.c(69) : error C2370: 'mpm_winnt_module' :
redefinition; different storage class
c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70)
: see declaration of 'mpm_winnt_module'
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Rainer Jung <ra...@kippdata.de>.
Hi Stefan,
On 29.01.2012 20:53, Stefan Fritsch wrote:
> On Wednesday 25 January 2012, Jim Jagielski wrote:
>> 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?)
>
> OK, patch is attached. This needs review/testing for Windows.
as far as I understand, this patch is for trunk.
Testing against 2.4.x would in addition need to first apply the
following four patches (all the trunk commits apply clean to 2.4.x):
------------------------------------------------------------------------
r1236122 | rpluem | 2012-01-26 11:03:36 +0100 (Thu, 26 Jan 2012) | 1 line
* Don't typedef twice (in .c and .h file).
------------------------------------------------------------------------
r1233882 | jorton | 2012-01-20 13:41:18 +0100 (Fri, 20 Jan 2012) | 4 lines
* server/core_filters.c (ap_core_input_filter): Only treat EAGAIN as
success if a non-blocking read was requested; for a blocking read,
it is an error condition.
------------------------------------------------------------------------
r1234899 | jorton | 2012-01-23 17:57:07 +0100 (Mon, 23 Jan 2012) | 4 lines
* server/core_filters.c (send_brigade_nonblocking): Use a non-blocking
bucket read, allowing any pending data to be flushed before trying a
(potentially slow) blocking read.
------------------------------------------------------------------------
r1235019 | sf | 2012-01-23 22:58:42 +0100 (Mon, 23 Jan 2012) | 6 lines
Make the core input/output filter contexts private and provide accessor APIs
for mpm_winnt and mod_ftp.
This allows to add members to the context structs without breaking binary
compatibility.
------------------------------------------------------------------------
Is that your understanding as well? Or are some of those unwanted for 2.4.x?
Regards,
Rainer
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Mon, 30 Jan 2012, Jim Jagielski wrote:
> On Jan 29, 2012, at 2:53 PM, Stefan Fritsch wrote:
>
>> On Wednesday 25 January 2012, Jim Jagielski wrote:
>>> 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?)
>>
>> OK, patch is attached. This needs review/testing for Windows.
>> <insert_network_bucket.diff>
>
> Just a quick scan but:
>
> +AP_IMPLEMENT_HOOK_RUN_FIRST(apr_status_t, insert_network_bucket,
> + (conn_rec *c, apr_bucket_brigade *bb,
> + apr_socket_t *socket),
> + (c, bb, socket), DECLINED)
> +
> ...
> + ap_hook_insert_network_bucket(core_insert_network_bucket, NULL, NULL,
> + APR_HOOK_REALLY_LAST);
>
> looks wonky...
I fail to see what is wonky here. Can you be more specific?
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jan 29, 2012, at 2:53 PM, Stefan Fritsch wrote:
> On Wednesday 25 January 2012, Jim Jagielski wrote:
>> 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?)
>
> OK, patch is attached. This needs review/testing for Windows.
> <insert_network_bucket.diff>
Just a quick scan but:
+AP_IMPLEMENT_HOOK_RUN_FIRST(apr_status_t, insert_network_bucket,
+ (conn_rec *c, apr_bucket_brigade *bb,
+ apr_socket_t *socket),
+ (c, bb, socket), DECLINED)
+
...
+ ap_hook_insert_network_bucket(core_insert_network_bucket, NULL, NULL,
+ APR_HOOK_REALLY_LAST);
looks wonky...
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
On 1/30/2012 11:14 PM, Gregg Smith wrote:
> On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
>> On Wednesday 25 January 2012, Jim Jagielski wrote:
>>> 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?)
>> OK, patch is attached. This needs review/testing for Windows.
> I'm assuming this is part of what is needed to get 2.4 going as well
> as Joe's big core overhaul in r1235019?
>
s/Joe's/Stefan's/
> I'm willing yet would prefer patching 2.4 for testing than patch &
> build trunk.
>
>
Re: [PATCH] trunk/2.4 core output filter is broken
Posted by Gregg Smith <gl...@gknw.net>.
On 1/29/2012 11:53 AM, Stefan Fritsch wrote:
> On Wednesday 25 January 2012, Jim Jagielski wrote:
>> 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?)
> OK, patch is attached. This needs review/testing for Windows.
I'm assuming this is part of what is needed to get 2.4 going as well as
Joe's big core overhaul in r1235019?
I'm willing yet would prefer patching 2.4 for testing than patch & build
trunk.
[PATCH] trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Wednesday 25 January 2012, Jim Jagielski wrote:
> 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?)
OK, patch is attached. This needs review/testing for Windows.
Re: trunk/2.4 core output filter is broken
Posted by Jim Jagielski <ji...@jaguNET.com>.
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 +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: the WinNT MPM use of this facility still
>> seems unnecessary. The same effect could be achieved using an input
>> filter which returns the extra data from AcceptEx.
>>
>> Here's a proof of concept for prefork, which prepends "GET" to data
>> received from the socket - no core hacks required. (This is dumb and
>> should be more sophisticated, it would need to handle
>> AP_MODE_GETBYTES/AP_MODE_GETLINE if possible from the "extra" data
>> alone, without always fetching a brigade from the socket)
>>
>> For mod_ftp the need to replace the socket bucket looks more convincing,
>> but still surely a hook calling *out* from the core is better?
>
> I agree that the it is not strictly necessary that WinNT MPM modifies the core input filter's brigade. But I don't think that yet another implementation of AP_MODE_GETBYTES/AP_MODE_GETLINE would be a superior design. I think we should defer this change to after Paul (or someone else) has implemented his input filter API redesign proposal from last November, which would make input filters more lightweight.
>
> BTW, the reason I didn't make this a hook is trying to be more efficient. But the difference may be negligible, it's only called once per connection. If you prefer to change it to a hook, please go ahead. I won't have time in the next few days.
>
Re: trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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.
>>
>> Opinions?
>
> After thinking about it more: the WinNT MPM use of this facility still
> seems unnecessary. The same effect could be achieved using an input
> filter which returns the extra data from AcceptEx.
>
> Here's a proof of concept for prefork, which prepends "GET" to data
> received from the socket - no core hacks required. (This is dumb and
> should be more sophisticated, it would need to handle
> AP_MODE_GETBYTES/AP_MODE_GETLINE if possible from the "extra" data
> alone, without always fetching a brigade from the socket)
>
> For mod_ftp the need to replace the socket bucket looks more convincing,
> but still surely a hook calling *out* from the core is better?
I agree that the it is not strictly necessary that WinNT MPM modifies the
core input filter's brigade. But I don't think that yet another
implementation of AP_MODE_GETBYTES/AP_MODE_GETLINE would be a superior
design. I think we should defer this change to after Paul (or someone
else) has implemented his input filter API redesign proposal from last
November, which would make input filters more lightweight.
BTW, the reason I didn't make this a hook is trying to be more efficient.
But the difference may be negligible, it's only called once per
connection. If you prefer to change it to a hook, please go ahead. I
won't have time in the next few days.
Re: trunk/2.4 core output filter is broken
Posted by Joe Orton <jo...@redhat.com>.
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: the WinNT MPM use of this facility still
seems unnecessary. The same effect could be achieved using an input
filter which returns the extra data from AcceptEx.
Here's a proof of concept for prefork, which prepends "GET" to data
received from the socket - no core hacks required. (This is dumb and
should be more sophisticated, it would need to handle
AP_MODE_GETBYTES/AP_MODE_GETLINE if possible from the "extra" data
alone, without always fetching a brigade from the socket)
For mod_ftp the need to replace the socket bucket looks more convincing,
but still surely a hook calling *out* from the core is better?
AP_DECLARE_HOOK(int, insert_network_bucket,
(connection_conn *c, apr_bucket_brigade *bb, apr_socket_t *socket));
then core i.f. does
net->in_ctx = ctx = ap_create_core_ctx(f->c);
ap_run_insert_network_bucket(c, ctx->b, net->client_socket);
and mod_ftp can insert its polling data bucket there instead?
(I haven't thought this one through, I admit, handwaving may be
required)
Index: server/mpm/prefork/prefork.c
===================================================================
--- server/mpm/prefork/prefork.c (revision 1235252)
+++ server/mpm/prefork/prefork.c (working copy)
@@ -95,6 +95,8 @@
static int mpm_state = AP_MPMQ_STARTING;
static ap_pod_t *pod;
+static ap_filter_rec_t *prefork_filter_handle;
+
/* data retained by prefork across load/unload of the module
* allocated on first call to pre-config hook; located on
* subsequent calls to pre-config hook
@@ -468,6 +470,23 @@
#endif
}
+static apr_status_t prefork_filter(ap_filter_t *f, apr_bucket_brigade *bb,
+ ap_input_mode_t mode, apr_read_type_e block,
+ apr_off_t bytes)
+{
+ apr_status_t rv;
+ const char *string = f->ctx;
+
+ rv = ap_get_brigade(f->next, bb, mode, block, bytes);
+ if (rv == APR_SUCCESS && string) {
+ apr_bucket *e = apr_bucket_immortal_create(string, strlen(string), f->c->bucket_alloc);
+ APR_BRIGADE_INSERT_HEAD(bb, e);
+ f->ctx = NULL;
+ }
+
+ return rv;
+}
+
/*****************************************************************
* Child process main loop.
* The following vars are static to avoid getting clobbered by longjmp();
@@ -691,6 +710,10 @@
current_conn = ap_run_create_connection(ptrans, ap_server_conf, csd, my_child_num, sbh, bucket_alloc);
if (current_conn) {
+
+ ap_add_input_filter_handle(prefork_filter_handle, "GET",
+ NULL, current_conn);
+
#if APR_HAS_THREADS
current_conn->current_thread = thd;
#endif
@@ -1454,6 +1477,10 @@
ap_hook_mpm(prefork_run, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_mpm_query(prefork_query, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_mpm_get_name(prefork_get_name, NULL, NULL, APR_HOOK_MIDDLE);
+
+ prefork_filter_handle =
+ ap_register_input_filter("PREFORK", prefork_filter, NULL,
+ AP_FTYPE_NETWORK - 1);
}
static const char *set_daemons_to_start(cmd_parms *cmd, void *dummy, const char *arg)
Re: trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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 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.
>
> Yes! I totally agree.
>
> > Is this something we should still do for 2.4.x (iff 2.4.0 is not
> > released)?
Attached is a simple patch to hide core input/output filter contexts.
The core_net_rec stays public. Obviously the winnt part is not tested
at all. The mod_ftp part compiles (after changing c->remote_ip
everywhere), docs are still missing.
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?
Re: trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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 could still
> do async write completion.
I have attached a modified version of your patch that includes this
optimization and adds some comments.
Re: trunk/2.4 core output filter is broken
Posted by Joe Orton <jo...@redhat.com>.
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 welcome. It seems to
do the right thing now, but I am worried I'm missing some subtlety of
the "async"ness here. Joe
Re: trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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 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.
>
> The blocking behaviour does matter here. If the brigade contains
> 60K of HEAP buckets and then a morphing bucket, the core must
> have written out the HEAP buckets before it attempts a blocking
> read on the morphing bucket. The usual style is per:
>
> http://httpd.apache.org/docs/trunk/developer/output-filters.html#no
> nblock
>
> The current behaviour is a regression back to early 2.0.x, PR 8482.
>
> > 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().
>
> I think I was not clear enough here: yes, the non-blocking read
> must be followed by blocking reads.
Right, that makes sense.
Re: trunk/2.4 core output filter is broken
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
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.
Re: trunk/2.4 core output filter is broken
Posted by Jim Jagielski <ji...@jaguNET.com>.
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
Posted by Joe Orton <jo...@redhat.com>.
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 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.
The blocking behaviour does matter here. If the brigade contains 60K of
HEAP buckets and then a morphing bucket, the core must have written out
the HEAP buckets before it attempts a *blocking* read on the morphing
bucket. The usual style is per:
http://httpd.apache.org/docs/trunk/developer/output-filters.html#nonblock
The current behaviour is a regression back to early 2.0.x, PR 8482.
> 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().
I think I was not clear enough here: yes, the non-blocking read must be
followed by blocking reads.
> > 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.
Yes! I totally agree.
> Is this something we should still do for 2.4.x (iff 2.4.0 is not
> released)?
Don't know. :)
Regards, Joe
Re: trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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?
> >>
> >> 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)?
> >
> > See also my observations about
> > https://issues.apache.org/bugzilla/show_bug.cgi?id=52476
>
> And before you make them private, look at other protocol modules
> such as mod_ftp. Losing these entirely would be foolish.
> Plugging into it could be made simpler, yes.
Good point. mod_ftp does exactly the same as the winnt MPM. I couldn't
find any other module that needs access to the core filter contexts.
Re: trunk/2.4 core output filter is broken
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
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 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)?
>
> See also my observations about
> https://issues.apache.org/bugzilla/show_bug.cgi?id=52476
And before you make them private, look at other protocol modules such
as mod_ftp. Losing these entirely would be foolish. Plugging into it
could be made simpler, yes.
Re: trunk/2.4 core output filter is broken
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
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 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)?
See also my observations about
https://issues.apache.org/bugzilla/show_bug.cgi?id=52476
Re: trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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)?
Re: trunk/2.4 core output filter is broken
Posted by Joe Orton <jo...@redhat.com>.
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?)
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).
send_brigade_nonblocking() also needs to be fixed to use a non-blocking
bucket read, but that is a separate issue.
Good catch on ctx->bytes_in. I'd add: why is core_output_filter_ctx_t in
a public header?
Index: server/core_filters.c
===================================================================
--- server/core_filters.c (revision 1233882)
+++ server/core_filters.c (working copy)
@@ -365,7 +365,7 @@
apr_bucket_brigade *bb = NULL;
apr_bucket *bucket, *next, *flush_upto = NULL;
apr_size_t bytes_in_brigade, non_file_bytes_in_brigade;
- int eor_buckets_in_brigade;
+ int eor_buckets_in_brigade, morphing_bucket_in_brigade;
apr_status_t rv;
/* Fail quickly if the connection has already been aborted. */
@@ -466,40 +466,40 @@
bytes_in_brigade = 0;
non_file_bytes_in_brigade = 0;
eor_buckets_in_brigade = 0;
+ morphing_bucket_in_brigade = 0;
+
for (bucket = APR_BRIGADE_FIRST(bb); bucket != APR_BRIGADE_SENTINEL(bb);
bucket = next) {
next = APR_BUCKET_NEXT(bucket);
if (!APR_BUCKET_IS_METADATA(bucket)) {
if (bucket->length == (apr_size_t)-1) {
- const char *data;
- apr_size_t length;
- /* XXX support nonblocking read here? */
- rv = apr_bucket_read(bucket, &data, &length, APR_BLOCK_READ);
- if (rv != APR_SUCCESS) {
- return rv;
+ morphing_bucket_in_brigade = 1;
+ }
+ else {
+ bytes_in_brigade += bucket->length;
+
+ if (!APR_BUCKET_IS_FILE(bucket)) {
+ non_file_bytes_in_brigade += bucket->length;
}
- /* reading may have split the bucket, so recompute next: */
- next = APR_BUCKET_NEXT(bucket);
}
- bytes_in_brigade += bucket->length;
- if (!APR_BUCKET_IS_FILE(bucket)) {
- non_file_bytes_in_brigade += bucket->length;
- }
}
else if (AP_BUCKET_IS_EOR(bucket)) {
eor_buckets_in_brigade++;
}
- if (APR_BUCKET_IS_FLUSH(bucket) ||
- (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER) ||
- (eor_buckets_in_brigade > MAX_REQUESTS_IN_PIPELINE) )
- {
+ if (APR_BUCKET_IS_FLUSH(bucket)
+ || non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER
+ || morphing_bucket_in_brigade
+ || eor_buckets_in_brigade > MAX_REQUESTS_IN_PIPELINE) {
+ /* this segment of the brigade MUST be sent before returning. */
+
if (APLOGctrace6(c)) {
char *reason = APR_BUCKET_IS_FLUSH(bucket) ?
"FLUSH bucket" :
(non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER) ?
"THRESHOLD_MAX_BUFFER" :
+ morphing_bucket_in_brigade ? "morphing bucket" :
"MAX_REQUESTS_IN_PIPELINE";
ap_log_cerror(APLOG_MARK, APLOG_TRACE6, 0, c,
"core_output_filter: flushing because of %s",
@@ -513,6 +513,7 @@
bytes_in_brigade = 0;
non_file_bytes_in_brigade = 0;
eor_buckets_in_brigade = 0;
+ morphing_bucket_in_brigade = 0;
}
}
Re: trunk/2.4 core output filter is broken
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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
> filter* itself is doing it, yegads.
>
> The fix may be as simple as adding an appropriate "break" to the
> loop, I'm not sure yet. It should be doing the non-block read,
> FLUSH & retry thing as well, so that streaming from slow
> generators works properly.
>
> prefork/mod_cgi, serve this as "nph-killme.pl" and wait for the
> fireworks:
>
> !/usr/bin/perl
>
> print "HTTP/1.0 200 OK\r\n";
> print "Transfer-Encoding: chunked\r\n";
> print "\r\n";
>
> print "5\r\nhello\r\n" while (1);
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?