Mailing List Archive

On Wed, Dec 20, 2023 at 5:20?PM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> On Wed, Dec 20, 2023 at 4:56?PM <jorton@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. */
--
?

On Wed, 20 Dec 2023, 16:30 Yann Ylavic, <ylavic.dev@gmail.com> wrote:

> On Wed, Dec 20, 2023 at 5:20?PM Yann Ylavic <ylavic.dev@gmail.com> wrote:
> >
> > On Wed, Dec 20, 2023 at 4:56?PM <jorton@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
>

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

On Wed, Dec 20, 2023 at 5:50?PM Joe Orton <jorton@redhat.com> wrote:
>
> On Wed, 20 Dec 2023, 16:30 Yann Ylavic, <ylavic.dev@gmail.com> 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.