very, very elegant.
> On Apr 14, 2020, at 8:37 AM, [email protected] 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;
> }
>
>
>
