PR291 is dead, long live PR306!

[Stefan Eissing]

The first PR to separate HTTP and HTTP/1.x processing is in: 
https://github.com/apache/httpd/pull/306

This is a small subset of PR291 that is easier to read. While it has all API 
additions from PR291, it
only uses a subset in the implementation. With further use coming in future PRs.

This PRs functional change is the move of the HTTP/1.x parts the HTTP_IN filter 
in a separate filter,
which allows a large code removal in mod_http2.

Kind Regards,
Stefan

Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

[Jim Jagielski]

It was added in anticipation of the capability to be folded in, and done so 
"now" so that it would;t require any API changes.

Unless it's actually breaking something, I'd vote to simply keep it

> On Apr 1, 2022, at 3:42 AM, jean-frederic clere  wrote:
> 
> On 01/04/2022 08:47, jean-frederic clere wrote:
>> On 31/03/2022 12:59, Ruediger Pluem wrote:
>>> 
>>> 
>>> On 3/31/22 12:34 PM, Stefan Eissing wrote:
 
 
> Am 31.03.2022 um 11:55 schrieb Ruediger Pluem :
> 
> 
> 
> On 3/31/22 11:11 AM, Ruediger Pluem wrote:
>> 
>> 
>> On 3/30/22 4:42 PM, jfcl...@apache.org wrote:
>>> Author: jfclere
>>> Date: Wed Mar 30 14:42:14 2022
>>> New Revision: 1899390
>>> 
>>> URL: http://svn.apache.org/viewvc?rev=1899390=rev
>>> Log:
>>> Add WorkerBalancerGrowth. To allow creation of workers
>>> to dynamically added balancers.
>>> 
>>> Modified:
>>> httpd/httpd/trunk/CHANGES
>>> httpd/httpd/trunk/modules/proxy/mod_proxy.c
>>> httpd/httpd/trunk/modules/proxy/mod_proxy.h
>>> 
>>> Modified: httpd/httpd/trunk/CHANGES
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff
>>>  
>>> ==
>>>  
>>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>>> +++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
>>> @@ -1,6 +1,10 @@
>>> -*- coding: utf-8 -*-
>>> Changes with Apache 2.5.1
>>> 
>>> + *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
>>> + balancer created dynamically or via "empty" 
>>> + [Jean-Frederic Clere]
>> 
>> I am not sure why this is needed. You can already do this via
>> 
>> 
>> 
> 
> Or
> 
> 
> ProxySet growth=10
> 
 
 FYI: Travis trunk also fails almost completely. Does not seem to accept a 
 proxy configuration.
>>> 
>>> This is because the if
>>> 
>>> +  if (!ap_strchr_c(conf->p, ':'))
>>> +  return apr_pstrcat(cmd->pool, thiscmd->name,
>>> +  "> arguments are not supported for non url.",
>>> +  NULL);
>>> 
>>> should not return with an error, but just encapsulate the remainder of the 
>>> block. And I think the further
>>> return apr_pstrcat are also wrong.
>>> 
>>> But as said I am not sure about the purpose at all as you can already do, 
>>> what the patch should provide if I understand the patch
>>> correctly.
>> The purpose was to be able to add a balancer in the balancer-manager handle 
>> but that needs to pre-create the mutex and the slots for the workers.
>> While looking to that I noted that:
>> 
>> 
>> was doing nothing, the balancer is ignored, I should I revert the patch and 
>> add an error message if there is an empty entry like this one?
> 
> There is also the BalancerGrowth directive that does nothing else than 
> creating a memory slot for balancers we never add/create in the 
> balancer-manager, I am tempted to remove it...
> 
> Would it be better to add the missing logic? I have some pieces in 
> mod_proxy_cluster (https://github.com/modcluster/mod_proxy_cluster 
>  that could use the logic.
>>> 
>>> Regards
>>> 
>>> Rüdiger
> 
> 
> -- 
> Cheers
> 
> Jean-Frederic

Re: HTTP and HTTP/1.x separation

[Stefan Eissing]

just a heads up: I will replace this PR with a series of smaller ones. 

The accumulated changes are too large to make someone review and have
an opinion about it in reasonable time. I will therefore introduce the
changes in small steps, trying to make them each easier to read.

I have the first one sitting here locally and will publish that once
our CI is working again (and I can be sure that they do as well).

Kind Regards,
Stefan


> Am 31.03.2022 um 09:40 schrieb jean-frederic clere :
> 
> On 30/03/2022 11:11, Stefan Eissing wrote:
>>> Am 28.03.2022 um 15:52 schrieb jean-frederic clere :
>>> 
>>> On 24/03/2022 13:21, Stefan Eissing wrote:
 You are invited to have a look at my PR for separating HTTP/1.x processing 
 from
 generic HTTP protocol handling and verification:
 https://github.com/apache/httpd/pull/291
 I made a description of the changes in the PR that helps reviewing it (I 
 hope).
 "Changes appear larger than they really are"
 A lot is code split+move from mod_http to mod_http1. In mod_http2, changes 
 are
 mainly removals of quirks necessary so far.
 Kind Regards,
 Stefan
>>> 
>>> Something fishy:
>>> http/1.1:
>>> +++
>>> 
>>> < HTTP/1.1 200 OK
>>> < Date: Mon, 28 Mar 2022 13:48:23 GMT
>>> < Server: Apache/2.5.1-dev (Unix) OpenSSL/1.1.1n
>>> < Last-Modified: Fri, 25 Mar 2022 15:47:39 GMT
>>> < ETag: "bf-5db0ce1e1e93e"
>>> < Accept-Ranges: bytes
>>> < Content-Length: 191
>>> < Content-Type: text/html
>>> 
>>> +++
>>> http/2:
>>> +++
>>> < HTTP/2 200
>>> < last-modified: Fri, 25 Mar 2022 15:47:39 GMT
>>> < etag: "bf-5db0ce1e1e93e"
>>> < accept-ranges: bytes
>>> < content-length: 191
>>> < content-type: text/html
>>> +++
>>> 
>>> Did I miss something?
>> Just added the fix to the PR:
>> *) core, mod_http1, mod_http: moved the handling of the standard
>> response headers `Date` and `Server` from mod_http1 into the
>> generic HTTP protocol handling.
>> Response buckets not always carry those headers (values preserved
>> from proxied responses), irregardless of the HTTP protocol
>> versions involved.
>> mod_http1: the serialization of response header into HTTP/1.x
>> format always writes `Date` and `Server` first if present. This
>> assured backward compatibility with clients who are accustomed
>> to this order.
> 
> Thanks my tests are passing now.
> 
>> Kind Regards,
>> Stefan
>>> 
>>> -- 
>>> Cheers
>>> 
>>> Jean-Frederic
>>> 
> 
> 
> -- 
> Cheers
> 
> Jean-Frederic

Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

[Ruediger Pluem]

On 4/1/22 10:23 AM, jean-frederic clere wrote:
> On 01/04/2022 10:03, Ruediger Pluem wrote:
>>

> 
> Sure I have a PR to revert, waiting on travis...
> 
>> Feel free to add a detection for such empty proxy blocks. I think a warning
>> should be sufficient. How do you want to detect this? By inspecting 
>> new_dir_conf after ap_walk_config was executed?
> 
> putting it in the proxysection() is not the best, correct?

I haven't made detailed thoughts on how to implement this detection. Hence I 
cannot answer :-)

Regards

Rüdiger

Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

[jean-frederic clere]

On 01/04/2022 10:03, Ruediger Pluem wrote:



On 4/1/22 8:47 AM, jean-frederic clere wrote:

On 31/03/2022 12:59, Ruediger Pluem wrote:



On 3/31/22 12:34 PM, Stefan Eissing wrote:




Am 31.03.2022 um 11:55 schrieb Ruediger Pluem :



On 3/31/22 11:11 AM, Ruediger Pluem wrote:



On 3/30/22 4:42 PM, jfcl...@apache.org wrote:

Author: jfclere
Date: Wed Mar 30 14:42:14 2022
New Revision: 1899390

URL: http://svn.apache.org/viewvc?rev=1899390=rev
Log:
Add WorkerBalancerGrowth. To allow creation of workers
to dynamically added balancers.

Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/proxy/mod_proxy.c
httpd/httpd/trunk/modules/proxy/mod_proxy.h

Modified: httpd/httpd/trunk/CHANGES
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff
==
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
@@ -1,6 +1,10 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.1

+ *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
+ balancer created dynamically or via "empty" 
+ [Jean-Frederic Clere]


I am not sure why this is needed. You can already do this via





Or


ProxySet growth=10



FYI: Travis trunk also fails almost completely. Does not seem to accept a proxy 
configuration.


This is because the if

+    if (!ap_strchr_c(conf->p, ':'))
+    return apr_pstrcat(cmd->pool, thiscmd->name,
+   "> arguments are not supported for non url.",
+   NULL);

should not return with an error, but just encapsulate the remainder of the 
block. And I think the further
return apr_pstrcat are also wrong.

But as said I am not sure about the purpose at all as you can already do, what 
the patch should provide if I understand the patch
correctly.


The purpose was to be able to add a balancer in the balancer-manager handle but 
that needs to pre-create the mutex and the slots
for the workers.

While looking to that I noted that:



was doing nothing, the balancer is ignored, I should I revert the patch and add 
an error message if there is an empty entry like
this one?


Do




or


ProxySet growth=10


work and do what you want? If yes, please revert.


Sure I have a PR to revert, waiting on travis...


Feel free to add a detection for such empty proxy blocks. I think a warning
should be sufficient. How do you want to detect this? By inspecting 
new_dir_conf after ap_walk_config was executed?


putting it in the proxysection() is not the best, correct?



Regards

Rüdiger



--
Cheers

Jean-Frederic

Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

[Ruediger Pluem]

On 4/1/22 8:47 AM, jean-frederic clere wrote:
> On 31/03/2022 12:59, Ruediger Pluem wrote:
>>
>>
>> On 3/31/22 12:34 PM, Stefan Eissing wrote:
>>>
>>>
 Am 31.03.2022 um 11:55 schrieb Ruediger Pluem :



 On 3/31/22 11:11 AM, Ruediger Pluem wrote:
>
>
> On 3/30/22 4:42 PM, jfcl...@apache.org wrote:
>> Author: jfclere
>> Date: Wed Mar 30 14:42:14 2022
>> New Revision: 1899390
>>
>> URL: http://svn.apache.org/viewvc?rev=1899390=rev
>> Log:
>> Add WorkerBalancerGrowth. To allow creation of workers
>> to dynamically added balancers.
>>
>> Modified:
>> httpd/httpd/trunk/CHANGES
>> httpd/httpd/trunk/modules/proxy/mod_proxy.c
>> httpd/httpd/trunk/modules/proxy/mod_proxy.h
>>
>> Modified: httpd/httpd/trunk/CHANGES
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff
>> ==
>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>> +++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
>> @@ -1,6 +1,10 @@
>> -*- coding: utf-8 -*-
>> Changes with Apache 2.5.1
>>
>> + *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
>> + balancer created dynamically or via "empty" 
>> + [Jean-Frederic Clere]
>
> I am not sure why this is needed. You can already do this via
>
> 
> 

 Or

 
 ProxySet growth=10
 
>>>
>>> FYI: Travis trunk also fails almost completely. Does not seem to accept a 
>>> proxy configuration.
>>
>> This is because the if
>>
>> +    if (!ap_strchr_c(conf->p, ':'))
>> +    return apr_pstrcat(cmd->pool, thiscmd->name,
>> +   "> arguments are not supported for non url.",
>> +   NULL);
>>
>> should not return with an error, but just encapsulate the remainder of the 
>> block. And I think the further
>> return apr_pstrcat are also wrong.
>>
>> But as said I am not sure about the purpose at all as you can already do, 
>> what the patch should provide if I understand the patch
>> correctly.
> 
> The purpose was to be able to add a balancer in the balancer-manager handle 
> but that needs to pre-create the mutex and the slots
> for the workers.
> 
> While looking to that I noted that:
> 
> 
> 
> was doing nothing, the balancer is ignored, I should I revert the patch and 
> add an error message if there is an empty entry like
> this one?

Do




or


ProxySet growth=10


work and do what you want? If yes, please revert. Feel free to add a detection 
for such empty proxy blocks. I think a warning
should be sufficient. How do you want to detect this? By inspecting 
new_dir_conf after ap_walk_config was executed?

Regards

Rüdiger

Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

[jean-frederic clere]

On 31/03/2022 12:34, Stefan Eissing wrote:




Am 31.03.2022 um 11:55 schrieb Ruediger Pluem :



On 3/31/22 11:11 AM, Ruediger Pluem wrote:



On 3/30/22 4:42 PM, jfcl...@apache.org wrote:

Author: jfclere
Date: Wed Mar 30 14:42:14 2022
New Revision: 1899390

URL: http://svn.apache.org/viewvc?rev=1899390=rev
Log:
Add WorkerBalancerGrowth. To allow creation of workers
to dynamically added balancers.

Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/proxy/mod_proxy.c
httpd/httpd/trunk/modules/proxy/mod_proxy.h

Modified: httpd/httpd/trunk/CHANGES
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff
==
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
@@ -1,6 +1,10 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.1

+ *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
+ balancer created dynamically or via "empty" 
+ [Jean-Frederic Clere]


I am not sure why this is needed. You can already do this via





Or


ProxySet growth=10



FYI: Travis trunk also fails almost completely. Does not seem to accept a proxy 
configuration.


I have done the PR and I am now waiting on Travis.





Regards

Rüdiger





--
Cheers

Jean-Frederic

Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

[jean-frederic clere]

On 01/04/2022 08:47, jean-frederic clere wrote:

On 31/03/2022 12:59, Ruediger Pluem wrote:



On 3/31/22 12:34 PM, Stefan Eissing wrote:




Am 31.03.2022 um 11:55 schrieb Ruediger Pluem :



On 3/31/22 11:11 AM, Ruediger Pluem wrote:



On 3/30/22 4:42 PM, jfcl...@apache.org wrote:

Author: jfclere
Date: Wed Mar 30 14:42:14 2022
New Revision: 1899390

URL: http://svn.apache.org/viewvc?rev=1899390=rev
Log:
Add WorkerBalancerGrowth. To allow creation of workers
to dynamically added balancers.

Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/proxy/mod_proxy.c
httpd/httpd/trunk/modules/proxy/mod_proxy.h

Modified: httpd/httpd/trunk/CHANGES
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff 

== 


--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
@@ -1,6 +1,10 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.1

+ *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
+ balancer created dynamically or via "empty" 
+ [Jean-Frederic Clere]


I am not sure why this is needed. You can already do this via





Or


ProxySet growth=10



FYI: Travis trunk also fails almost completely. Does not seem to 
accept a proxy configuration.


This is because the if

+    if (!ap_strchr_c(conf->p, ':'))
+    return apr_pstrcat(cmd->pool, thiscmd->name,
+   "> arguments are not supported for non 
url.",

+   NULL);

should not return with an error, but just encapsulate the remainder of 
the block. And I think the further

return apr_pstrcat are also wrong.

But as said I am not sure about the purpose at all as you can already 
do, what the patch should provide if I understand the patch

correctly.


The purpose was to be able to add a balancer in the balancer-manager 
handle but that needs to pre-create the mutex and the slots for the 
workers.


While looking to that I noted that:



was doing nothing, the balancer is ignored, I should I revert the patch 
and add an error message if there is an empty entry like this one?


There is also the BalancerGrowth directive that does nothing else than 
creating a memory slot for balancers we never add/create in the 
balancer-manager, I am tempted to remove it...


Would it be better to add the missing logic? I have some pieces in 
mod_proxy_cluster (https://github.com/modcluster/mod_proxy_cluster that 
could use the logic.




Regards

Rüdiger






--
Cheers

Jean-Frederic

Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

[Stefan Eissing]

> Am 01.04.2022 um 08:47 schrieb jean-frederic clere :
> 
> On 31/03/2022 12:59, Ruediger Pluem wrote:
>> On 3/31/22 12:34 PM, Stefan Eissing wrote:
>>> 
>>> 
 Am 31.03.2022 um 11:55 schrieb Ruediger Pluem :
 
 
 
 On 3/31/22 11:11 AM, Ruediger Pluem wrote:
> 
> 
> On 3/30/22 4:42 PM, jfcl...@apache.org wrote:
>> Author: jfclere
>> Date: Wed Mar 30 14:42:14 2022
>> New Revision: 1899390
>> 
>> URL: http://svn.apache.org/viewvc?rev=1899390=rev
>> Log:
>> Add WorkerBalancerGrowth. To allow creation of workers
>> to dynamically added balancers.
>> 
>> Modified:
>> httpd/httpd/trunk/CHANGES
>> httpd/httpd/trunk/modules/proxy/mod_proxy.c
>> httpd/httpd/trunk/modules/proxy/mod_proxy.h
>> 
>> Modified: httpd/httpd/trunk/CHANGES
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff
>> ==
>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>> +++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
>> @@ -1,6 +1,10 @@
>> -*- coding: utf-8 -*-
>> Changes with Apache 2.5.1
>> 
>> + *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
>> + balancer created dynamically or via "empty" 
>> + [Jean-Frederic Clere]
> 
> I am not sure why this is needed. You can already do this via
> 
> 
> 
 
 Or
 
 
 ProxySet growth=10
 
>>> 
>>> FYI: Travis trunk also fails almost completely. Does not seem to accept a 
>>> proxy configuration.
>> This is because the if
>> + if (!ap_strchr_c(conf->p, ':'))
>> + return apr_pstrcat(cmd->pool, thiscmd->name,
>> + "> arguments are not supported for non url.",
>> + NULL);
>> should not return with an error, but just encapsulate the remainder of the 
>> block. And I think the further
>> return apr_pstrcat are also wrong.
>> But as said I am not sure about the purpose at all as you can already do, 
>> what the patch should provide if I understand the patch
>> correctly.
> 
> The purpose was to be able to add a balancer in the balancer-manager handle 
> but that needs to pre-create the mutex and the slots for the workers.
> 
> While looking to that I noted that:
> 
> 
> 
> was doing nothing, the balancer is ignored, I should I revert the patch and 
> add an error message if there is an empty entry like this one?

I am much in favour of making CI work again, as I am currently flying blind on 
my changes and PRs. However that works best for you.

Kind Regards,
Stefan

> 
>> Regards
>> Rüdiger
> 
> 
> -- 
> Cheers
> 
> Jean-Frederic

Re: svn commit: r1899390 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h

[jean-frederic clere]

On 31/03/2022 12:59, Ruediger Pluem wrote:



On 3/31/22 12:34 PM, Stefan Eissing wrote:




Am 31.03.2022 um 11:55 schrieb Ruediger Pluem :



On 3/31/22 11:11 AM, Ruediger Pluem wrote:



On 3/30/22 4:42 PM, jfcl...@apache.org wrote:

Author: jfclere
Date: Wed Mar 30 14:42:14 2022
New Revision: 1899390

URL: http://svn.apache.org/viewvc?rev=1899390=rev
Log:
Add WorkerBalancerGrowth. To allow creation of workers
to dynamically added balancers.

Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/proxy/mod_proxy.c
httpd/httpd/trunk/modules/proxy/mod_proxy.h

Modified: httpd/httpd/trunk/CHANGES
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1899390=1899389=1899390=diff
==
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Wed Mar 30 14:42:14 2022
@@ -1,6 +1,10 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.1

+ *) mod_proxy: Add WorkerBalancerGrowth to allow adding workers to
+ balancer created dynamically or via "empty" 
+ [Jean-Frederic Clere]


I am not sure why this is needed. You can already do this via





Or


ProxySet growth=10



FYI: Travis trunk also fails almost completely. Does not seem to accept a proxy 
configuration.


This is because the if

+if (!ap_strchr_c(conf->p, ':'))
+return apr_pstrcat(cmd->pool, thiscmd->name,
+   "> arguments are not supported for non url.",
+   NULL);

should not return with an error, but just encapsulate the remainder of the 
block. And I think the further
return apr_pstrcat are also wrong.

But as said I am not sure about the purpose at all as you can already do, what 
the patch should provide if I understand the patch
correctly.


The purpose was to be able to add a balancer in the balancer-manager 
handle but that needs to pre-create the mutex and the slots for the workers.


While looking to that I noted that:



was doing nothing, the balancer is ignored, I should I revert the patch 
and add an error message if there is an empty entry like this one?




Regards

Rüdiger



--
Cheers

Jean-Frederic