Hi Oystein,
The patch looks ok. I have one suggestion for change below. Let me know whether
you agree with it.
Oystein Grovlen wrote:
> #At file:///export/home/tmp/oysteing/mysql/mysql-6.0-backup/
>
> 2693 Oystein Grovlen 2008-09-05
> Bug#39152 Intermittent debug_sync time-outs in backup tests
> Timeouts have occurred due to race conditions in debug_sync
> Make sure all accesses to debug_sync_global.ds_signal is protected
> by mutex.
> modified:
> sql/debug_sync.cc
>
> per-file messages:
> sql/debug_sync.cc
> Make sure all accesses to debug_sync_global.ds_signal is protected
> by mutex.
> @@ -1582,6 +1589,13 @@
> thd_proc_info(thd, proc_info.c_ptr());
> }
>
> + /*
> + Take mutex to ensure that only one thread access
> + debug_sync_global.ds_signal at a time. Need to take mutex for
> + read access too, to create a memory barrier in order to avoid that
> + threads just reads an old cached version of the signal.
> + */
> + pthread_mutex_lock(&debug_sync_global.ds_mutex);
> if (action->signal.length())
> {
> /* Copy the signal to the global variable. */
> @@ -1606,7 +1620,6 @@
> int error= 0;
> struct timespec abstime;
>
> - pthread_mutex_lock(&debug_sync_global.ds_mutex);
> /*
> We don't use enter_cond()/exit_cond(). They do not save old
> mutex and cond. This would prohibit the use of DEBUG_SYNC
> @@ -1670,6 +1683,11 @@
> pthread_mutex_unlock(&thd->mysys_var->mutex);
>
> } /* end if (action->wait_for.length()) */
> + else
> + {
> + // Make sure to realease mutex also when there is no WAIT_FOR
> + pthread_mutex_unlock(&debug_sync_global.ds_mutex);
> + }
Since the mutex was taken otside the if statement, I think it is better to also
release it outside. I.e., the code should look like this:
} /* end if (action->wait_for.length()) */
+
+ pthread_mutex_unlock(&debug_sync_global.ds_mutex);
This change would mean that thd->mysys_var->mutex is taken while
debug_sync_global.ds_mutex is being held. But I think it is ok as we don't wait
for anything while holding thd->mysys_var->mutex. The synchronization sequence
will be:
- take debug_sync_global.ds_mutex
- take thd->mysys_var->mutex
- release thd->mysys_var->mutex
- release debug_sync_global.ds_mutex
Which I think is safe.
Rafal
