On Wed, Apr 15, 2020 at 09:50:01AM -0400, Jim Jagielski wrote:
> very, very elegant.
Thank you Jim!
I wonder if the new state variable is redundant with the other state
variables in the watchdog structure? I don't understand the watchdog
model well enough to be confident here.
struct ap_watchdog_t
{
apr_uint32_t thread_started; /* set to 1 once thread running */
...
int singleton;
int active;
Also reviewing this module more, in the post_config hook:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/core/mod_watchdog.c?annotate=1876619#l434
it looks to me like the ap_state_query() call correctly guards against
multiple post_config runs during start, so there will never be a point
where the wd_server_conf attached to pconf is reused or reusable.
- in initial startup you get only one post_config invocation if the
_PRE_CONFIG phase is skipped
- during subsequent server reloads pconf is cleared each time
I may be missing something.
Regards, Joe
>
> > On Apr 14, 2020, at 8:37 AM, jor...@apache.org wrote:
> >
> > Author: jorton
> > Date: Tue Apr 14 12:37:17 2020
> > New Revision: 1876511
> >
> > URL: http://svn.apache.org/viewvc?rev=1876511&view=rev
> > Log:
> > * modules/core/mod_watchdog.c: Switch to simpler logic to avoid the
> > thread cleanup running before the thread has started, avoiding
> > mutex operations which both have undefined behaviour:
> >
> > a) double-locking an UNNESTED (non-recursive) mutex twice in the parent
> > b) unlocking a mutex in the spawned thread which was locked by the parent
> >
> > (wd_startup, wd_worker_cleanup, wd_worker): Use a boolean to ensure
> > the cleanup does nothing if the thread wasn't started, drop the mutex.
> >
> > Modified:
> > httpd/httpd/trunk/modules/core/mod_watchdog.c
> >
> > Modified: httpd/httpd/trunk/modules/core/mod_watchdog.c
> > URL:
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/core/mod_watchdog.c?rev=1876511&r1=1876510&r2=1876511&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/modules/core/mod_watchdog.c (original)
> > +++ httpd/httpd/trunk/modules/core/mod_watchdog.c Tue Apr 14 12:37:17 2020
> > @@ -24,6 +24,8 @@
> > #include "http_core.h"
> > #include "util_mutex.h"
> >
> > +#include "apr_atomic.h"
> > +
> > #define AP_WATCHDOG_PGROUP "watchdog"
> > #define AP_WATCHDOG_PVERSION "parent"
> > #define AP_WATCHDOG_CVERSION "child"
> > @@ -43,7 +45,7 @@ struct watchdog_list_t
> >
> > struct ap_watchdog_t
> > {
> > - apr_thread_mutex_t *startup;
> > + apr_uint32_t thread_started; /* set to 1 once thread running
> > */
> > apr_proc_mutex_t *mutex;
> > const char *name;
> > watchdog_list_t *callbacks;
> > @@ -74,6 +76,10 @@ static apr_status_t wd_worker_cleanup(vo
> > apr_status_t rv;
> > ap_watchdog_t *w = (ap_watchdog_t *)data;
> >
> > + /* Do nothing if the thread wasn't started. */
> > + if (apr_atomic_read32(&w->thread_started) != 1)
> > + return APR_SUCCESS;
> > +
> > if (w->is_running) {
> > watchdog_list_t *wl = w->callbacks;
> > while (wl) {
> > @@ -110,7 +116,8 @@ static void* APR_THREAD_FUNC wd_worker(a
> > w->pool = apr_thread_pool_get(thread);
> > w->is_running = 1;
> >
> > - apr_thread_mutex_unlock(w->startup);
> > + apr_atomic_set32(&w->thread_started, 1); /* thread started */
> > +
> > if (w->mutex) {
> > while (w->is_running) {
> > if (ap_mpm_query(AP_MPMQ_MPM_STATE, &mpmq_s) != APR_SUCCESS) {
> > @@ -264,10 +271,7 @@ static apr_status_t wd_startup(ap_watchd
> > {
> > apr_status_t rc;
> >
> > - /* Create thread startup mutex */
> > - rc = apr_thread_mutex_create(&w->startup, APR_THREAD_MUTEX_UNNESTED,
> > p);
> > - if (rc != APR_SUCCESS)
> > - return rc;
> > + apr_atomic_set32(&w->thread_started, 0);
> >
> > if (w->singleton) {
> > /* Initialize singleton mutex in child */
> > @@ -277,22 +281,12 @@ static apr_status_t wd_startup(ap_watchd
> > return rc;
> > }
> >
> > - /* This mutex fixes problems with a fast start/fast end, where the pool
> > - * cleanup was being invoked before the thread completely spawned.
> > - */
> > - apr_thread_mutex_lock(w->startup);
> > - apr_pool_pre_cleanup_register(p, w, wd_worker_cleanup);
> > -
> > /* Start the newly created watchdog */
> > rc = apr_thread_create(&w->thread, NULL, wd_worker, w, p);
> > - if (rc) {
> > - apr_pool_cleanup_kill(p, w, wd_worker_cleanup);
> > + if (rc == APR_SUCCESS) {
> > + apr_pool_pre_cleanup_register(p, w, wd_worker_cleanup);
> > }
> >
> > - apr_thread_mutex_lock(w->startup);
> > - apr_thread_mutex_unlock(w->startup);
> > - apr_thread_mutex_destroy(w->startup);
> > -
> > return rc;
> > }
> >
> >
> >
>
