Re: svn commit: r1894220 - in /httpd/httpd/trunk: modules/http2/h2_bucket_beam.c test/modules/http2/test_004_post.py test/modules/http2/test_400_push.py test/modules/http2/test_401_early_hints.py

2021-10-14 Thread ste...@eissing.org 
It has been changed! h2 header buckets have now a length of 0.

> Am 14.10.2021 um 11:34 schrieb ste...@eissing.org:
> 
> 
> 
>> Am 14.10.2021 um 11:28 schrieb Ruediger Pluem :
>> 
>> 
>> 
>> On 10/14/21 11:20 AM, ste...@eissing.org wrote:
>>> 
>>> 
 Am 14.10.2021 um 11:17 schrieb Ruediger Pluem :
 
 
 
 On 10/14/21 10:59 AM, ic...@apache.org wrote:
> Author: icing
> Date: Thu Oct 14 08:59:12 2021
> New Revision: 1894220
> 
> URL: http://svn.apache.org/viewvc?rev=1894220=rev
> Log:
> *) mod_http2: no longer splitting buckets on adding them to a beam,
>   accepting the whole bucket since no memory is saved by a split.
>   Also, allowing meta buckets to be added to a "full" beam.
>   Re-enabled test cases for travis verification.
> 
> 
> Modified:
>  httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
>  httpd/httpd/trunk/test/modules/http2/test_004_post.py
>  httpd/httpd/trunk/test/modules/http2/test_400_push.py
>  httpd/httpd/trunk/test/modules/http2/test_401_early_hints.py
> 
> Modified: httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_bucket_beam.c?rev=1894220=1894219=1894220=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_bucket_beam.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_bucket_beam.c Thu Oct 14 08:59:12 
> 2021
> @@ -420,29 +420,41 @@ void h2_beam_abort(h2_bucket_beam *beam,
> }
> 
> static apr_status_t append_bucket(h2_bucket_beam *beam,
> -  apr_bucket *b,
> +  apr_bucket_brigade *bb,
> apr_read_type_e block,
> apr_size_t *pspace_left,
> apr_off_t *pwritten)
> {
> +apr_bucket *b;
>   const char *data;
>   apr_size_t len;
> -apr_status_t status = APR_SUCCESS;
> -int can_beam = 0, check_len;
> +apr_status_t rv = APR_SUCCESS;
> +int can_beam = 0;
> 
>   (void)block;
>   if (beam->aborted) {
> -return APR_ECONNABORTED;
> +rv = APR_ECONNABORTED;
> +goto cleanup;
>   }
> -
> +
> +b = APR_BRIGADE_FIRST(bb);
>   if (APR_BUCKET_IS_METADATA(b)) {
>   APR_BUCKET_REMOVE(b);
>   apr_bucket_setaside(b, beam->pool);
>   H2_BLIST_INSERT_TAIL(>buckets_to_send, b);
>   *pwritten += (apr_off_t)b->length;
 
 Are there meta buckets that have a length that is not zero?
 I mean is it even allowed to define meta buckets with a length != 0?
>>> 
>>> We need the bucket police for that one!
>>> 
>>> There is currently the H2HEADER bucket that does this shameful thing. It 
>>> should probably not do that and find another way.
>> 
>> I am personally fine with metabuckets that have a non zero length, but I 
>> fear there could be assumptions in the existing code
>> that metabucket means length == 0. If this assumption is only there for 
>> memory accounting I guess it does not matter as
>> metabuckets with length != 0 likely only have a small length.
>> Some I am keen to hear what others think about non zero length metabuckets.
> 
> Background: this "hack" exists, because mod_logio reports overall data and 
> response body data separately. And this counts (inaccurately) the amount of 
> bytes that header and footers are later turned into.
> 
> Since mod_logio lives in http/1.1 country, it is a bit tricky to report the 
> bytes send for a h2 stream.
> 
> But there is an alternative way of getting that while keeping b->length == 0 
> for H2HEADER buckets. So for simplicity alone, this should be changed.
> 
> - Stefan

Re: svn commit: r1894220 - in /httpd/httpd/trunk: modules/http2/h2_bucket_beam.c test/modules/http2/test_004_post.py test/modules/http2/test_400_push.py test/modules/http2/test_401_early_hints.py

2021-10-14 Thread ste...@eissing.org 



> Am 14.10.2021 um 11:28 schrieb Ruediger Pluem :
> 
> 
> 
> On 10/14/21 11:20 AM, ste...@eissing.org wrote:
>> 
>> 
>>> Am 14.10.2021 um 11:17 schrieb Ruediger Pluem :
>>> 
>>> 
>>> 
>>> On 10/14/21 10:59 AM, ic...@apache.org wrote:
 Author: icing
 Date: Thu Oct 14 08:59:12 2021
 New Revision: 1894220
 
 URL: http://svn.apache.org/viewvc?rev=1894220=rev
 Log:
 *) mod_http2: no longer splitting buckets on adding them to a beam,
accepting the whole bucket since no memory is saved by a split.
Also, allowing meta buckets to be added to a "full" beam.
Re-enabled test cases for travis verification.
 
 
 Modified:
   httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
   httpd/httpd/trunk/test/modules/http2/test_004_post.py
   httpd/httpd/trunk/test/modules/http2/test_400_push.py
   httpd/httpd/trunk/test/modules/http2/test_401_early_hints.py
 
 Modified: httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
 URL: 
 http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_bucket_beam.c?rev=1894220=1894219=1894220=diff
 ==
 --- httpd/httpd/trunk/modules/http2/h2_bucket_beam.c (original)
 +++ httpd/httpd/trunk/modules/http2/h2_bucket_beam.c Thu Oct 14 08:59:12 
 2021
 @@ -420,29 +420,41 @@ void h2_beam_abort(h2_bucket_beam *beam,
 }
 
 static apr_status_t append_bucket(h2_bucket_beam *beam,
 -  apr_bucket *b,
 +  apr_bucket_brigade *bb,
  apr_read_type_e block,
  apr_size_t *pspace_left,
  apr_off_t *pwritten)
 {
 +apr_bucket *b;
const char *data;
apr_size_t len;
 -apr_status_t status = APR_SUCCESS;
 -int can_beam = 0, check_len;
 +apr_status_t rv = APR_SUCCESS;
 +int can_beam = 0;
 
(void)block;
if (beam->aborted) {
 -return APR_ECONNABORTED;
 +rv = APR_ECONNABORTED;
 +goto cleanup;
}
 -
 +
 +b = APR_BRIGADE_FIRST(bb);
if (APR_BUCKET_IS_METADATA(b)) {
APR_BUCKET_REMOVE(b);
apr_bucket_setaside(b, beam->pool);
H2_BLIST_INSERT_TAIL(>buckets_to_send, b);
*pwritten += (apr_off_t)b->length;
>>> 
>>> Are there meta buckets that have a length that is not zero?
>>> I mean is it even allowed to define meta buckets with a length != 0?
>> 
>> We need the bucket police for that one!
>> 
>> There is currently the H2HEADER bucket that does this shameful thing. It 
>> should probably not do that and find another way.
> 
> I am personally fine with metabuckets that have a non zero length, but I fear 
> there could be assumptions in the existing code
> that metabucket means length == 0. If this assumption is only there for 
> memory accounting I guess it does not matter as
> metabuckets with length != 0 likely only have a small length.
> Some I am keen to hear what others think about non zero length metabuckets.

Background: this "hack" exists, because mod_logio reports overall data and 
response body data separately. And this counts (inaccurately) the amount of 
bytes that header and footers are later turned into.

Since mod_logio lives in http/1.1 country, it is a bit tricky to report the 
bytes send for a h2 stream.

But there is an alternative way of getting that while keeping b->length == 0 
for H2HEADER buckets. So for simplicity alone, this should be changed.

- Stefan

Re: svn commit: r1894220 - in /httpd/httpd/trunk: modules/http2/h2_bucket_beam.c test/modules/http2/test_004_post.py test/modules/http2/test_400_push.py test/modules/http2/test_401_early_hints.py

2021-10-14 Thread Joe Orton 
On Thu, Oct 14, 2021 at 11:17:11AM +0200, Ruediger Pluem wrote:
> On 10/14/21 10:59 AM, ic...@apache.org wrote:
> > -
> > +
> > +b = APR_BRIGADE_FIRST(bb);
> >  if (APR_BUCKET_IS_METADATA(b)) {
> >  APR_BUCKET_REMOVE(b);
> >  apr_bucket_setaside(b, beam->pool);
> >  H2_BLIST_INSERT_TAIL(>buckets_to_send, b);
> >  *pwritten += (apr_off_t)b->length;
> 
> Are there meta buckets that have a length that is not zero?
> I mean is it even allowed to define meta buckets with a length != 0?

No, definitely not, all metadata buckets must have length = 0.

Re: svn commit: r1894220 - in /httpd/httpd/trunk: modules/http2/h2_bucket_beam.c test/modules/http2/test_004_post.py test/modules/http2/test_400_push.py test/modules/http2/test_401_early_hints.py

2021-10-14 Thread Ruediger Pluem 



On 10/14/21 11:20 AM, ste...@eissing.org wrote:
> 
> 
>> Am 14.10.2021 um 11:17 schrieb Ruediger Pluem :
>>
>>
>>
>> On 10/14/21 10:59 AM, ic...@apache.org wrote:
>>> Author: icing
>>> Date: Thu Oct 14 08:59:12 2021
>>> New Revision: 1894220
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1894220=rev
>>> Log:
>>>  *) mod_http2: no longer splitting buckets on adding them to a beam,
>>> accepting the whole bucket since no memory is saved by a split.
>>> Also, allowing meta buckets to be added to a "full" beam.
>>> Re-enabled test cases for travis verification.
>>>
>>>
>>> Modified:
>>>httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
>>>httpd/httpd/trunk/test/modules/http2/test_004_post.py
>>>httpd/httpd/trunk/test/modules/http2/test_400_push.py
>>>httpd/httpd/trunk/test/modules/http2/test_401_early_hints.py
>>>
>>> Modified: httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_bucket_beam.c?rev=1894220=1894219=1894220=diff
>>> ==
>>> --- httpd/httpd/trunk/modules/http2/h2_bucket_beam.c (original)
>>> +++ httpd/httpd/trunk/modules/http2/h2_bucket_beam.c Thu Oct 14 08:59:12 
>>> 2021
>>> @@ -420,29 +420,41 @@ void h2_beam_abort(h2_bucket_beam *beam,
>>> }
>>>
>>> static apr_status_t append_bucket(h2_bucket_beam *beam,
>>> -  apr_bucket *b,
>>> +  apr_bucket_brigade *bb,
>>>   apr_read_type_e block,
>>>   apr_size_t *pspace_left,
>>>   apr_off_t *pwritten)
>>> {
>>> +apr_bucket *b;
>>> const char *data;
>>> apr_size_t len;
>>> -apr_status_t status = APR_SUCCESS;
>>> -int can_beam = 0, check_len;
>>> +apr_status_t rv = APR_SUCCESS;
>>> +int can_beam = 0;
>>>
>>> (void)block;
>>> if (beam->aborted) {
>>> -return APR_ECONNABORTED;
>>> +rv = APR_ECONNABORTED;
>>> +goto cleanup;
>>> }
>>> -
>>> +
>>> +b = APR_BRIGADE_FIRST(bb);
>>> if (APR_BUCKET_IS_METADATA(b)) {
>>> APR_BUCKET_REMOVE(b);
>>> apr_bucket_setaside(b, beam->pool);
>>> H2_BLIST_INSERT_TAIL(>buckets_to_send, b);
>>> *pwritten += (apr_off_t)b->length;
>>
>> Are there meta buckets that have a length that is not zero?
>> I mean is it even allowed to define meta buckets with a length != 0?
> 
> We need the bucket police for that one!
> 
> There is currently the H2HEADER bucket that does this shameful thing. It 
> should probably not do that and find another way.

I am personally fine with metabuckets that have a non zero length, but I fear 
there could be assumptions in the existing code
that metabucket means length == 0. If this assumption is only there for memory 
accounting I guess it does not matter as
metabuckets with length != 0 likely only have a small length.
Some I am keen to hear what others think about non zero length metabuckets.

Regards

Rüdiger

Re: svn commit: r1894220 - in /httpd/httpd/trunk: modules/http2/h2_bucket_beam.c test/modules/http2/test_004_post.py test/modules/http2/test_400_push.py test/modules/http2/test_401_early_hints.py

2021-10-14 Thread ste...@eissing.org 



> Am 14.10.2021 um 11:17 schrieb Ruediger Pluem :
> 
> 
> 
> On 10/14/21 10:59 AM, ic...@apache.org wrote:
>> Author: icing
>> Date: Thu Oct 14 08:59:12 2021
>> New Revision: 1894220
>> 
>> URL: http://svn.apache.org/viewvc?rev=1894220=rev
>> Log:
>>  *) mod_http2: no longer splitting buckets on adding them to a beam,
>> accepting the whole bucket since no memory is saved by a split.
>> Also, allowing meta buckets to be added to a "full" beam.
>> Re-enabled test cases for travis verification.
>> 
>> 
>> Modified:
>>httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
>>httpd/httpd/trunk/test/modules/http2/test_004_post.py
>>httpd/httpd/trunk/test/modules/http2/test_400_push.py
>>httpd/httpd/trunk/test/modules/http2/test_401_early_hints.py
>> 
>> Modified: httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_bucket_beam.c?rev=1894220=1894219=1894220=diff
>> ==
>> --- httpd/httpd/trunk/modules/http2/h2_bucket_beam.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_bucket_beam.c Thu Oct 14 08:59:12 2021
>> @@ -420,29 +420,41 @@ void h2_beam_abort(h2_bucket_beam *beam,
>> }
>> 
>> static apr_status_t append_bucket(h2_bucket_beam *beam,
>> -  apr_bucket *b,
>> +  apr_bucket_brigade *bb,
>>   apr_read_type_e block,
>>   apr_size_t *pspace_left,
>>   apr_off_t *pwritten)
>> {
>> +apr_bucket *b;
>> const char *data;
>> apr_size_t len;
>> -apr_status_t status = APR_SUCCESS;
>> -int can_beam = 0, check_len;
>> +apr_status_t rv = APR_SUCCESS;
>> +int can_beam = 0;
>> 
>> (void)block;
>> if (beam->aborted) {
>> -return APR_ECONNABORTED;
>> +rv = APR_ECONNABORTED;
>> +goto cleanup;
>> }
>> -
>> +
>> +b = APR_BRIGADE_FIRST(bb);
>> if (APR_BUCKET_IS_METADATA(b)) {
>> APR_BUCKET_REMOVE(b);
>> apr_bucket_setaside(b, beam->pool);
>> H2_BLIST_INSERT_TAIL(>buckets_to_send, b);
>> *pwritten += (apr_off_t)b->length;
> 
> Are there meta buckets that have a length that is not zero?
> I mean is it even allowed to define meta buckets with a length != 0?

We need the bucket police for that one!

There is currently the H2HEADER bucket that does this shameful thing. It should 
probably not do that and find another way.


>> -return APR_SUCCESS;
>> +goto cleanup;
>> }
>> -else if (APR_BUCKET_IS_FILE(b)) {
>> +/* non meta bucket */
>> +
>> +/* in case of indeterminate length, we need to read the bucket,
>> + * so that it transforms itself into something stable. */
>> +if (b->length == ((apr_size_t)-1)) {
>> +rv = apr_bucket_read(b, , , APR_BLOCK_READ);
>> +if (rv != APR_SUCCESS) goto cleanup;
>> +}
>> +
>> +if (APR_BUCKET_IS_FILE(b)) {
>> /* For file buckets the problem is their internal readpool that
>>  * is used on the first read to allocate buffer/mmap.
>>  * Since setting aside a file bucket will de-register the
> 
> Regards
> 
> Rüdiger

Re: svn commit: r1894220 - in /httpd/httpd/trunk: modules/http2/h2_bucket_beam.c test/modules/http2/test_004_post.py test/modules/http2/test_400_push.py test/modules/http2/test_401_early_hints.py

2021-10-14 Thread Ruediger Pluem 



On 10/14/21 10:59 AM, ic...@apache.org wrote:
> Author: icing
> Date: Thu Oct 14 08:59:12 2021
> New Revision: 1894220
> 
> URL: http://svn.apache.org/viewvc?rev=1894220=rev
> Log:
>   *) mod_http2: no longer splitting buckets on adding them to a beam,
>  accepting the whole bucket since no memory is saved by a split.
>  Also, allowing meta buckets to be added to a "full" beam.
>  Re-enabled test cases for travis verification.
> 
> 
> Modified:
> httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
> httpd/httpd/trunk/test/modules/http2/test_004_post.py
> httpd/httpd/trunk/test/modules/http2/test_400_push.py
> httpd/httpd/trunk/test/modules/http2/test_401_early_hints.py
> 
> Modified: httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_bucket_beam.c?rev=1894220=1894219=1894220=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_bucket_beam.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_bucket_beam.c Thu Oct 14 08:59:12 2021
> @@ -420,29 +420,41 @@ void h2_beam_abort(h2_bucket_beam *beam,
>  }
>  
>  static apr_status_t append_bucket(h2_bucket_beam *beam,
> -  apr_bucket *b,
> +  apr_bucket_brigade *bb,
>apr_read_type_e block,
>apr_size_t *pspace_left,
>apr_off_t *pwritten)
>  {
> +apr_bucket *b;
>  const char *data;
>  apr_size_t len;
> -apr_status_t status = APR_SUCCESS;
> -int can_beam = 0, check_len;
> +apr_status_t rv = APR_SUCCESS;
> +int can_beam = 0;
>  
>  (void)block;
>  if (beam->aborted) {
> -return APR_ECONNABORTED;
> +rv = APR_ECONNABORTED;
> +goto cleanup;
>  }
> -
> +
> +b = APR_BRIGADE_FIRST(bb);
>  if (APR_BUCKET_IS_METADATA(b)) {
>  APR_BUCKET_REMOVE(b);
>  apr_bucket_setaside(b, beam->pool);
>  H2_BLIST_INSERT_TAIL(>buckets_to_send, b);
>  *pwritten += (apr_off_t)b->length;

Are there meta buckets that have a length that is not zero?
I mean is it even allowed to define meta buckets with a length != 0?


> -return APR_SUCCESS;
> +goto cleanup;
>  }
> -else if (APR_BUCKET_IS_FILE(b)) {
> +/* non meta bucket */
> +
> +/* in case of indeterminate length, we need to read the bucket,
> + * so that it transforms itself into something stable. */
> +if (b->length == ((apr_size_t)-1)) {
> +rv = apr_bucket_read(b, , , APR_BLOCK_READ);
> +if (rv != APR_SUCCESS) goto cleanup;
> +}
> +
> +if (APR_BUCKET_IS_FILE(b)) {
>  /* For file buckets the problem is their internal readpool that
>   * is used on the first read to allocate buffer/mmap.
>   * Since setting aside a file bucket will de-register the

Regards

Rüdiger