[Vm-dev] Why isn't signalSemaphoreWithIndex() thread-safe?

[Igor Stasenko]

2009/9/21 Joshua Gargus <schwa at fastmail.us>:
>
> John M McIntosh wrote:
>>
>> Well there is no generic synchronizing code, so you are correct it has
>> to be platform specific.
>> When I wrote the original code a decade? back in the macintosh os-9
>> era there was no agreement
>> about doing platform specific implementations since at the time there
>> was a tendency to keep all
>> the code as generic as possible.
>>
>> So do you have a test case
>
> No test case.
>
>> or scenario where the code fails?
>
> Just the obvious race condition.  Let's just look at the first case of
> the if-statement; the case for the other buffer is symmetric.  Here's
> the code in interp.c
>
>    if (foo->semaphoresUseBufferA) {
>        if (foo->semaphoresToSignalCountA < SemaphoresToSignalSize) {
>            foo->semaphoresToSignalCountA += 1;
>            foo->semaphoresToSignalA[foo->semaphoresToSignalCountA] = index;
>        }
>    }
>
>
> Let's say that there are two native threads A and B that want to signal
> semaphores with indices 7 and 8, and that there are no other semaphores
> to be signalled.  Let's say that thread A is running until just after
> "semaphoresToSignalCountA" is incremented, but is interrupted before it
> assigns the index.  Then thread B runs, increments
> "semaphoresToSignalCountA" again (so its value is now 2), and sets
> foo->semaphoresToSignalA[2] = 8.  Then A resumes and stomps this value
> by setting foo->semaphoresToSignalA[2] = 7.  Now the semaphore with
> index 8 will not be signalled, and just as bad, the index stored in
> oo->semaphoresToSignalA[1] is now garbage that will be treated as a
> semaphore-index to signal.
>
> Cheers,
> Josh
>
>
>
+1 , its not thread-safe.

In Hydra VM, i made thread-safe event queue, and made signaling going though it.

In win32 Squeak vm, the above situation (race condition you have
described) is not possible, because
there are additional mutex, than makes sure that only single thread
can be inside a call of signaling semaphore
(See synchronousSemaphoreSignal() function).
But it doesn't makes this code smells better, for sure :)

-- 
Best regards,
Igor Stasenko AKA sig.