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

[Andrew Dougherty]

On Tue, 28 Aug 2012, Andy Dougherty wrote:

> On Tue, 28 Aug 2012, Andrew Whitworth wrote:
> 
> > Yes, that patch actually does look very reasonable. Does it change the
> > test results you've seen, at all?
> 

For these three systems:
    NetBSD 51/i386
    OpenBSD 5.1/amd64
    Solaris 11/i86pc
the patch either helps or does no obvious harm.  All the tests pass.

For Solaris 8/SPARC-32 the patch makes no difference. Parrot still hangs
in t/pmc/alarm.t.  All the alarms are set, but then it just loops forever
through Parrot_cx_check_scheduler.  Parrot_alarm_check keeps returning 0.
It looks as if the thread that is supposed to run Parrot_alarm_runloop()
never actually gets a chance to run.  Unfortunately, inserting any
debugging information apparently changes the operating system's scheduling
and makes the test run just fine.  This may well be a bug in Solaris
8's thread scheduling algorithm.  I would not consider this a blocker
to merge.

I would recommend applying the patch.  (Quoted below again for reference.)

> 
> > On Tue, Aug 28, 2012 at 10:34 AM, Andy Dougherty <doughera at lafayette.edu> wrote:
> > > I've never really done any threads programming, so I could be quite off
> > > here, but it looks to me as if there's a race condition in src/alarm.c in
> > > the threads branch.  Specifically, Parrot_alarm_init() creates a thread
> > > that checks sleep_cond before it initializes sleep_cond.  (Similar remarks
> > > hold for alarm_lock.)
> > >
> > > Does this patch look appropriate?
> > >
> > > diff --git a/src/alarm.c b/src/alarm.c
> > > index 298387f..0ec6a1f 100644
> > > --- a/src/alarm.c
> > > +++ b/src/alarm.c
> > > @@ -56,9 +56,9 @@ Parrot_alarm_init(void)
> > >  {
> > >      ASSERT_ARGS(Parrot_alarm_init)
> > >      Parrot_thread thread;
> > > -    THREAD_CREATE_JOINABLE(thread, Parrot_alarm_runloop, NULL);
> > >      MUTEX_INIT(alarm_lock);
> > >      COND_INIT(sleep_cond);
> > > +    THREAD_CREATE_JOINABLE(thread, Parrot_alarm_runloop, NULL);
> > >  }
> > >
> > >  /*
> > >

-- 
    Andy Dougherty		doughera at lafayette.edu