[master] 054fd6a Revisit the managers code to start and stop the child.
Poul-Henning Kamp
phk at varnish-cache.org
Tue Jan 29 15:27:53 CET 2013
commit 054fd6a6d3a79fd26d5136b0a486413bce55ff24
Author: Poul-Henning Kamp <phk at FreeBSD.org>
Date: Tue Jan 29 14:25:39 2013 +0000
Revisit the managers code to start and stop the child.
Eliminate SIGCHLD usage, it's icky, at best, when we have other
child processes (See #1256)
Instead of reaping the child on SIGCHLD, we do it explicitly,
and there is now a 10 second wait to give the child a chance to
shut down gracefully, before we take a bat to the kneecaps.
More work may be warranted, but I want to get some feedback on
this bit first.
Fixes #1256
diff --git a/bin/varnishd/mgt/mgt_child.c b/bin/varnishd/mgt/mgt_child.c
index e6f8700..3def397 100644
--- a/bin/varnishd/mgt/mgt_child.c
+++ b/bin/varnishd/mgt/mgt_child.c
@@ -57,13 +57,12 @@
#include "mgt_cli.h"
-pid_t child_pid = -1;
-
+pid_t child_pid = -1;
static struct vbitmap *fd_map;
static int child_cli_in = -1;
-static int child_VCLI_Out = -1;
+static int child_cli_out = -1;
static int child_output = -1;
static enum {
@@ -84,10 +83,11 @@ static const char * const ch_state[] = {
static struct vev *ev_poker;
static struct vev *ev_listen;
-static struct vlu *vlu;
+static struct vlu *child_std_vlu;
static struct vsb *child_panic = NULL;
+/* XXX: Doesn't really belong here, but only place we use it */
static inline int
MGT_FEATURE(enum feature_bits x)
{
@@ -95,8 +95,82 @@ MGT_FEATURE(enum feature_bits x)
(0x80U >> ((unsigned)x & 7)));
}
+static void mgt_reap_child(void);
-/*--------------------------------------------------------------------
+/*---------------------------------------------------------------------
+ * A handy little function
+ */
+
+static inline void
+closex(int *fd)
+{
+
+ assert(*fd >= 0);
+ AZ(close(*fd));
+ *fd = -1;
+}
+
+/*=====================================================================
+ * Panic string evacuation and handling
+ */
+
+static void
+mgt_panic_record(pid_t r)
+{
+ char time_str[30];
+
+ AN(heritage.panic_str[0]);
+ REPORT(LOG_ERR, "Child (%jd) Panic message: %s",
+ (intmax_t)r, heritage.panic_str);
+
+ if (child_panic != NULL)
+ VSB_delete(child_panic);
+ child_panic = VSB_new_auto();
+ AN(child_panic);
+ VTIM_format(VTIM_real(), time_str);
+ VSB_printf(child_panic, "Last panic at: %s\n", time_str);
+ VSB_cat(child_panic, heritage.panic_str);
+ AZ(VSB_finish(child_panic));
+}
+
+static void
+mgt_panic_clear(void)
+{
+ VSB_delete(child_panic);
+ child_panic = NULL;
+}
+
+void __match_proto__(cli_func_t)
+mcf_panic_show(struct cli *cli, const char * const *av, void *priv)
+{
+ (void)av;
+ (void)priv;
+
+ if (!child_panic) {
+ VCLI_SetResult(cli, CLIS_CANT);
+ VCLI_Out(cli,
+ "Child has not panicked or panic has been cleared");
+ return;
+ }
+
+ VCLI_Out(cli, "%s\n", VSB_data(child_panic));
+}
+
+void __match_proto__(cli_func_t)
+mcf_panic_clear(struct cli *cli, const char * const *av, void *priv)
+{
+ (void)av;
+ (void)priv;
+
+ if (child_panic == NULL) {
+ VCLI_SetResult(cli, CLIS_CANT);
+ VCLI_Out(cli, "No panic to clear");
+ return;
+ }
+ mgt_panic_clear();
+}
+
+/*=====================================================================
* Track the highest file descriptor the parent knows is being used.
*
* This allows the child process to clean/close only a small fraction
@@ -124,19 +198,6 @@ mgt_got_fd(int fd)
}
/*--------------------------------------------------------------------
- * A handy little function
- */
-
-static inline void
-closex(int *fd)
-{
-
- assert(*fd >= 0);
- AZ(close(*fd));
- *fd = -1;
-}
-
-/*--------------------------------------------------------------------
* Keep track of which filedescriptors the child should inherit and
* which should be closed after fork()
*/
@@ -155,80 +216,12 @@ mgt_child_inherit(int fd, const char *what)
vbit_clr(fd_map, fd);
}
-/*--------------------------------------------------------------------*/
-
-static int
-child_line(void *priv, const char *p)
-{
- (void)priv;
-
- REPORT(LOG_NOTICE, "Child (%jd) said %s", (intmax_t)child_pid, p);
- return (0);
-}
-
-/*--------------------------------------------------------------------*/
-
-static int
-child_listener(const struct vev *e, int what)
-{
-
- (void)e;
- if ((what & ~EV_RD)) {
- ev_listen = NULL;
- return (1);
- }
- if (VLU_Fd(child_output, vlu)) {
- ev_listen = NULL;
- return (1);
- }
- return (0);
-}
-
-/*--------------------------------------------------------------------*/
-
-static int
-child_poker(const struct vev *e, int what)
-{
-
- (void)e;
- (void)what;
- if (child_state != CH_RUNNING)
- return (1);
- if (child_pid < 0)
- return (0);
- if (!mgt_cli_askchild(NULL, NULL, "ping\n"))
- return (0);
- return (0);
-}
-
-/*--------------------------------------------------------------------
- * If CLI communications with the child process fails, there is nothing
- * for us to do but to drag it behind the barn and get it over with.
+/*=====================================================================
+ * Open and close the accept sockets.
*
- * The typical case is where the child process fails to return a reply
- * before the cli_timeout expires. This invalidates the CLI pipes for
- * all future use, as we don't know if the child was just slow and the
- * result gets piped later on, or if the child is catatonic.
+ * (The child is priv-sep'ed, so it can't do it.)
*/
-void
-MGT_Child_Cli_Fail(void)
-{
-
- if (child_state != CH_RUNNING)
- return;
- if (child_pid < 0)
- return;
- REPORT(LOG_ERR, "Child (%jd) not responding to CLI, killing it.",
- (intmax_t)child_pid);
- if (MGT_FEATURE(FEATURE_NO_COREDUMP))
- (void)kill(child_pid, SIGKILL);
- else
- (void)kill(child_pid, SIGQUIT);
-}
-
-/*--------------------------------------------------------------------*/
-
static int
open_sockets(void)
{
@@ -268,10 +261,61 @@ close_sockets(void)
}
}
-/*--------------------------------------------------------------------*/
+/*=====================================================================
+ * Listen to stdout+stderr from the child
+ */
+
+static int
+child_line(void *priv, const char *p)
+{
+ (void)priv;
+
+ REPORT(LOG_NOTICE, "Child (%jd) said %s", (intmax_t)child_pid, p);
+ return (0);
+}
+
+/*--------------------------------------------------------------------
+ * NB: Notice cleanup call from mgt_reap_child()
+ */
+
+static int __match_proto__(vev_cb_f)
+child_listener(const struct vev *e, int what)
+{
+
+ if ((what & ~EV_RD) || VLU_Fd(child_output, child_std_vlu)) {
+ ev_listen = NULL;
+ if (e != NULL)
+ mgt_reap_child();
+ return (1);
+ }
+ return (0);
+}
+
+/*=====================================================================
+ * Periodically poke the child, to see that it still lives
+ */
+
+static int __match_proto__(vev_cb_f)
+child_poker(const struct vev *e, int what)
+{
+
+ (void)e;
+ (void)what;
+ if (child_state != CH_RUNNING)
+ return (1);
+ if (child_pid < 0)
+ return (0);
+ if (!mgt_cli_askchild(NULL, NULL, "ping\n"))
+ return (0);
+ return (0);
+}
+
+/*=====================================================================
+ * Launch the child process
+ */
static void
-start_child(struct cli *cli)
+mgt_launch_child(struct cli *cli)
{
pid_t pid;
unsigned u;
@@ -300,7 +344,7 @@ start_child(struct cli *cli)
AZ(pipe(cp));
heritage.cli_in = cp[0];
mgt_child_inherit(heritage.cli_in, "cli_in");
- child_VCLI_Out = cp[1];
+ child_cli_out = cp[1];
/* Open pipe for child->mgr CLI */
AZ(pipe(cp));
@@ -321,6 +365,7 @@ start_child(struct cli *cli)
AN(heritage.vsm);
AN(heritage.param);
if ((pid = fork()) < 0) {
+ /* XXX */
perror("Could not fork child");
exit(1);
}
@@ -334,6 +379,7 @@ start_child(struct cli *cli)
/* Close anything we shouldn't know about */
closelog();
+
for (i = STDERR_FILENO + 1; i < CLOSE_FD_UP_TO; i++) {
if (vbit_test(fd_map, i))
continue;
@@ -352,6 +398,7 @@ start_child(struct cli *cli)
exit(1);
}
+ assert(pid > 1);
REPORT(LOG_NOTICE, "child (%jd) Started", (intmax_t)pid);
/* Close stuff the child got */
@@ -365,8 +412,8 @@ start_child(struct cli *cli)
close_sockets();
- vlu = VLU_New(NULL, child_line, 0);
- AN(vlu);
+ child_std_vlu = VLU_New(NULL, child_line, 0);
+ AN(child_std_vlu);
AZ(ev_listen);
e = vev_new();
@@ -388,7 +435,7 @@ start_child(struct cli *cli)
ev_poker = e;
}
- mgt_cli_start_child(child_cli_in, child_VCLI_Out);
+ mgt_cli_start_child(child_cli_in, child_cli_out);
child_pid = pid;
if (mgt_push_vcls_and_start(&u, &p)) {
REPORT(LOG_ERR, "Pushing vcls failed:\n%s", p);
@@ -399,81 +446,64 @@ start_child(struct cli *cli)
child_state = CH_RUNNING;
}
-/*--------------------------------------------------------------------*/
-
-void
-mgt_stop_child(void)
-{
-
- if (child_state != CH_RUNNING)
- return;
-
- child_state = CH_STOPPING;
-
- REPORT0(LOG_DEBUG, "Stopping Child");
- if (ev_poker != NULL) {
- vev_del(mgt_evb, ev_poker);
- free(ev_poker);
- }
- ev_poker = NULL;
-
- mgt_cli_stop_child();
-
- /* We tell the child to die gracefully by closing the CLI */
- closex(&child_VCLI_Out);
- closex(&child_cli_in);
-}
-
-/*--------------------------------------------------------------------*/
-
-static void
-mgt_handle_panicstr(pid_t r)
-{
- char time_str[30];
-
- AN(heritage.panic_str[0]);
- REPORT(LOG_ERR, "Child (%jd) Panic message: %s",
- (intmax_t)r, heritage.panic_str);
-
- if (child_panic)
- VSB_delete(child_panic);
- child_panic = VSB_new_auto();
- XXXAN(child_panic);
- VTIM_format(VTIM_real(), time_str);
- VSB_printf(child_panic, "Last panic at: %s\n", time_str);
- VSB_cat(child_panic, heritage.panic_str);
- AZ(VSB_finish(child_panic));
-}
+/*=====================================================================
+ * Cleanup when child dies.
+ */
static void
-mgt_clear_panic(void)
-{
- VSB_delete(child_panic);
- child_panic = NULL;
-}
-
-/*--------------------------------------------------------------------*/
-
-static int
-mgt_sigchld(const struct vev *e, int what)
+mgt_reap_child(void)
{
+ int i;
int status;
struct vsb *vsb;
pid_t r;
- (void)e;
- (void)what;
+ assert(child_pid != -1);
+
+ /*
+ * Close the CLI connections
+ * This signals orderly shut down to child
+ */
+ mgt_cli_stop_child();
+ if (child_cli_out >= 0)
+ closex(&child_cli_out);
+ if (child_cli_in >= 0)
+ closex(&child_cli_in);
+ /* Stop the poker */
if (ev_poker != NULL) {
vev_del(mgt_evb, ev_poker);
free(ev_poker);
}
ev_poker = NULL;
- r = waitpid(child_pid, &status, WNOHANG);
- if (r == 0 || (r == -1 && errno == ECHILD))
- return (0);
+ /* Stop the listener */
+ if (ev_listen != NULL) {
+ vev_del(mgt_evb, ev_listen);
+ free(ev_listen);
+ ev_listen = NULL;
+ }
+
+ /* Wait 10 seconds for child to die */
+ for (i = 0; i < 10; i++) {
+ r = waitpid(child_pid, &status, WNOHANG);
+ if (r == child_pid)
+ break;
+ (void)sleep(1);
+ }
+ if (r == 0) {
+ /* Kick it Jim... */
+ if (MGT_FEATURE(FEATURE_NO_COREDUMP))
+ (void)kill(child_pid, SIGKILL);
+ else
+ (void)kill(child_pid, SIGQUIT);
+ r = waitpid(child_pid, &status, 0);
+ }
+ if (r != child_pid)
+ fprintf(stderr, "WAIT 0x%x\n", r);
assert(r == child_pid);
+
+ /* Compose obituary */
vsb = VSB_new_auto();
XXXAN(vsb);
VSB_printf(vsb, "Child (%ld) %s", (long)r, status ? "died" : "ended");
@@ -495,47 +525,118 @@ mgt_sigchld(const struct vev *e, int what)
REPORT(LOG_INFO, "%s", VSB_data(vsb));
VSB_delete(vsb);
+ /* Dispose of shared memory but evacuate panic messages first */
if (heritage.panic_str[0] != '\0') {
- mgt_handle_panicstr(r);
+ mgt_panic_record(r);
mgt_SHM_Destroy(1);
} else {
mgt_SHM_Destroy(0);
}
mgt_SHM_Create();
- child_pid = -1;
-
- if (child_state == CH_RUNNING) {
+ if (child_state == CH_RUNNING)
child_state = CH_DIED;
- mgt_cli_stop_child();
- closex(&child_VCLI_Out);
- closex(&child_cli_in);
- }
- if (ev_listen != NULL) {
- vev_del(mgt_evb, ev_listen);
- free(ev_listen);
- ev_listen = NULL;
- }
/* Pick up any stuff lingering on stdout/stderr */
(void)child_listener(NULL, EV_RD);
closex(&child_output);
+ VLU_Destroy(child_std_vlu);
+
+ child_pid = -1;
REPORT0(LOG_DEBUG, "Child cleanup complete");
if (child_state == CH_DIED && mgt_param.auto_restart)
- start_child(NULL);
- else if (child_state == CH_DIED) {
+ mgt_launch_child(NULL);
+ else if (child_state == CH_DIED)
child_state = CH_STOPPED;
- } else if (child_state == CH_STOPPING)
+ else if (child_state == CH_STOPPING)
child_state = CH_STOPPED;
+}
- return (0);
+/*=====================================================================
+ * If CLI communications with the child process fails, there is nothing
+ * for us to do but to drag it behind the barn and get it over with.
+ *
+ * The typical case is where the child process fails to return a reply
+ * before the cli_timeout expires. This invalidates the CLI pipes for
+ * all future use, as we don't know if the child was just slow and the
+ * result gets piped later on, or if the child is catatonic.
+ */
+
+void
+MGT_Child_Cli_Fail(void)
+{
+
+ if (child_state != CH_RUNNING)
+ return;
+ if (child_pid < 0)
+ return;
+ REPORT(LOG_ERR, "Child (%jd) not responding to CLI, killing it.",
+ (intmax_t)child_pid);
+ if (MGT_FEATURE(FEATURE_NO_COREDUMP))
+ (void)kill(child_pid, SIGKILL);
+ else
+ (void)kill(child_pid, SIGQUIT);
+}
+
+/*=====================================================================
+ * Controlled stop of child process
+ *
+ * Reaping the child asks for orderly shutdown
+ */
+
+void
+mgt_stop_child(void)
+{
+
+ if (child_state != CH_RUNNING)
+ return;
+
+ child_state = CH_STOPPING;
+
+ REPORT0(LOG_DEBUG, "Stopping Child");
+
+ mgt_reap_child();
+}
+
+/*=====================================================================
+ * CLI command to start/stop child
+ */
+
+void __match_proto__(cli_func_t)
+mcf_server_startstop(struct cli *cli, const char * const *av, void *priv)
+{
+
+ (void)av;
+ if (priv != NULL && child_state == CH_RUNNING)
+ mgt_stop_child();
+ else if (priv == NULL && child_state == CH_STOPPED) {
+ if (mgt_has_vcl()) {
+ mgt_launch_child(cli);
+ } else {
+ VCLI_SetResult(cli, CLIS_CANT);
+ VCLI_Out(cli, "No VCL available");
+ }
+ } else {
+ VCLI_SetResult(cli, CLIS_CANT);
+ VCLI_Out(cli, "Child in state %s", ch_state[child_state]);
+ }
}
/*--------------------------------------------------------------------*/
-static int
+void
+mcf_server_status(struct cli *cli, const char * const *av, void *priv)
+{
+ (void)av;
+ (void)priv;
+ VCLI_Out(cli, "Child in state %s", ch_state[child_state]);
+}
+
+/*--------------------------------------------------------------------*/
+
+static int __match_proto__(vev_cb_f)
mgt_sigint(const struct vev *e, int what)
{
@@ -548,7 +649,7 @@ mgt_sigint(const struct vev *e, int what)
exit (2);
}
-/*--------------------------------------------------------------------
+/*=====================================================================
* This thread is the master thread in the management process.
* The relatively simple task is to start and stop the child process
* and to reincarnate it in case of trouble.
@@ -575,14 +676,6 @@ MGT_Run(void)
e->name = "mgt_sigint";
AZ(vev_add(mgt_evb, e));
- e = vev_new();
- XXXAN(e);
- e->sig = SIGCHLD;
- e->sig_flags = SA_NOCLDSTOP;
- e->callback = mgt_sigchld;
- e->name = "mgt_sigchild";
- AZ(vev_add(mgt_evb, e));
-
#ifdef HAVE_SETPROCTITLE
setproctitle("Varnish-Mgr %s", heritage.name);
#endif
@@ -597,7 +690,7 @@ MGT_Run(void)
if (!d_flag && !mgt_has_vcl())
REPORT0(LOG_ERR, "No VCL loaded yet");
else if (!d_flag) {
- start_child(NULL);
+ mgt_launch_child(NULL);
if (child_state == CH_STOPPED) {
exit_status = 2;
return;
@@ -610,65 +703,3 @@ MGT_Run(void)
REPORT0(LOG_ERR, "manager dies");
}
-
-/*--------------------------------------------------------------------*/
-
-void __match_proto__(cli_func_t)
-mcf_server_startstop(struct cli *cli, const char * const *av, void *priv)
-{
-
- (void)av;
- if (priv != NULL && child_state == CH_RUNNING)
- mgt_stop_child();
- else if (priv == NULL && child_state == CH_STOPPED) {
- if (mgt_has_vcl()) {
- start_child(cli);
- } else {
- VCLI_SetResult(cli, CLIS_CANT);
- VCLI_Out(cli, "No VCL available");
- }
- } else {
- VCLI_SetResult(cli, CLIS_CANT);
- VCLI_Out(cli, "Child in state %s", ch_state[child_state]);
- }
-}
-
-/*--------------------------------------------------------------------*/
-
-void
-mcf_server_status(struct cli *cli, const char * const *av, void *priv)
-{
- (void)av;
- (void)priv;
- VCLI_Out(cli, "Child in state %s", ch_state[child_state]);
-}
-
-void
-mcf_panic_show(struct cli *cli, const char * const *av, void *priv)
-{
- (void)av;
- (void)priv;
-
- if (!child_panic) {
- VCLI_SetResult(cli, CLIS_CANT);
- VCLI_Out(cli, "Child has not panicked or panic has been cleared");
- return;
- }
-
- VCLI_Out(cli, "%s\n", VSB_data(child_panic));
-}
-
-void
-mcf_panic_clear(struct cli *cli, const char * const *av, void *priv)
-{
- (void)av;
- (void)priv;
-
- if (!child_panic) {
- VCLI_SetResult(cli, CLIS_CANT);
- VCLI_Out(cli, "No panic to clear");
- return;
- }
-
- mgt_clear_panic();
-}
More information about the varnish-commit
mailing list