Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Rainer Jung]

Am 08.05.2016 um 20:06 schrieb Yann Ylavic:

[top posting reodered]

On Sun, May 8, 2016 at 7:21 PM, Stefan Eissing
 wrote:



Am 08.05.2016 um 16:30 schrieb Rainer Jung :

If that would be consensus, it would mean, we should only reset the request 
info at the start of a connection. For sync connections, that's exactly what 
happens already. For http2 I'm not sure. F


Isn't conn_rec->keepalives an indicator?


Yes, !c->keepalives would work equivalently, but the issue is that
with mpm_event, at BUSY_READ time, the previous scoreboard entry is
not necessarily for the same connection, hence we could keep the
request-line/vhost related to another connection (mixup discussed in
PR 59333).

That's why I proposed to restore the previous request's informations
saved in the conn_rec itself, and since at the start of the connection
those would be blank, this also blanks the scoreboard entry when
necessary.


Ah, OK, then your patch is way better.

Regards,

Rainer

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

[top posting reodered]

On Sun, May 8, 2016 at 7:21 PM, Stefan Eissing
 wrote:
>
>> Am 08.05.2016 um 16:30 schrieb Rainer Jung :
>>
>> If that would be consensus, it would mean, we should only reset the request 
>> info at the start of a connection. For sync connections, that's exactly what 
>> happens already. For http2 I'm not sure. F
>
> Isn't conn_rec->keepalives an indicator?

Yes, !c->keepalives would work equivalently, but the issue is that
with mpm_event, at BUSY_READ time, the previous scoreboard entry is
not necessarily for the same connection, hence we could keep the
request-line/vhost related to another connection (mixup discussed in
PR 59333).

That's why I proposed to restore the previous request's informations
saved in the conn_rec itself, and since at the start of the connection
those would be blank, this also blanks the scoreboard entry when
necessary.

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Stefan Eissing]

Isn't conn_rec->keepalives an indicator?

/Stefan

> Am 08.05.2016 um 16:30 schrieb Rainer Jung :
> 
> If that would be consensus, it would mean, we should only reset the request 
> info at the start of a connection. For sync connections, that's exactly what 
> happens already. For http2 I'm not sure. F

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Rainer Jung]

Am 08.05.2016 um 16:30 schrieb Rainer Jung:

Am 08.05.2016 um 14:29 schrieb Yann Ylavic:

On Sun, May 8, 2016 at 12:30 PM,   wrote:


+   * Don't globber scoreboard request info if read_request_line()
fails with
+ a timeout. In that case there's not yet any new useful request
info
+ available.
+ Noticed via server-status showing request "NULL" after keep-alive
+ connections timed out.
+ No CHANGES entry needed, because there's already one for PR 59333.
+ Sorry for the two patches. The second is better. Both change
the same
+ line, so technically only the second is needed, but merging
both keeps
+ mergeinfo more complete.
+ trunk patch: http://svn.apache.org/r1742791
+  http://svn.apache.org/r1742792


This is to avoid "NULL" instead of blank, right?


No, "NULL" instead of the request line from the previous request.


ISTM that the request line has already been clobbered (blanked) by the
previous call to ap_update_child_status_from_conn(SERVER_BUSY_READ) in
ap_process_http_[a]sync_connection(), where we already lost the
previous request's line.


I observed the problem for prefork. The sync connection loop differs
from the async loop in that it calls ap_update_child_status_from_conn()
only once and then loops over the requests. The async loop calls it
before each request. That's why I observed it for prefork. For event you
are right, the field would've already been clobbered.


That's why I proposed to save the previous request line and host in
the conn_rec, and use them in the scoreboard until a new (real)
request is available (it didn't work as expected because the
scoreboard was clobbered in multiple places before Bill's changes, but
it should work better/easily now).


I guess we need to define, at which point in time we want the shown data
to switch between previous conn/req and current conn/req and how much
consistency we need for the conn data vs. the req data. conn data would
be client IP, req data request line and vhost.

I'd say when a new connection comes in, it would be OK to immediately
reset the data, set client IP and keep conn and req data consistent.

For further steps in an existing connection, I'd prefer to keep the data
until another request was read, so especially if a timeout prevented the
next one from being read.

One problem is if the client IP comes from a header (mod_remoteip). In
that case, we would have to switch behavior for a new connection,
because we don't know (yet) the client IP when the new connection
starts. Or we accept to use the connection client IP until the first
request comes in. IMHO that would be OK.

If that would be consensus, it would mean, we should only reset the
request info at the start of a connection. For sync connections, that's
exactly what happens already. For http2 I'm not sure. For async
connections I don't know, whether we can identify from the conn_rec,
whether we are at the start of the connection or not. If we could, then
we could simply adjust ap_update_child_status_from_conn() to only
overwrite the request info with the empty string if we are at the start
of the connection. I don't think we really need to safe the full request
line and vhost in the conn_rec, a flag that tells us whether we are at
start or not should suffice.


That would be: when no r is available but c is, use the saved
request-line/host from c; when r is available, use the ones from r and
update them in c.
That way there would be no blank at all (IOW, the scoreboard entry
would be left with the latest request handled on the connection, not a
blank which inevitably ends every connection after keepalive timeout
or close, and shouldn't be taken into account as an empty request
IMHO).


Agreed. The only blank that might be OK is after a new connection came
in, so we update the client IP address. Then having the new IP address
but the old request line would be inconsistent data, so it would be
better to zero the request info.


Also, that would be consistent in both MPMs worker and event, whereas
currently event is more likely to show this behaviour, thanks to
queuing (no worker thread used for keepalive handling and hence no
BUSY_READ during that time, no clobbering either).
Thus for mpm_event, we'd need to use
ap_update_child_status_from_conn(SERVER_CLOSING, c) so that the saved
request-line/vhost from the connection get reused when/if a worker
thread handles the lingering close.

How about the attached patch?


What about the attached one using a flag? (plus minor MMN change)

Regards,

Rainer

Index: include/httpd.h
===
--- include/httpd.h	(revision 1742822)
+++ include/httpd.h	(working copy)
@@ -1217,6 +1217,9 @@

 /** The minimum level of filter type to allow setaside buckets */
 int async_filter;
+
+/** Did we already see a request? Used by the scoreboard. */
+unsigned short request_seen;
 };

 struct conn_slave_rec {

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Rainer Jung]

Am 08.05.2016 um 14:29 schrieb Yann Ylavic:

On Sun, May 8, 2016 at 12:30 PM,   wrote:


+   * Don't globber scoreboard request info if read_request_line() fails with
+ a timeout. In that case there's not yet any new useful request info
+ available.
+ Noticed via server-status showing request "NULL" after keep-alive
+ connections timed out.
+ No CHANGES entry needed, because there's already one for PR 59333.
+ Sorry for the two patches. The second is better. Both change the same
+ line, so technically only the second is needed, but merging both keeps
+ mergeinfo more complete.
+ trunk patch: http://svn.apache.org/r1742791
+  http://svn.apache.org/r1742792


This is to avoid "NULL" instead of blank, right?


No, "NULL" instead of the request line from the previous request.


ISTM that the request line has already been clobbered (blanked) by the
previous call to ap_update_child_status_from_conn(SERVER_BUSY_READ) in
ap_process_http_[a]sync_connection(), where we already lost the
previous request's line.


I observed the problem for prefork. The sync connection loop differs 
from the async loop in that it calls ap_update_child_status_from_conn() 
only once and then loops over the requests. The async loop calls it 
before each request. That's why I observed it for prefork. For event you 
are right, the field would've already been clobbered.



That's why I proposed to save the previous request line and host in
the conn_rec, and use them in the scoreboard until a new (real)
request is available (it didn't work as expected because the
scoreboard was clobbered in multiple places before Bill's changes, but
it should work better/easily now).


I guess we need to define, at which point in time we want the shown data 
to switch between previous conn/req and current conn/req and how much 
consistency we need for the conn data vs. the req data. conn data would 
be client IP, req data request line and vhost.


I'd say when a new connection comes in, it would be OK to immediately 
reset the data, set client IP and keep conn and req data consistent.


For further steps in an existing connection, I'd prefer to keep the data 
until another request was read, so especially if a timeout prevented the 
next one from being read.


One problem is if the client IP comes from a header (mod_remoteip). In 
that case, we would have to switch behavior for a new connection, 
because we don't know (yet) the client IP when the new connection 
starts. Or we accept to use the connection client IP until the first 
request comes in. IMHO that would be OK.


If that would be consensus, it would mean, we should only reset the 
request info at the start of a connection. For sync connections, that's 
exactly what happens already. For http2 I'm not sure. For async 
connections I don't know, whether we can identify from the conn_rec, 
whether we are at the start of the connection or not. If we could, then 
we could simply adjust ap_update_child_status_from_conn() to only 
overwrite the request info with the empty string if we are at the start 
of the connection. I don't think we really need to safe the full request 
line and vhost in the conn_rec, a flag that tells us whether we are at 
start or not should suffice.



That would be: when no r is available but c is, use the saved
request-line/host from c; when r is available, use the ones from r and
update them in c.
That way there would be no blank at all (IOW, the scoreboard entry
would be left with the latest request handled on the connection, not a
blank which inevitably ends every connection after keepalive timeout
or close, and shouldn't be taken into account as an empty request
IMHO).


Agreed. The only blank that might be OK is after a new connection came 
in, so we update the client IP address. Then having the new IP address 
but the old request line would be inconsistent data, so it would be 
better to zero the request info.



Also, that would be consistent in both MPMs worker and event, whereas
currently event is more likely to show this behaviour, thanks to
queuing (no worker thread used for keepalive handling and hence no
BUSY_READ during that time, no clobbering either).
Thus for mpm_event, we'd need to use
ap_update_child_status_from_conn(SERVER_CLOSING, c) so that the saved
request-line/vhost from the connection get reused when/if a worker
thread handles the lingering close.

How about the attached patch?


Looks reasonable if we want to safe the strings in the conn_rec. I'll 
give it a try.


Regards,

Rainer

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

On Sun, May 8, 2016 at 12:30 PM,   wrote:
>
> +   * Don't globber scoreboard request info if read_request_line() fails with
> + a timeout. In that case there's not yet any new useful request info
> + available.
> + Noticed via server-status showing request "NULL" after keep-alive
> + connections timed out.
> + No CHANGES entry needed, because there's already one for PR 59333.
> + Sorry for the two patches. The second is better. Both change the same
> + line, so technically only the second is needed, but merging both keeps
> + mergeinfo more complete.
> + trunk patch: http://svn.apache.org/r1742791
> +  http://svn.apache.org/r1742792

This is to avoid "NULL" instead of blank, right?

ISTM that the request line has already been clobbered (blanked) by the
previous call to ap_update_child_status_from_conn(SERVER_BUSY_READ) in
ap_process_http_[a]sync_connection(), where we already lost the
previous request's line.

That's why I proposed to save the previous request line and host in
the conn_rec, and use them in the scoreboard until a new (real)
request is available (it didn't work as expected because the
scoreboard was clobbered in multiple places before Bill's changes, but
it should work better/easily now).
That would be: when no r is available but c is, use the saved
request-line/host from c; when r is available, use the ones from r and
update them in c.
That way there would be no blank at all (IOW, the scoreboard entry
would be left with the latest request handled on the connection, not a
blank which inevitably ends every connection after keepalive timeout
or close, and shouldn't be taken into account as an empty request
IMHO).

Also, that would be consistent in both MPMs worker and event, whereas
currently event is more likely to show this behaviour, thanks to
queuing (no worker thread used for keepalive handling and hence no
BUSY_READ during that time, no clobbering either).
Thus for mpm_event, we'd need to use
ap_update_child_status_from_conn(SERVER_CLOSING, c) so that the saved
request-line/vhost from the connection get reused when/if a worker
thread handles the lingering close.

How about the attached patch?

Regards,
Yann.
Index: include/httpd.h
===
--- include/httpd.h	(revision 1742803)
+++ include/httpd.h	(working copy)
@@ -1095,6 +1095,9 @@ typedef enum {
 AP_CONN_KEEPALIVE
 } ap_conn_keepalive_e;
 
+/* AP_SB_*_SIZE needed by conn_rec */
+#include "scoreboard.h"
+
 /**
  * @brief Structure to store things which are per connection
  */
@@ -1217,6 +1220,10 @@ struct conn_rec {
 
 /** The minimum level of filter type to allow setaside buckets */
 int async_filter;
+
+/** Preserve scoreboard's worker last request infos */
+char sb_lastrline[AP_SB_RLINE_SIZE];
+char sb_lastvhost[AP_SB_VHOST_SIZE];
 };
 
 struct conn_slave_rec {
Index: include/scoreboard.h
===
--- include/scoreboard.h	(revision 1742803)
+++ include/scoreboard.h	(working copy)
@@ -85,6 +85,9 @@ typedef enum {
 SB_SHARED = 2
 } ap_scoreboard_e;
 
+#define AP_SB_RLINE_SIZE 64
+#define AP_SB_VHOST_SIZE 32
+
 /* stuff which is worker specific */
 typedef struct worker_score worker_score;
 struct worker_score {
@@ -113,8 +116,8 @@ struct worker_score {
 struct tms times;
 #endif
 char client[40];/* Keep 'em small... but large enough to hold an IPv6 address */
-char request[64];   /* We just want an idea... */
-char vhost[32]; /* What virtual host is being accessed? */
+char request[AP_SB_RLINE_SIZE]; /* We just want an idea... */
+char vhost[AP_SB_VHOST_SIZE];   /* What virtual host is being accessed? */
 char protocol[16];  /* What protocol is used on the connection? */
 };
 
Index: server/connection.c
===
--- server/connection.c	(revision 1742803)
+++ server/connection.c	(working copy)
@@ -101,7 +101,7 @@ AP_DECLARE(int) ap_prep_lingering_close(conn_rec *
 ap_run_pre_close_connection(c);
 
 if (c->sbh) {
-ap_update_child_status(c->sbh, SERVER_CLOSING, NULL);
+ap_update_child_status_from_conn(c->sbh, SERVER_CLOSING, c);
 }
 return 0;
 }
Index: server/scoreboard.c
===
--- server/scoreboard.c	(revision 1742803)
+++ server/scoreboard.c	(working copy)
@@ -499,9 +499,10 @@ static int update_child_status_internal(int child_
 }
 else if (r) {
 copy_request(ws->request, sizeof(ws->request), r);
+apr_cpystrn(c->sb_lastrline, ws->request, sizeof(c->sb_lastrline));
 }
 else if (c) {
-ws->request[0]='\0';
+apr_cpystrn(ws->request, c->sb_lastrline, sizeof(ws->request));
 }
 
 if (r && r->useragent_ip) {
@@ 

Bug report for Apache httpd-2 [2016/05/08]

[bugzilla]

+---+
| Bugzilla Bug ID   |
| +-+
| | Status: UNC=Unconfirmed NEW=New ASS=Assigned|
| | OPN=ReopenedVER=Verified(Skipped Closed/Resolved)   |
| |   +-+
| |   | Severity: BLK=Blocker CRI=Critical  REG=Regression  MAJ=Major   |
| |   |   MIN=Minor   NOR=NormalENH=Enhancement TRV=Trivial |
| |   |   +-+
| |   |   | Date Posted |
| |   |   |  +--+
| |   |   |  | Description  |
| |   |   |  |  |
| 8713|Inf|Min|2002-05-01|No Errorlog on PROPFIND/Depth:Infinity|
| 8867|Opn|Cri|2002-05-07|exports.c generation fails when using a symlink to|
|10747|New|Maj|2002-07-12|ftp SIZE command and 'smart' ftp servers results i|
|11294|New|Enh|2002-07-30|desired vhost_alias option|
|11580|Opn|Enh|2002-08-09|generate Content-Location headers |
|12033|Opn|Nor|2002-08-26|Graceful restart immediately result in [warn] long|
|13599|Inf|Nor|2002-10-14|autoindex formating broken for multibyte sequences|
|13661|Ass|Enh|2002-10-15|Apache cannot not handle dynamic IP reallocation  |
|14104|Opn|Enh|2002-10-30|not documented: must restart server to load new CR|
|14496|New|Enh|2002-11-13|Cannot upgrade any version on Windows. Must uninst|
|14922|Inf|Enh|2002-11-28| is currently hardcoded to 'apache2'  |
|15719|Inf|Nor|2002-12-30|WebDAV MOVE to destination URI which is content-ne|
|16761|Inf|Nor|2003-02-04|CustomLog with pipe spawns process during config  |
|16802|New|Enh|2003-02-05|Additional AllowOverride directive "Restrict" |
|16811|Ass|Maj|2003-02-05|mod_autoindex always return webpages in UTF-8.|
|17107|New|Min|2003-02-16|Windows should not install printenv   |
|17114|New|Enh|2003-02-17|Please add strip and install-strip targets to Make|
|17244|Ass|Nor|2003-02-20|./configure --help gives false information regardi|
|17497|Opn|Nor|2003-02-27|mod_mime_magic generates incorrect response header|
|18325|New|Enh|2003-03-25|PAM support for suEXEC|
|18334|Inf|Cri|2003-03-25|Server crashes when authenticating users against L|
|19043|New|Min|2003-04-15|Interesting interaction between cern_meta module a|
|19670|New|Enh|2003-05-05|content type header supplied upon PUT is thrown aw|
|20036|Ass|Nor|2003-05-19|Trailing Dots stripped from PATH_INFO environment |
|21260|New|Nor|2003-07-02|CacheMaxExpire directive not enforced !   |
|21533|Ass|Cri|2003-07-11|Multiple levels of htacces files can cause mod_aut|
|22237|New|Enh|2003-08-08|option to disable ServerSignature on index pages  |
|22484|Opn|Maj|2003-08-16|semaphore problem takes httpd down|
|22686|Opn|Nor|2003-08-25|ab: apr_poll: The timeout specified has expired (7|
|22898|Opn|Nor|2003-09-02|nph scripts with two HTTP header  |
|23167|Inf|Cri|2003-09-14|--enable-layout never goes to apr apr-util|
|23181|New|Nor|2003-09-15|Status 304 (Not modified) and chunking leads to an|
|23238|New|Cri|2003-09-18|non-async-signal-safe operations from signal handl|
|23330|New|Enh|2003-09-22|Enhance ApacheMonitor to view and control Tomcat s|
|23911|Opn|Cri|2003-10-18|CGI processes left defunct/zombie under 2.0.54|
|24031|New|Enh|2003-10-23|Passphrase protected private key in SSLProxyMachin|
|24095|Opn|Cri|2003-10-24|ERROR "Parent: child process exited with status 32|
|24437|Opn|Nor|2003-11-05|mod_auth_ldap doubly-escapes backslash (\) charact|
|24890|Opn|Nor|2003-11-21|Apache config parser should not be local aware ( g|
|25014|New|Enh|2003-11-26|A flexible interface for mod_log_config   |
|25201|New|Enh|2003-12-04|Provide Cache Purge operation |
|25240|Inf|Enh|2003-12-05|SSL Library Error: 336105671 logged as information|
|25435|New|Enh|2003-12-11|sethandler and directoryindex not playing nice|
|25469|Opn|Enh|2003-12-12|create AuthRoot for defining paths to auth files  |
|25484|Ass|Nor|2003-12-12|Non-service Apache cannot be stopped in WinXP |
|25543|Inf|Nor|2003-12-15|mod_proxy_ajp overwrites existing response headers|
|25667|New|Nor|2003-12-19|Memory leak in function ssl_scache_dbm_retrieve().|
|25863|New|Enh|2004-01-02|new per-host initialization hooks |
|26005|New|Nor|2004-01-08|SERVER_NAME incorrect when using IPv6 address in U|
|26142|New|Maj|2004-01-14|EnableSendFile Off for Windows XP Home|
|26153|Opn|Cri|2004-01-15|Apache cygwin directory traversal vulnerability   |