Simple lock up with delay + semaphore - not fixed with 0006576

On Jan 4, 2008 2:58 PM, Georg Köster < [hidden email]> wrote:

> Delay timeoutSemaphore: s afterMSecs: 1.

Despite the comment in the source code, Delay
class>>timeoutSemaphore:afterMSecs: doesn't signal the given
semaphore; in fact, it never uses that parameter. That method should
be deprecated, if not simply removed. I've filed a bug report:

   http://bugs.squeak.org/view.php?id=6834

> I have this fix for my use-case: Avoid Delay>>timeoutSempaphore:afterMSecs:

> |s|
> s := Semaphore new.
> [(Delay forSeconds:1) wait. s signal.] fork.
> s wait.
> Transcript show: 'Now gets here - line will be printed'; cr.

> Resource intense, I know.

Why; because you used fork? Squeak processes don't work the way you
might think. I would recommend a solution along those exact lines.

Hope this helps!

--Tom Phoenix

Andreas Raab schrieb:

> Georg Köster wrote:
>> This shows the use case. Shorter even:
>> |s|
>> s := Semaphore new.
>> Delay timeoutSemaphore: s afterMSecs: 1.
>> s wait.
>> Transcript show: 'doesnt get here - not printed'; cr.
>>
>> I hope I'm doing something wrong here!
>
> You must be. This works fine in a Croquet SDK image.

Unfortunately it seems that a ChangeSet of mine addressing [1]
introduced this bug. I guess it got included in 3.9 without anyone else
looking at it. As I mention in [2] the Delay class side method should read:

timeoutSemaphore: aSemaphore afterMSecs: anInteger
  "...comment..."

   ^ (self new setDelay: anInteger forSemaphore: aSemaphore) schedule

I guess trying to refactor too much made me change the method. I'm sorry
that I did not double check it more carefully in the first place.

Alex

[1] http://bugs.squeak.org/view.php?id=854
[2] http://bugs.squeak.org/view.php?id=6834

Tom Phoenix wrote:

> Is that the source in your image for that method? Here's what I have:
>
>       waitTimeoutMSecs: anInteger
>               "Wait on this semaphore for up to the given number of milliseconds,
> then timeout. It is up to the sender to determine the difference
> between the expected event and a timeout."
>
>               | waitingProcess wakeupProcess |
>               waitingProcess _ Processor activeProcess.
>               wakeupProcess _
>                       [(Delay forMilliseconds: (anInteger max: 0)) wait.
>                       self resumeProcess: waitingProcess] fork.
>
>               self wait.
>               wakeupProcess terminate.

How odd. It seems that this version is only present in 3.9 - all other
images have a variant of the (much simpler) code:

waitTimeoutMSecs: anInteger
    "Wait on this semaphore for up to the given number of milliseconds,
then timeout. It is up to the sender to determine the difference between
the expected event and a timeout."

    | d |
    d := Delay timeoutSemaphore: self afterMSecs: (anInteger max: 0).

    [self wait] ensure:[d unschedule].

The 3.9 version is tagged "jf 12/2/2003" but there is no indication what
it is trying to address (if anything).

Cheers,
   - Andreas

Georg Köster wrote:
> Hey I prefer Tom's version! I shouldn't be racy if I read the
> resumeProcess and resume code correctly. The resumeProcess message has
> no effect on non-waiting processes!

To the contrary. This code *introduces* a race condition when
manipulating the Semaphore's list of processes without protecting it
against concurrent modifications. Worse, the code *cannot be protected*
against concurrent modification since the VM manipulates that list on
its own. It may actually explain why some people have reported issues on
3.9 that do not seem to appear on 3.8 or 3.10 variants.

>
>  >               | waitingProcess wakeupProcess |
>  >               waitingProcess _ Processor activeProcess.
>  >               wakeupProcess _
>  >                       [(Delay forMilliseconds: (anInteger max: 0)) wait.
>  >                       self resumeProcess: waitingProcess] fork.
>  >
>  >               self wait.
>     "preempting here and getting the resumeProcess message sent would
> have no effect - therefore no race!"

What are you talking about? With the original code there was no race
condition whatsoever. If you think there is a race condition somewhere,
please explain in detail where you think that race condition is.

>     "in comparison having a semaphore getting signaled here would cause
> an excess signal on the sem: bad"

First, the original code was guarded with a call to #unschedule which
would avoid a second signal if the "real event" occurs long enough
before the "timeout event". On the other hand, neither implementation
prevents a double-signal if the real event occurs within an epsilon of
the timeout event. And due to the asynchronous nature of some semaphore
signals it is impossible to have a light-weight implementation like the
one provided in Semaphore handle this correctly.  It's possible to
handle this but this would require to separate the semaphore and the
delay signal and use a critical section to decide which one came first
and how to deal with the other. That, on the other hand, seems overkill
for the 99% of the cases in which the simplistic implementation in
3.8/3.10 is all that's needed.

Cheers,
  - Andreas

Georg Köster wrote:
> The second implementation has clearly other troubles (I didn't see that
> it's just using a LinkedList that's apparently not thread safe), but I
> explained before that I believe at least the double signaling is dealt
> with.

It most definitely does not prevent double signaling! What makes you
think it does? It is easiest to see in the case where we first get a
timeout signal, and then -while being inside #resumeProcess:- the "real"
event. Obviously there is nothing in the implementation that prevents
the second signal from occurring.

> I would recommend at least adding this problem to the documentation of
> wait*, that if one of the timed waits is used the wait (without timeout)
> contract is violated in that it might return even if no user process
> sent a signal.

Well, duh. It is the very *definition* of waitTimeoutMSecs: that it may
return although the event hasn't occurred. That's what the method does.
The comment is explicit, too noting that  "It is up to the sender to

determine the difference between the expected event and a timeout".

Cheers,
  - Andreas