On Sun, May 8, 2016 at 12:30 PM, <rj...@apache.org> 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) {
@@ -522,6 +523,8 @@ static int update_child_status_internal(int child_
if (c) {
apr_snprintf(ws->vhost, sizeof(ws->vhost), "%s:%d",
s->server_hostname, c->local_addr->port);
+ apr_cpystrn(c->sb_lastvhost, ws->vhost,
+ sizeof(c->sb_lastvhost));
}
else {
apr_cpystrn(ws->vhost, s->server_hostname, sizeof(ws->vhost));
@@ -528,7 +531,7 @@ static int update_child_status_internal(int child_
}
}
else if (c) {
- ws->vhost[0]='\0';
+ apr_cpystrn(ws->vhost, c->sb_lastvhost, sizeof(ws->vhost));
}
if (c) {
