On Thu, Sep 26, 2019 at 8:20 AM Pluem, Ruediger, Vodafone Group
<[email protected]> wrote:
>
> > -----Ursprüngliche Nachricht-----
> > Von: Yann Ylavic <[email protected]>
> >
> > Likewise, I think the MPMs themselves shouldn't use pchild for their
> > internal allocations possibly still in use at exit().
> > So v2 (attached) may be the thing..
>
> Hm, haven't checked, but aren't there any cleanups that should run and
> currently run before exit that will not run any longer when we tie
> stuff to pconf instead of pchild?
> I guess pure allocations are not a problem, since the process dies,
> but I would be a little worried about other OS resources like
> shared memory or locks not being cleaned up properly.
I think you are right, proc mutexes at least need to cleanup properly
on child exit.
I updated the patch (attached) to keep them on pchild.
> Regarding the watchdog threads I guess we could handle this
> like Stefan suggested by handling it similar to still running connections.
> Give them a grace period and kill them afterwards during regular shutdown.
> For an immediate shutdown kill them off directly.
Killing threads is going to be hard to achieve, all the more so in a
portable way. There is no apr_thread_kill() for instance,
pthread_kill() is not suitable, I know of tgkill() on linux...
But we shouldn't take that road IMHO, and regarding the state of
shared/proc resources potentially used by these threads it looks like
a can of worms..
Asking for watchdog callbacks (including third-parties') to
[un]gracefully stop is not something in the current "contract"
unfortunately, we are quite weaponless here I'm afraid.
So I can only think of _exit() like in attached v3, although in
addition to not run atexit() handlers _exit() also potentially does
not flush stdios, but all fds are closed so pending outputs should
still finish (for whatever that means in linux/BSD docs..).
This is still going to be racy with anything initialized on pchild
though, like mod_ssl caches mutexes (session, stapling) :/
Regards,
Yann.
Index: modules/core/mod_watchdog.c
===================================================================
--- modules/core/mod_watchdog.c (revision 1866998)
+++ modules/core/mod_watchdog.c (working copy)
@@ -567,8 +567,8 @@ static void wd_child_init_hook(apr_pool_t *p, serv
"Watchdog: nothing configured?");
return;
}
- if ((wl = ap_list_provider_names(p, AP_WATCHDOG_PGROUP,
- AP_WATCHDOG_CVERSION))) {
+ if ((wl = ap_list_provider_names(wd_server_conf->pool, AP_WATCHDOG_PGROUP,
+ AP_WATCHDOG_CVERSION))) {
const ap_list_provider_names_t *wn;
int i;
wn = (ap_list_provider_names_t *)wl->elts;
Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c (revision 1866998)
+++ server/mpm/event/event.c (working copy)
@@ -772,7 +772,7 @@ static void clean_child_exit(int code)
event_note_child_killed(/* slot */ 0, 0, 0);
}
- exit(code);
+ _exit(code);
}
static void just_die(int sig)
@@ -2799,9 +2799,9 @@ static void child_main(int child_num_arg, int chil
* and we want a 0 entry to indicate a thread which was not created
*/
threads = ap_calloc(threads_per_child, sizeof(apr_thread_t *));
- ts = apr_palloc(pchild, sizeof(*ts));
+ ts = apr_palloc(pconf, sizeof(*ts));
- apr_threadattr_create(&thread_attr, pchild);
+ apr_threadattr_create(&thread_attr, pconf);
/* 0 means PTHREAD_CREATE_JOINABLE */
apr_threadattr_detach_set(thread_attr, 0);
@@ -2821,7 +2821,7 @@ static void child_main(int child_num_arg, int chil
ts->threadattr = thread_attr;
rv = apr_thread_create(&start_thread_id, thread_attr, start_threads,
- ts, pchild);
+ ts, pconf);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf, APLOGNO(00480)
"apr_thread_create: unable to create worker thread");
Index: server/mpm/motorz/motorz.c
===================================================================
--- server/mpm/motorz/motorz.c (revision 1866998)
+++ server/mpm/motorz/motorz.c (working copy)
@@ -598,7 +598,7 @@ static void clean_child_exit(int code)
}
ap_mpm_pod_close(my_bucket->pod);
- exit(code);
+ _exit(code);
}
#if 0 /* unused for now */
@@ -812,7 +812,7 @@ static void child_main(motorz_core_t *mz, int chil
ap_run_child_init(pchild, ap_server_conf);
- ap_create_sb_handle(&sbh, pchild, my_child_num, 0);
+ ap_create_sb_handle(&sbh, mz->pool, my_child_num, 0);
ap_update_child_status(sbh, SERVER_READY, NULL);
Index: server/mpm/prefork/prefork.c
===================================================================
--- server/mpm/prefork/prefork.c (revision 1866998)
+++ server/mpm/prefork/prefork.c (working copy)
@@ -234,7 +234,7 @@ static void clean_child_exit(int code)
ap_mpm_pod_close(my_bucket->pod);
chdir_for_gprof();
- exit(code);
+ _exit(code);
}
static apr_status_t accept_mutex_on(void)
@@ -403,6 +403,11 @@ static void child_main(int child_num_arg, int chil
ap_fatal_signal_child_setup(ap_server_conf);
+#if APR_HAS_THREADS
+ osthd = apr_os_thread_current();
+ apr_os_thread_put(&thd, &osthd, pconf);
+#endif
+
/* Get a sub context for global allocations in this child, so that
* we can have cleanups occur when the child exits.
*/
@@ -412,11 +417,6 @@ static void child_main(int child_num_arg, int chil
apr_allocator_owner_set(allocator, pchild);
apr_pool_tag(pchild, "pchild");
-#if APR_HAS_THREADS
- osthd = apr_os_thread_current();
- apr_os_thread_put(&thd, &osthd, pchild);
-#endif
-
apr_pool_create(&ptrans, pchild);
apr_pool_tag(ptrans, "transaction");
@@ -449,12 +449,12 @@ static void child_main(int child_num_arg, int chil
ap_run_child_init(pchild, ap_server_conf);
- ap_create_sb_handle(&sbh, pchild, my_child_num, 0);
+ ap_create_sb_handle(&sbh, pconf, my_child_num, 0);
(void) ap_update_child_status(sbh, SERVER_READY, (request_rec *) NULL);
/* Set up the pollfd array */
- status = apr_pollset_create(&pollset, num_listensocks, pchild,
+ status = apr_pollset_create(&pollset, num_listensocks, pconf,
APR_POLLSET_NOCOPY);
if (status != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_EMERG, status, ap_server_conf, APLOGNO(00156)
@@ -463,7 +463,7 @@ static void child_main(int child_num_arg, int chil
}
for (lr = my_bucket->listeners, i = num_listensocks; i--; lr = lr->next) {
- apr_pollfd_t *pfd = apr_pcalloc(pchild, sizeof *pfd);
+ apr_pollfd_t *pfd = apr_pcalloc(pconf, sizeof *pfd);
pfd->desc_type = APR_POLL_SOCKET;
pfd->desc.s = lr->sd;
@@ -489,7 +489,7 @@ static void child_main(int child_num_arg, int chil
retained->mpm->mpm_state = AP_MPMQ_RUNNING;
- bucket_alloc = apr_bucket_alloc_create(pchild);
+ bucket_alloc = apr_bucket_alloc_create(pconf);
/* die_now is set when AP_SIG_GRACEFUL is received in the child;
* {shutdown,restart}_pending are set when a signal is received while
Index: server/mpm/simple/simple_children.c
===================================================================
--- server/mpm/simple/simple_children.c (revision 1866998)
+++ server/mpm/simple/simple_children.c (working copy)
@@ -29,6 +29,8 @@
APLOG_USE_MODULE(mpm_simple);
+static apr_pool_t *pchild = NULL;
+
static void simple_kill_random_child(simple_core_t * sc)
{
/* See comment in simple_spawn_child for why we check here. */
@@ -54,8 +56,11 @@ static void simple_kill_random_child(simple_core_t
static void clean_child_exit(int code) __attribute__ ((noreturn));
static void clean_child_exit(int code)
{
- /* TODO: Pool cleanups.... sigh. */
- exit(code);
+ if (pchild) {
+ apr_pool_destroy(pchild);
+ }
+
+ _exit(code);
}
static int simple_spawn_child(simple_core_t * sc)
@@ -81,8 +86,13 @@ static int simple_spawn_child(simple_core_t * sc)
if (pid == 0) {
/* this is the child process */
- rv = simple_child_loop(sc);
+ /* Get a sub context for global allocations in this child,
+ * so that we can have cleanups occur when the child exits.
+ */
+ apr_pool_create(&pchild, sc->pool);
+ apr_pool_tag(pchild, "pchild");
+ rv = simple_child_loop(sc, pchild);
if (rv) {
clean_child_exit(APEXIT_CHILDFATAL);
}
Index: server/mpm/simple/simple_run.c
===================================================================
--- server/mpm/simple/simple_run.c (revision 1866998)
+++ server/mpm/simple/simple_run.c (working copy)
@@ -276,7 +276,7 @@ static int simple_setup_pollcb(simple_core_t * sc)
return rv;
}
-int simple_child_loop(simple_core_t * sc)
+int simple_child_loop(simple_core_t * sc, apr_pool_t * pchild)
{
apr_status_t rv;
@@ -310,7 +310,7 @@ static int simple_setup_pollcb(simple_core_t * sc)
return !OK;
}
- ap_run_child_init(sc->pool, ap_server_conf);
+ ap_run_child_init(pchild, ap_server_conf);
return simple_run_loop(sc);
}
Index: server/mpm/simple/simple_run.h
===================================================================
--- server/mpm/simple/simple_run.h (revision 1866998)
+++ server/mpm/simple/simple_run.h (working copy)
@@ -21,7 +21,7 @@
void simple_single_process_hack(simple_core_t * sc);
-int simple_child_loop(simple_core_t * sc);
+int simple_child_loop(simple_core_t * sc, apr_pool_t * pchild);
int simple_main_loop(simple_core_t * sc);
Index: server/mpm/worker/worker.c
===================================================================
--- server/mpm/worker/worker.c (revision 1866998)
+++ server/mpm/worker/worker.c (working copy)
@@ -441,7 +441,7 @@ static void clean_child_exit(int code)
worker_note_child_killed(/* slot */ 0, 0, 0);
}
- exit(code);
+ _exit(code);
}
static void just_die(int sig)
@@ -1172,9 +1172,9 @@ static void child_main(int child_num_arg, int chil
*/
threads = (apr_thread_t **)ap_calloc(1,
sizeof(apr_thread_t *) * threads_per_child);
- ts = (thread_starter *)apr_palloc(pchild, sizeof(*ts));
+ ts = (thread_starter *)apr_palloc(pconf, sizeof(*ts));
- apr_threadattr_create(&thread_attr, pchild);
+ apr_threadattr_create(&thread_attr, pconf);
/* 0 means PTHREAD_CREATE_JOINABLE */
apr_threadattr_detach_set(thread_attr, 0);
@@ -1194,7 +1194,7 @@ static void child_main(int child_num_arg, int chil
ts->threadattr = thread_attr;
rv = apr_thread_create(&start_thread_id, thread_attr, start_threads,
- ts, pchild);
+ ts, pconf);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf, APLOGNO(00282)
"apr_thread_create: unable to create worker thread");
