You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2021/10/14 09:17:11 UTC
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
On 10/14/21 10:59 AM, icing@apache.org wrote:
> Author: icing
> Date: Thu Oct 14 08:59:12 2021
> New Revision: 1894220
>
> URL: http://svn.apache.org/viewvc?rev=1894220&view=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&r1=1894219&r2=1894220&view=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(&beam->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, &data, &len, 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
Posted by "stefan@eissing.org" <st...@eissing.org>.
It has been changed! h2 header buckets have now a length of 0.
> Am 14.10.2021 um 11:34 schrieb stefan@eissing.org:
>
>
>
>> Am 14.10.2021 um 11:28 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>
>>
>> On 10/14/21 11:20 AM, stefan@eissing.org wrote:
>>>
>>>
>>>> Am 14.10.2021 um 11:17 schrieb Ruediger Pluem <rp...@apache.org>:
>>>>
>>>>
>>>>
>>>> On 10/14/21 10:59 AM, icing@apache.org wrote:
>>>>> Author: icing
>>>>> Date: Thu Oct 14 08:59:12 2021
>>>>> New Revision: 1894220
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1894220&view=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&r1=1894219&r2=1894220&view=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(&beam->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
Posted by "stefan@eissing.org" <st...@eissing.org>.
> Am 14.10.2021 um 11:28 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 10/14/21 11:20 AM, stefan@eissing.org wrote:
>>
>>
>>> Am 14.10.2021 um 11:17 schrieb Ruediger Pluem <rp...@apache.org>:
>>>
>>>
>>>
>>> On 10/14/21 10:59 AM, icing@apache.org wrote:
>>>> Author: icing
>>>> Date: Thu Oct 14 08:59:12 2021
>>>> New Revision: 1894220
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1894220&view=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&r1=1894219&r2=1894220&view=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(&beam->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
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/14/21 11:20 AM, stefan@eissing.org wrote:
>
>
>> Am 14.10.2021 um 11:17 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>
>>
>> On 10/14/21 10:59 AM, icing@apache.org wrote:
>>> Author: icing
>>> Date: Thu Oct 14 08:59:12 2021
>>> New Revision: 1894220
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1894220&view=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&r1=1894219&r2=1894220&view=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(&beam->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
Posted by "stefan@eissing.org" <st...@eissing.org>.
> Am 14.10.2021 um 11:17 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 10/14/21 10:59 AM, icing@apache.org wrote:
>> Author: icing
>> Date: Thu Oct 14 08:59:12 2021
>> New Revision: 1894220
>>
>> URL: http://svn.apache.org/viewvc?rev=1894220&view=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&r1=1894219&r2=1894220&view=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(&beam->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, &data, &len, 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
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Oct 14, 2021 at 11:17:11AM +0200, Ruediger Pluem wrote:
> On 10/14/21 10:59 AM, icing@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(&beam->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.