[PATCH] Possible threads race condition in src/alarm.c

[Reini Urban]

On Thu, Aug 30, 2012 at 1:00 AM, Andrew Whitworth <wknight8111 at gmail.com> wrote:
> On Wed, Aug 29, 2012 at 4:50 PM, Reini Urban <rurban at x-ray.at> wrote:
>> I verified that it fixes the sleep deadlock, but with
>> the deadlock fix there come 3 more races detected by tsan.
>> (See http://blogs.perl.org/users/rurban/2012/08/threadsanitizer.html)
>>
>> Before:
>> $ tsan --announce-threads ./parrot t/pmc/task.t
>> 1..6
>> ok 1 - initialized
>> ok 2 task1 ran
>> ok 3 task2 ran
>> ok 4 sub1 ran
>> .. Deadlock in sleep cond_wait.
>>
>> After:
>> ...
>
> Do those deadlocks cause any test failures? Are they serious concerns?
> I think we're getting pretty darn close to 100% here, I don't want to
> rush a merge until we're ready.

tsan found two remaining races, one might be be fixed after the merge,
one can be ignored IMHO.

case 1:
Parrot_Task_nci_send writes to the task mailbox (src/pmc/task.c:315),
while Parrot_Task_invoke (src/pmc/task.c:174) might read from it.
I started writing a mutex lock around the mailbox like this, but it
did not help.

diff --git a/src/pmc/task.pmc b/src/pmc/task.pmc
index f3e151c..73127a2 100644
--- a/src/pmc/task.pmc
+++ b/src/pmc/task.pmc
@@ -442,7 +442,9 @@ Send a message to this task.
     METHOD send(PMC *message) {
         Parrot_Task_attributes * const tdata = PARROT_TASK(SELF);
         if (PMC_IS_NULL(tdata->mailbox)) {
+            LOCK(tdata->mailbox_lock);
             tdata->mailbox = Parrot_pmc_new(interp, enum_class_PMCList);
+            UNLOCK(tdata->mailbox_lock);
             PARROT_GC_WRITE_BARRIER(interp, SELF);
         }

I haven't seen that yet in the wild, but it could happen.

case 2:
When creating new threads, threads_array will be filled with a new
non-null entry,
but handling pending signals in Parrot_thread_notify_threads might
miss the new thread entry
in threads_array.
I don't think that this scenario is worth fixing.
And it is also very hard to fix, since interp is not passed through
Parrot_thread_notify_threads. The interp arg should be changed to SHIM_INTERP
also.

> I'm away for the weekend. I suggest we don't attempt any merge until
> next week at the earliest, if we think we've gotten the issues ironed
> out by then. Come Tuesday or Wednesday if things are looking good I
> would definitely like to start pushing forward.
-- 
Reini Urban
http://cpanel.net/   http://www.perl-compiler.org/