Re: cvs commit: httpd-2.0/modules/echo mod_echo.c

[Cliff Woolley <jw...@virginia.edu>]

On Tue, 15 Jun 2004 jorton@apache.org wrote:

>   * modules/echo/mod_echo.c (process_echo_connection): Fix brigade
>   handling: don't re-use a passed brigade.

Hang on.. why is the passed brigade getting destroyed?  You're supposed to
be able to reuse your brigades after the call stack has returned to
you--that's why if a later filter wants to stash all the buckets in the
brigade you've passed them, they have to create their own brigade to put
them in.

Brigade reuse is why there's a distinction between apr_brigade_cleanup()
and apr_brigade_destroy()... sounds like there's a destroy somewhere that
ought to be a cleanup.

--Cliff

Re: cvs commit: httpd-2.0/modules/echo mod_echo.c

[Joe Orton <jo...@redhat.com>]

On Tue, Jun 15, 2004 at 06:00:01PM -0400, Cliff Woolley wrote:
> On Tue, 15 Jun 2004, Joe Orton wrote:
> 
> > Hmmm, doing anything with a brigade after you pass it on to the next
> > filter surely breaks the golden "caller relinquishes ownership of the
> > brigade" rule for using ap_pass_brigade()?
> 
> I'm saying I thought the rule was expressly opposite that.  :)

Well, I didn't make the rules, that's just how ap_pass_brigade() is
documented in util_filter.h. ;)

> > core_output_filter destroys the brigade - if it didn't, the patches I
> > posted wouldn't be sufficient to fix the memory consumption problem.
> > The alternative is to go through and find all the places where brigades
> > are created and add _destroy calls everywhere as necessary.  That would
> > perhaps be safer by not breaking existing filters, but it seems ugly and
> > would also surely just leave more memory leaks waiting to happen...
> 
> Lemme ponder that and check around to see if we explicitly said anywhere
> that you were supposed to be able to keep old brigades after passing them.
> My main concern is that if we keep your changes as-is, it might mean we're
> constrained not to backport those changes to the APACHE_2_0_BRANCH because
> of breaking backward compatibility.

Yeah, concern shared... thanks for looking into this Cliff.

joe

Re: cvs commit: httpd-2.0/modules/echo mod_echo.c

[Cliff Woolley <jw...@virginia.edu>]

On Tue, 15 Jun 2004, Joe Orton wrote:

> Hmmm, doing anything with a brigade after you pass it on to the next
> filter surely breaks the golden "caller relinquishes ownership of the
> brigade" rule for using ap_pass_brigade()?

I'm saying I thought the rule was expressly opposite that.  :)

> core_output_filter destroys the brigade - if it didn't, the patches I
> posted wouldn't be sufficient to fix the memory consumption problem.
> The alternative is to go through and find all the places where brigades
> are created and add _destroy calls everywhere as necessary.  That would
> perhaps be safer by not breaking existing filters, but it seems ugly and
> would also surely just leave more memory leaks waiting to happen...

Lemme ponder that and check around to see if we explicitly said anywhere
that you were supposed to be able to keep old brigades after passing them.
My main concern is that if we keep your changes as-is, it might mean we're
constrained not to backport those changes to the APACHE_2_0_BRANCH because
of breaking backward compatibility.

Re: cvs commit: httpd-2.0/modules/echo mod_echo.c

[Joe Orton <jo...@redhat.com>]

On Tue, Jun 15, 2004 at 05:08:18PM -0400, Cliff Woolley wrote:
> On Tue, 15 Jun 2004 jorton@apache.org wrote:
> 
> >   * modules/echo/mod_echo.c (process_echo_connection): Fix brigade
> >   handling: don't re-use a passed brigade.
> 
> Hang on.. why is the passed brigade getting destroyed?  You're supposed to
> be able to reuse your brigades after the call stack has returned to
> you--that's why if a later filter wants to stash all the buckets in the
> brigade you've passed them, they have to create their own brigade to put
> them in.

Hmmm, doing anything with a brigade after you pass it on to the next
filter surely breaks the golden "caller relinquishes ownership of the
brigade" rule for using ap_pass_brigade()?

> Brigade reuse is why there's a distinction between apr_brigade_cleanup()
> and apr_brigade_destroy()... sounds like there's a destroy somewhere that
> ought to be a cleanup.

core_output_filter destroys the brigade - if it didn't, the patches I
posted wouldn't be sufficient to fix the memory consumption problem. 
The alternative is to go through and find all the places where brigades
are created and add _destroy calls everywhere as necessary.  That would
perhaps be safer by not breaking existing filters, but it seems ugly and
would also surely just leave more memory leaks waiting to happen...

Regards,

joe