On Fri, Jan 8, 2016 at 9:39 PM, Ben Coman <btc@openinworld.com> wrote:

btw, Looking at the following... CriticalSection>>critical: aBlock ^self primitiveEnterCriticalSection ifTrue: [aBlock value] ifFalse: [aBlock ensure: [self primitiveExitCriticalSection]]

without intimate VM knowledge but seeing Eliot recently say "invocations of methods with primitives [...] are not suspension points, unless their primitives fail" -- I'm paranoid that since #ensure: primitive 198 always fail, there might be some some small window for a race where a process might be terminated before #primitiveExitCriticalSection can be executed. I'll take a refutation of this to improve the method comment.

The following instils more confidence...

CriticalSection2>>critical: aBlock | reEntered | reEntered := false. [ reEntered := self primitiveEnterCriticalSection. aBlock value ] ensure: [ reEntered ifFalse: [ self primitiveExitCriticalSection] ]

Having spent more time considering this, I found I was off track here. If a process waiting at #primitiveEnterCriticalSection is terminated, then #primitiveExitCriticalSection is erroneously executed. And it will be the same problem with the proposal for Case17373 based on OwnedLock>>acquire Mutex>>critical: aBlock | lockAcquiredNotHere | <lockAt: #lock tracksStateAt: 1> lockAcquiredNotHere := true. ^[ lockAcquiredNotHere := false. lockAcquiredNotHere := lock acquire. aBlock value ] ensure: [lockAcquiredNotHere ifFalse: [lock release]]. So now I believe the original #critical: proposed by Eliot is optimal. * If #primitiveEnterCriticalSection is terminated while waiting, then the #ifFalse: is never executed. * When #primitiveEnterCriticalSection returns, the inlined #ifTrue:ifFalse cant be interrupted. * #ensure is a primitive which can't be interrupted. By the time it has done its usual "fail", which I raised concern over, actually it has already done its job to make sure #primitiveExitCriticalSection is executed.