RFC: Documenting changes in the CHANGES file
Reviewing our backport process I noticed that in many cases a clean merge via svn merge fails due to conflicts in CHANGES. While these are easy to solve it puts IMHO unnecessary extra work on the backport process, both for reviewing and for actually doing the backport. How about if we change the way we document changes the following way: 1. We create a changes-fragments directory (name to be determined) at the top level. 2. For each release we create a subdirectory such that we end up with the following structure: changes-fragments/ 2.4.41/ 2.4.42/ 2.4.43/ 2.4.44/ 3. Each directory contains the changes for each release and each change entry is a single file. 4. We have a script that builds our current CHANGES file from the content in changes-fragments directories with the help of a template or at least some sort of header / footer that is static. 5. This script can be called either manually and we commit the resulting CHANGES file as we like just like the x-forms commits for documentation plus this script is called by the release scripts from Daniel as part of the preparation of rolling a release. Regards Rüdiger
Re: Trying to understand the logic of mod_proxy_http2 with regards to timeouts
On 5/29/20 11:41 AM, Stefan Eissing wrote:
>
>
>> Am 29.05.2020 um 11:23 schrieb Ruediger Pluem :
>>
>>
>>
>> On 5/29/20 10:09 AM, Stefan Eissing wrote:
>>> Looks good. Now I learned about the "ping" parameter...
>>
>> Committed as r1878264 with a tweaked comment to make clear what I do.
Thanks for backporting both
>>
>> Getting me to the next possible enhancement. I already had a patch but when
>> putting it to the mail I got doubts that it could work
>> due to the fact, that in most use cases HTTP/2 is encrypted.
>> In AJP a set ping parameter on the worker will also cause an AJP ping to be
>> sent as the first thing even on fresh connections and
>> we only wait for the timeout set in the parameter for a reply. The idea
>> behind this is that the backend might be able to deal with
>> a TCP handshake, but not with processing a request, because maybe all
>> processing threads / processes on the backend application
>> are busy. This way this can be detected quickly and early and we can sent
>> our request to a different backend in case of a load
>> balancing scenario.
>> With HTTP/2 we will likely have a TLS handshake first which likely already
>> requires a responding backend application. So it would
>> not work to wait only for ping timeout time on a ping reply as we will
>> already wait for the timeout set on the socket to get an
>> answer to our TLS client Hello. So the idea would be to already lower the
>> socket timeout to ping timeout before the TLS handshake
>> starts and reset it back after we received the ping from the backend.
>> Opinions?
>
> HTTP/2 also has an initial SETTINGS frame handshake. We could use the ping
> timeout on the socket until the first NGHTTP2_SETTINGS frame from the backend
> arrives on a new connection.
And now I learned about the HTTP/2 handshake :-). I haven't though about the
SETTINGS frame. So the high level idea would be:
1. If a fresh session in h2_proxy_session_setup is created and the ping
parameter is set on the worker change the socket timeout
to the one in ping, but tuck away the current socket timeout.
2. In on_frame_recv in the NGHTTP2_SETTINGS restore the old timeout (if it was
changed) and continue.
So something like the below?
Index: modules/http2/h2_proxy_session.c
===
--- modules/http2/h2_proxy_session.c(revision 1878265)
+++ modules/http2/h2_proxy_session.c(working copy)
@@ -203,6 +203,21 @@
case NGHTTP2_PUSH_PROMISE:
break;
case NGHTTP2_SETTINGS:
+/*
+ * Check if we have saved a socket timeout before sending the
+ * SETTINGS frame in h2_proxy_session_setup to perform a quick
+ * "ping" on the backend. If yes, restore the saved timeout to
+ * the socket.
+ */
+if (session->save_timeout != -1) {
+apr_socket_t *socket = NULL;
+
+socket = ap_get_conn_socket(session->c);
+if (socket) {
+apr_socket_timeout_set(socket, session->save_timeout);
+session->save_timeout = -1;
+}
+}
if (frame->settings.niv > 0) {
n = nghttp2_session_get_remote_settings(ngh2,
NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS);
if (n > 0) {
@@ -634,6 +649,8 @@
session->input = apr_brigade_create(session->pool,
session->c->bucket_alloc);
session->output = apr_brigade_create(session->pool,
session->c->bucket_alloc);
+session->save_timeout = -1;
+
nghttp2_session_callbacks_new();
nghttp2_session_callbacks_set_on_frame_recv_callback(cbs,
on_frame_recv);
nghttp2_session_callbacks_set_on_data_chunk_recv_callback(cbs,
stream_response_data);
@@ -654,6 +671,22 @@
nghttp2_option_del(option);
nghttp2_session_callbacks_del(cbs);
+/*
+ * If a ping parameter is set on the worker set the socket timeout to
+ * it to "use" the possible TLS handshake and the initial SETTINGS
+ * frame as kind of ping. Tuck away the old timeout to restore it, once
+ * the SETTING frame arrived from the backend.
+ */
+if (p_conn->worker->s->ping_timeout_set) {
+apr_socket_t *socket = NULL;
+
+socket = ap_get_conn_socket(session->c);
+if (socket) {
+apr_socket_timeout_get(socket, >save_timeout);
+apr_socket_timeout_set(socket,
p_conn->worker->s->ping_timeout);
+}
+}
+
ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, APLOGNO(03362)
"setup session for %s", p_conn->hostname);
}
Index: modules/http2/h2_proxy_session.h
===
--- modules/http2/h2_proxy_session.h(revision 1878265)
+++ modules/http2/h2_proxy_session.h(working copy)
@@ -94,6
Re: svn commit: r1878280 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c
On 5/29/20 7:05 PM, yla...@apache.org wrote: > Author: ylavic > Date: Fri May 29 17:05:29 2020 > New Revision: 1878280 > > URL: http://svn.apache.org/viewvc?rev=1878280=rev > Log: > mod_proxy_http: don't strip EOS when spooling request body to file. > > To prevent stream_reqbody() from sending the FILE and EOS bucket in separate > brigades, and thus apr_file_setaside() to trigger if network congestion occurs > with the backend, restore the EOS in spool_reqbody_cl() which was stripped > when spooling the request body to a file. > > Until APR r1878279 is released (and installed by users), apr_file_setaside() > on a temporary file (mktemp) will simply drop the file cleanup, leaking the > fd and inode.. > > This fixes BZ 64452. This was a really tricky and nasty one. Excellent debug job. Plus it brought us nice improvements to .gdbinit :-) Regards Rüdiger
Warning free code
Once in a while I post the build warnings in Windows 32/64. Till now this is not really picked up, some times it is solved. Should it not be a goal to get warning free code on all platforms ? What do you think. Steffen
Re: svn commit: r1878268 - /httpd/httpd/trunk/CHANGES
On Fri, May 29, 2020 at 8:14 AM Stefan Eissing wrote: > > > > Am 29.05.2020 um 13:53 schrieb Yann Ylavic : > > > > On Fri, May 29, 2020 at 12:25 PM wrote: > >> > >> Changes with Apache 2.5.1 > >> > >> - *) mod_proxy_http2: respect ProxyTimeout settings on backend connections > >> - while waiting on incoming data. [Ruediger Pluem, Stefan Eissing] > > > > This is still a change between 2.5.0 and 2.5.1, so I think we should > > not remove 2.5.x CHANGES entries now that 2.5.0 is tagged, even if the > > same change was backported to 2.4.x. > > Which means we do not remove something in trunk/CHANGES until a new 2.5 is > made? I think longer, until 2.5.x is an actual branch.
Re: svn commit: r1878268 - /httpd/httpd/trunk/CHANGES
> Am 29.05.2020 um 13:53 schrieb Yann Ylavic : > > On Fri, May 29, 2020 at 12:25 PM wrote: >> >> Changes with Apache 2.5.1 >> >> - *) mod_proxy_http2: respect ProxyTimeout settings on backend connections >> - while waiting on incoming data. [Ruediger Pluem, Stefan Eissing] > > This is still a change between 2.5.0 and 2.5.1, so I think we should > not remove 2.5.x CHANGES entries now that 2.5.0 is tagged, even if the > same change was backported to 2.4.x. Which means we do not remove something in trunk/CHANGES until a new 2.5 is made?
Re: svn commit: r1878268 - /httpd/httpd/trunk/CHANGES
On Fri, May 29, 2020 at 12:25 PM wrote: > > Changes with Apache 2.5.1 > > - *) mod_proxy_http2: respect ProxyTimeout settings on backend connections > - while waiting on incoming data. [Ruediger Pluem, Stefan Eissing] This is still a change between 2.5.0 and 2.5.1, so I think we should not remove 2.5.x CHANGES entries now that 2.5.0 is tagged, even if the same change was backported to 2.4.x. Cheers, Yann.
Re: Trying to understand the logic of mod_proxy_http2 with regards to timeouts
> Am 29.05.2020 um 11:23 schrieb Ruediger Pluem : > > > > On 5/29/20 10:09 AM, Stefan Eissing wrote: >> Looks good. Now I learned about the "ping" parameter... > > Committed as r1878264 with a tweaked comment to make clear what I do. > > Getting me to the next possible enhancement. I already had a patch but when > putting it to the mail I got doubts that it could work > due to the fact, that in most use cases HTTP/2 is encrypted. > In AJP a set ping parameter on the worker will also cause an AJP ping to be > sent as the first thing even on fresh connections and > we only wait for the timeout set in the parameter for a reply. The idea > behind this is that the backend might be able to deal with > a TCP handshake, but not with processing a request, because maybe all > processing threads / processes on the backend application > are busy. This way this can be detected quickly and early and we can sent our > request to a different backend in case of a load > balancing scenario. > With HTTP/2 we will likely have a TLS handshake first which likely already > requires a responding backend application. So it would > not work to wait only for ping timeout time on a ping reply as we will > already wait for the timeout set on the socket to get an > answer to our TLS client Hello. So the idea would be to already lower the > socket timeout to ping timeout before the TLS handshake > starts and reset it back after we received the ping from the backend. > Opinions? HTTP/2 also has an initial SETTINGS frame handshake. We could use the ping timeout on the socket until the first NGHTTP2_SETTINGS frame from the backend arrives on a new connection. - Stefan
Re: Trying to understand the logic of mod_proxy_http2 with regards to timeouts
On 5/29/20 10:09 AM, Stefan Eissing wrote: > Looks good. Now I learned about the "ping" parameter... Committed as r1878264 with a tweaked comment to make clear what I do. Getting me to the next possible enhancement. I already had a patch but when putting it to the mail I got doubts that it could work due to the fact, that in most use cases HTTP/2 is encrypted. In AJP a set ping parameter on the worker will also cause an AJP ping to be sent as the first thing even on fresh connections and we only wait for the timeout set in the parameter for a reply. The idea behind this is that the backend might be able to deal with a TCP handshake, but not with processing a request, because maybe all processing threads / processes on the backend application are busy. This way this can be detected quickly and early and we can sent our request to a different backend in case of a load balancing scenario. With HTTP/2 we will likely have a TLS handshake first which likely already requires a responding backend application. So it would not work to wait only for ping timeout time on a ping reply as we will already wait for the timeout set on the socket to get an answer to our TLS client Hello. So the idea would be to already lower the socket timeout to ping timeout before the TLS handshake starts and reset it back after we received the ping from the backend. Opinions? Regards Rüdiger
AW: svn commit: r1878247 - /httpd/httpd/trunk/.gdbinit
C2 General
> -Ursprüngliche Nachricht-
> Von: Yann Ylavic
> Gesendet: Freitag, 29. Mai 2020 10:42
> An: httpd-dev
> Betreff: Re: svn commit: r1878247 - /httpd/httpd/trunk/.gdbinit
>
> CYBER SECURITY WARNING: This email is from an external source - be
> careful of attachments and links. Please follow the Cyber Code and
> report suspicious emails.
>
> On Fri, May 29, 2020 at 9:58 AM Ruediger Pluem
> wrote:
> >
> > On 5/28/20 9:54 PM, yla...@apache.org wrote:
> > > Author: ylavic
> > > Date: Thu May 28 19:54:02 2020
> > > New Revision: 1878247
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1878247=rev
> > > Log:
> > > .gdbinit: dump pool (pre_)cleanups [skip ci]
> > >
> > > Modified:
> > > httpd/httpd/trunk/.gdbinit
> >
> > Really nice one.
>
> Thanks!
>
> >
> > >
> > > Modified: httpd/httpd/trunk/.gdbinit
> > > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/.gdbinit?rev=1878247=1
> 878246=1878247=diff
> > >
>
> ==
> > > --- httpd/httpd/trunk/.gdbinit (original)
> > > +++ httpd/httpd/trunk/.gdbinit Thu May 28 19:54:02 2020
> >
> > > +c_num = 0
> > > +c = darg['pre_cleanups']
> > > +while c:
> > > +c_num = c_num + 1
> > > +dc = c.dereference()
> > > +print("%s pre_cleanup #%.2i: data = %s, plain_cleanup_fn =
> %s, child_cleanup_fn = %s" % (indent, c_num, dc['data'],
> dc['plain_cleanup_fn'].dereference(),
> dc['plain_cleanup_fn'].dereference()))
> > > +c = dc['next']
> > > +c = darg['cleanups']
> >
> > Wouldn't it make sense to do a c_num = 0 here again?
>
> I thought it could be easier, for debugging, to talk about some pool's
> cleanup number N with no ambiguity wrt pre or post cleanup.
> Not a strong opinion..
Fair enough. This is a valid point I haven't thought of.
Regards
Rüdiger
Re: svn commit: r1878247 - /httpd/httpd/trunk/.gdbinit
On Fri, May 29, 2020 at 9:58 AM Ruediger Pluem wrote:
>
> On 5/28/20 9:54 PM, yla...@apache.org wrote:
> > Author: ylavic
> > Date: Thu May 28 19:54:02 2020
> > New Revision: 1878247
> >
> > URL: http://svn.apache.org/viewvc?rev=1878247=rev
> > Log:
> > .gdbinit: dump pool (pre_)cleanups [skip ci]
> >
> > Modified:
> > httpd/httpd/trunk/.gdbinit
>
> Really nice one.
Thanks!
>
> >
> > Modified: httpd/httpd/trunk/.gdbinit
> > URL:
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/.gdbinit?rev=1878247=1878246=1878247=diff
> > ==
> > --- httpd/httpd/trunk/.gdbinit (original)
> > +++ httpd/httpd/trunk/.gdbinit Thu May 28 19:54:02 2020
>
> > +c_num = 0
> > +c = darg['pre_cleanups']
> > +while c:
> > +c_num = c_num + 1
> > +dc = c.dereference()
> > +print("%s pre_cleanup #%.2i: data = %s, plain_cleanup_fn = %s,
> > child_cleanup_fn = %s" % (indent, c_num, dc['data'],
> > dc['plain_cleanup_fn'].dereference(), dc['plain_cleanup_fn'].dereference()))
> > +c = dc['next']
> > +c = darg['cleanups']
>
> Wouldn't it make sense to do a c_num = 0 here again?
I thought it could be easier, for debugging, to talk about some pool's
cleanup number N with no ambiguity wrt pre or post cleanup.
Not a strong opinion..
Regards;
Yann.
Re: Trying to understand the logic of mod_proxy_http2 with regards to timeouts
Looks good. Now I learned about the "ping" parameter...
> Am 28.05.2020 um 21:30 schrieb Ruediger Pluem :
>
>
>
> On 5/28/20 12:06 PM, Stefan Eissing wrote:
>>
>>> Am 28.05.2020 um 12:05 schrieb Ruediger Pluem :
>>>
>>>
>>>
>>> On 5/28/20 11:36 AM, Stefan Eissing wrote:
>>>
You are correct. I made a v2 of the patch:
>>>
>>> Thanks. This one looks good.
>>
>> Thanks for reviewing this.
>
> Thanks for commiting as r1878233. One further enhancement on top of this:
>
> Index: modules/http2/h2_proxy_session.c
> ===
> --- modules/http2/h2_proxy_session.c (revision 1878243)
> +++ modules/http2/h2_proxy_session.c (working copy)
> @@ -1399,6 +1399,7 @@
> {
> apr_status_t status;
> int have_written = 0, have_read = 0;
> +apr_interval_time_t timeout;
>
> ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, session->c,
> "h2_proxy_session(%s): process", session->id);
> @@ -1441,7 +1442,14 @@
> * configured via ProxyTimeout in our socket. There is
> * nothing we want to send or check until we get more data
> * from the backend. */
> -status = h2_proxy_session_read(session, 1, 0);
> +if (session->check_ping
> +&& session->p_conn->worker->s->ping_timeout_set) {
> +timeout = session->p_conn->worker->s->ping_timeout;
> +}
> +else {
> +timeout = 0;
> +}
> +status = h2_proxy_session_read(session, 1, timeout);
> if (status == APR_SUCCESS) {
> have_read = 1;
> dispatch_event(session, H2_PROXYS_EV_DATA_READ, 0, NULL);
>
>
> This would make use of the worker ping parameter if we wait for a ping and
> the ping parameter was specified.
> Opinions (and yes if this is fine I would adjust the comment above regarding
> ProxyTimeout)?
>
> Regards
>
> Rüdiger
Re: svn commit: r1878247 - /httpd/httpd/trunk/.gdbinit
On 5/28/20 9:54 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Thu May 28 19:54:02 2020
> New Revision: 1878247
>
> URL: http://svn.apache.org/viewvc?rev=1878247=rev
> Log:
> .gdbinit: dump pool (pre_)cleanups [skip ci]
>
> Modified:
> httpd/httpd/trunk/.gdbinit
Really nice one.
>
> Modified: httpd/httpd/trunk/.gdbinit
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/.gdbinit?rev=1878247=1878246=1878247=diff
> ==
> --- httpd/httpd/trunk/.gdbinit (original)
> +++ httpd/httpd/trunk/.gdbinit Thu May 28 19:54:02 2020
> @@ -530,16 +530,29 @@ class DumpPoolAndChilds (gdb.Command):
>tag = darg['tag'].string()
> else:
>tag = "No tag"
> -print("Pool '%s' [%s]: %d/%d free (%d blocks) allocator: %s free blocks
> in allocator: %i kiB" % (tag, arg, free, size, nodes, darg['allocator'],
> self._allocator_free_blocks(darg['allocator'])))
> +print("%sPool '%s' [%s]: %d/%d free (%d blocks) allocator: %s free
> blocks in allocator: %i kiB" % (indent, tag, arg, free, size, nodes,
> darg['allocator'], self._allocator_free_blocks(darg['allocator'])))
> self.free = self.free + free
> self.size = self.size + size
> self.nodes = self.nodes + nodes
> +c_num = 0
> +c = darg['pre_cleanups']
> +while c:
> +c_num = c_num + 1
> +dc = c.dereference()
> +print("%s pre_cleanup #%.2i: data = %s, plain_cleanup_fn = %s,
> child_cleanup_fn = %s" % (indent, c_num, dc['data'],
> dc['plain_cleanup_fn'].dereference(), dc['plain_cleanup_fn'].dereference()))
> +c = dc['next']
> +c = darg['cleanups']
Wouldn't it make sense to do a c_num = 0 here again?
> +while c:
> +c_num = c_num + 1
> +dc = c.dereference()
> +print("%s pst_cleanup #%.2i: data = %s, plain_cleanup_fn = %s,
> child_cleanup_fn = %s" % (indent, c_num, dc['data'],
> dc['plain_cleanup_fn'].dereference(), dc['plain_cleanup_fn'].dereference()))
> +c = dc['next']
>
>def _dump(self, arg, depth):
> pool = arg
> +indent = "%*c" % (depth * 4 + 1, " ")
> while pool:
> -print("%*c" % (depth * 4 + 1, " "), end="")
> -self._dump_one_pool(pool)
> +self._dump_one_pool(pool, indent)
> if pool['child'] != 0:
> self._dump(pool['child'], depth + 1)
> pool = pool['sibling']
>
>
Regards
Rüdiger
