On Wed, Dec 20, 2023 at 5:20 PM Yann Ylavic <ylavic....@gmail.com> wrote:
>
> On Wed, Dec 20, 2023 at 4:56 PM <jor...@apache.org> wrote:
> >
> > Author: jorton
> > Date: Wed Dec 20 15:56:15 2023
> > New Revision: 1914804
> >
> > URL: http://svn.apache.org/viewvc?rev=1914804&view=rev
> > Log:
> > * modules/http/chunk_filter.c (ap_http_chunk_filter): For a brigade
> > containing [FLUSH EOS], insert the last-chunk terminator before the
> > FLUSH rather than between the FLUSH and the EOS.
> >
> > Github: closes #400
>
> It seems that we should set the last-chunk before the FLUSH only if
> the latter precedes an EOS?
>
> So below:
>
> > --- httpd/httpd/trunk/modules/http/chunk_filter.c (original)
> > +++ httpd/httpd/trunk/modules/http/chunk_filter.c Wed Dec 20 15:56:15 2023
> > @@ -64,7 +64,7 @@ apr_status_t ap_http_chunk_filter(ap_fil
> >
> > for (more = tmp = NULL; b; b = more, more = NULL) {
> > apr_off_t bytes = 0;
> > - apr_bucket *eos = NULL;
> > + apr_bucket *eos = NULL; /* EOS bucket, or FLUSH preceding EOS */
> > apr_bucket *flush = NULL;
> > /* XXX: chunk_hdr must remain at this scope since it is used in a
> > * transient bucket.
> > @@ -99,8 +99,20 @@ apr_status_t ap_http_chunk_filter(ap_fil
> > }
> > if (APR_BUCKET_IS_FLUSH(e)) {
> > flush = e;
>
> Don't set "flush" above.
>
> > +
> > + /* Special case to catch common brigade ending of
> > + * [FLUSH] [EOS] - insert the last_chunk before
> > + * the FLUSH rather than between the FLUSH and the
> > + * EOS. */
> > if (e != APR_BRIGADE_LAST(b)) {
> > - more = apr_brigade_split_ex(b, APR_BUCKET_NEXT(e),
> > tmp);
> > + if (APR_BUCKET_IS_EOS(APR_BUCKET_NEXT(e))) {
>
> But here only?
>
> > + eos = e;
> > + /* anything after EOS is dropped, no need
> > + * to split. */
> > + }
> > + else {
> > + more = apr_brigade_split_ex(b,
> > APR_BUCKET_NEXT(e), tmp);
> > + }
> > }
> > break;
> > }
> > @@ -173,12 +185,12 @@ apr_status_t ap_http_chunk_filter(ap_fil
> > * FLUSH bucket, or appended to the brigade
> > */
> > e = apr_bucket_immortal_create(CRLF_ASCII, 2, c->bucket_alloc);
> > - if (eos != NULL) {
> > - APR_BUCKET_INSERT_BEFORE(eos, e);
> > - }
> > - else if (flush != NULL) {
> > + if (flush != NULL) {
> > APR_BUCKET_INSERT_BEFORE(flush, e);
> > }
> > + else if (eos != NULL) {
> > + APR_BUCKET_INSERT_BEFORE(eos, e);
> > + }
> > else {
> > APR_BRIGADE_INSERT_TAIL(b, e);
> > }
Ah no, this is for every chunk so we are good here.
For the last-chunk, I think we need:
Index: modules/http/chunk_filter.c
===================================================================
--- modules/http/chunk_filter.c (revision 1914804)
+++ modules/http/chunk_filter.c (working copy)
@@ -217,7 +217,7 @@ apr_status_t ap_http_chunk_filter(ap_filter_t *f,
* now.
*/
if (eos && !ctx->bad_gateway_seen) {
- ap_h1_add_end_chunk(b, eos, f->r, ctx->trailers);
+ ap_h1_add_end_chunk(b, flush ? flush : eos, f->r, ctx->trailers);
}
/* pass the brigade to the next filter. */
--
?
