WAMutexTest>>testSelfTerminate ... what behavior is being tested?

[dalehenrich]

This test as written:

testSelfTerminate
    | value semaphore |
    value := nil.
    semaphore := GRPlatform current semaphoreClass new.
    process := [ 
    value := mutex
        critical: [ 
            semaphore signal.
            mutex terminateOwner.
            1 ] ]
        newProcess.
    process resume.
    semaphore wait.
    self assert: mutex owner isNil.
    self assert: value isNil.
    self assert: (GRPlatform current isProcessTerminated: process)

presumably has been passing in Pharo and GemStone for a long time ... However, in 3.7.0 (upcoming release) we have done a fair amount of work to improve the process scheduling and termination model ... specifically targeting the handling of ensure blocks ... we weren't quite up to snuff for all cases.

We had to add a Delay after the semaphore wait in order for the "test to pass":

testSelfTerminate
    | value semaphore |
    value := nil.
    semaphore := GRPlatform current semaphoreClass new.
    process := [ 
    value := mutex
        critical: [ 
            semaphore signal.
            mutex terminateOwner.
            1 ] ]
        newProcess.
    process resume.
    semaphore wait.
"-->"   [ process isTerminated ] whileFalse: [ Delay waitForMilliseconds: 20 ].
    self assert: mutex owner isNil.
    self assert: value isNil.
    self assert: (GRPlatform current isProcessTerminated: process)

In our new and improved implementation, the process waiting on the semaphore is allowed to run as soon as the semaphore signal is sent ... and as a result the mutex owner isn't cleared by the time the assertion is made and this is an explicit change in behavior we made, because we think it is the correct semantics for a process to be allowed to start running as soon as the semaphore is signaled, rather than wait it's turn in the scheduler queue and wait for the running process to be terminated before allowing another process to run ...

From the test it isn't clear whether it is important:

1. that the mutex owner is cleared before any other process gets a chance to run, OR
2. that the mutex owner is (eventually) cleared upon process termination

If order of process execution is important then it will be necessary for us to not only change this test for GemStone, but to make changes in the other parts of the system where this behavior is important ...

As you know, there is probably not a whole lot of multi-process operations that should be done in a GemStone Seaside implementation, but I don't want to just change this test and cross our fingers :)

[dalehenrich]

There are actually 2 tests that are failing since our changes to GsProcess:

    #1 [224672513 size:37  String] WAMutexTest debug: #testSelfTerminate
    #2 [224672001 size:45  String] WAGemStoneMutexTest debug: #testSelfTerminate

[jbrichau]

Good question. Looking at what the owner instance variable is used for: is there another use for the owner instance variable than preventing deadlock? If there is none, I'm inclined to agree with option 2 (eventually cleared is fine).

I guess someone was lucky with where the process scheduler yields the individual processes such that it worked fine across all platforms to signal the semaphore before terminating the process :-)

I'm curious if the loop that waits for the process to terminate is going to work on all platforms... I guess any invocation of Delay waitFor: xxx will yield other processes on any platform?

[dalehenrich]

I guess any invocation of Delay waitFor: xxx will yield other processes on any platform?

I'm certain that it does yield, as blocking would be counterproductive ...

[martinmcclure]

The problem that I see in this test is that it attempts to detect when a process is done terminating by using a semaphore. This cannot work reliably. If the process being terminated signals the semaphore (as the test does) then the process waiting on the semaphore is free to run before the process is done terminating, and the assertion of isProcessTerminated will fail. And after the process is fully terminated, it cannot signal the semaphore. It would be more reliable to use process priority to make sure that the process is scheduled through full termination before the test process asserts the termination. I suggest this:

testSelfTerminate
    | value |
    value := nil.
    process := 
    [ value := mutex critical: 
        [ mutex terminateOwner.
        1 ] ] newProcess.
    process 
        priority: Processor activeProcess priority + 1;
        resume.
    "Processor yield."  "<--- required for GemStone prior to 3.7"
    self assert: mutex owner isNil.
    self assert: value isNil.
    self assert: (GRPlatform current isProcessTerminated: process)

This version passes in Pharo 9 and the latest GemStone 3.7. As noted, in the code, GemStone prior to 3.7 requires a yield, since it did not conform to Blue Book process preemption by priority. Please let me know if this version of the test fails in any other platforms supported by Seaside.