On Thu, Jan 7, 2016 at 1:48 AM, Denis Kudriashov <dionisiydk@gmail.com> wrote:
I realize that current approach to handle situation in the #terminate method is the only way. But we need to generalize it somehow and make more explicit. It should be visible inside method which required such logic (like current Semaphore>>critical:).
You suggest something like culling unwind context to ensure method. But it looks like too radical solution. And it is not easy to implement.
Cant we implement Semaphore>>waitIfInterrupted: interruptingBlock? And we put in terminating logic only this knowledge. Terminate method should detect this method and call interruptingBlock . So clients can set simple flag in such cases that lock was not acquired. For example:
critical: aBlock
[signalRequired := true. semaphore waitIfInterrupted: [signalRequired := false]. ^aBlock value] ensure: [signalRequired ifTrue: [semaphore signal]]
What you think?
I'm not qualified to judge, but is sounds okay. I have an alternative proposal to consider that hopefully might remove Semaphore handling completely from Process>>terminate. What if we have a wait primitive that instead of returning itself (i.e. a Semaphore), it always returns true. Then if the process is terminated while the primitive is waiting, before it consumes a signal, the assignment to signalConsumed never occurs. Then we have... critical: mutuallyExcludedBlock signalConsumed := false. [ signalConsumed := self primWaitSucceeded. blockValue := mutuallyExcludedBlock value ] ensure: [ signalConsumed ifTrue: [self signal] ]. ^blockValue It *feels* more atomic. Someone knowledgeable with the vm would need to advise whether after the primitive wakes up and consumes a signal, the assignment into signalConsumed can be guaranteed to occur before the process gets a chance to terminate, even if the primitive needs to directly poke the value into signalConsumed. Maybe the following alternative would be required... critical: mutuallyExcludedBlock signalConsumed := false. [ self primWaitSucceeded: signalConsumed. blockValue := mutuallyExcludedBlock value ] ensure: [ signalConsumed ifTrue: [self signal] ]. ^blockValue but I think my first proposal reads better. OR.... we could have a primitive similar to ensure: ensure: | complete returnValue | <primitive: 198> returnValue := self valueNoContextSwitch. complete ifNil:[ complete := true. aBlock value ]. ^ returnValue where I guess the primitive manipulates local variable "complete" (??). Then it could set that based on whether the primitive consumed a signal or not... afterWaitSucceedsEnsure: aBlock | completeAndNeedSignal mutuallyExcludedValue | <primitive: 198b > mutuallyExcludedValue := self valueNoContextSwitch. completeAndNeedSignal ifNil:[ completeAndNeedSignal := true. aBlock value. ]. ^ mutuallyExcludedValue Then we could just have critical: mutuallyExcludedBlock mutuallyExcludedBlock afterWaitSucceedsEnsure: [ self signal ]. but I still like my first proposal better. cheers -ben
2016-01-06 14:48 GMT+01:00 Denis Kudriashov <dionisiydk@gmail.com>:
I publish slice with MutexTest. So future changes around all this will be verified by CI
2016-01-06 14:32 GMT+01:00 Denis Kudriashov <dionisiydk@gmail.com>:
Ok. I wrote MutextTests which show problems. See in attachment. Attached Mutex.st contains fix which suggest Ben for his scenario. But it broke other scenario.
Mutex>>critical: aBlock "Evaluate aBlock protected by the receiver." | activeProcess signalRequired blockValue | activeProcess := Processor activeProcess. activeProcess == owner ifTrue:[^aBlock value]. signalRequired := false. [ signalRequired := true. semaphore wait. owner:= activeProcess. blockValue := aBlock value ] ensure: [signalRequired & (owner == activeProcess) ifTrue: [owner := nil. semaphore signal]]. ^blockValue
My idea of ifCurtailed: usage not helps too. But if you change MutexTest to use Semaphore instead of Mutex then all tests will be green.
2016-01-06 13:38 GMT+01:00 Henrik Johansen <henrik.s.johansen@veloxit.no>:
If you run it, and are confused '2 start' never shows up in transcript, I intended the start messages to be outside critical blocks, sorry for old copypasta.
Cheers, Henry
On 06 Jan 2016, at 1:29 , Henrik Johansen <henrik.s.johansen@veloxit.no> wrote:
On 05 Jan 2016, at 7:52 , Henrik Johansen <henrik.s.johansen@veloxit.no> wrote:
On 05 Jan 2016, at 7:24 , Denis Kudriashov <dionisiydk@gmail.com> wrote:
2016-01-05 18:49 GMT+01:00 Henrik Johansen <henrik.s.johansen@veloxit.no>:
ifCurtailed: only unwinds if an error/termination occured, so that would mean not signalling the semaphore when everything goes as planned, and we leave the critical section...
I not suppose to replace #ensure: with #ifCurtailed:. I think about:
signalRequired := false. [ [signalRequired := true. self wait] ifCurtailed: [signalRequired := false]. blockValue := mutuallyExcludedBlock value ] ensure: [signalRequired ifTrue: [self signal]]. ^blockValue
Ah, that makes more sense, yes, much simpler than what I wrote! AFAICT, it should work too ;)
After thinking a bit more, it suffers from a similar edge case where the semaphore has been signaled, but process is terminated before it's had a chance to run, after changing to ifCurtailed and cleaning #terminate:
"This runs on UI thread, pri 40" waiter := Semaphore new. waiter2 := Semaphore new. lock := Semaphore forMutualExclusion.
proc := [lock critical: [ Transcript crShow: '1 start'. waiter wait. Transcript crShow: '1 done'.]] newProcess. proc priority: 70. proc resume.
proc2 := [lock critical: [ Transcript crShow: '2 start'. waiter2 wait. Transcript crShow: '2 done'. ]] newProcess. proc2 priority: 15. proc2 resume.
"Yield for proc2 so it gets to waiting on lock" 1 second wait. "Signal waiter and yield, causing proc1 to run, and release the lock. lock will then remove proc2 from waiting list" waiter signal. Processor yield. "We have a higher pri than proc2, and will resume execution here before proc2 has chance to run" proc2 terminate. "proc2 has been curtailed before running, thus ifCurtailed: guard is still on stack and will be run as part of termination." (lock instVarNamed: 'excessSignals') 0 "Should be 1"
Cheers, Henry