Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c

[Yann Ylavic]

On Wed, Dec 20, 2023 at 5:50 PM Joe Orton  wrote:
>
> On Wed, 20 Dec 2023, 16:30 Yann Ylavic,  wrote:
>>
>> For the last-chunk, I think we need
>
> I think that's not necessary because in the special case eos=flush, in the 
> normal case flush is NULL. Actually the reordering of the tests above doesn't 
> make any difference either, I could have skipped that. In the first iteration 
> of the patch I renamed eos to "terminator" to make it more obvious.

You are right, I missed that "eos = e" and not "eos =
APR_BUCKET_NEXT(e)" in this case.
My noise, all good!

Regards;
Yann.

Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c

[Joe Orton]

On Wed, 20 Dec 2023, 16:30 Yann Ylavic,  wrote:

> On Wed, Dec 20, 2023 at 5:20 PM Yann Ylavic  wrote:
> >
> > On Wed, Dec 20, 2023 at 4:56 PM  wrote:
> > >
> > > Author: jorton
> > > Date: Wed Dec 20 15:56:15 2023
> > > New Revision: 1914804
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1914804=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
>

I think that's not necessary because in the special case eos=flush, in the
normal case flush is NULL. Actually the reordering of the tests above
doesn't make any difference either, I could have skipped that. In the first
iteration of the patch I renamed eos to "terminator" to make it more
obvious.

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. */
> --
> ?
>
>

Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c

[Yann Ylavic]

On Wed, Dec 20, 2023 at 5:20 PM Yann Ylavic  wrote:
>
> On Wed, Dec 20, 2023 at 4:56 PM  wrote:
> >
> > Author: jorton
> > Date: Wed Dec 20 15:56:15 2023
> > New Revision: 1914804
> >
> > URL: http://svn.apache.org/viewvc?rev=1914804=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. */
--
?

Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c

[Yann Ylavic]

On Wed, Dec 20, 2023 at 4:56 PM  wrote:
>
> Author: jorton
> Date: Wed Dec 20 15:56:15 2023
> New Revision: 1914804
>
> URL: http://svn.apache.org/viewvc?rev=1914804=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);
>  }


Regards;
Yann.