On 06 Jan 2016, at 10:25 , Ben Coman <btc@openinworld.com> wrote:
In ExternalSemaphoreTable I noticed two class variables set by class initialization as: ProtectAdd := Semaphore forMutualExclusion. ProtectRemove := Semaphore forMutualExclusion
and I'm curious, what are the TWO shared resources that these TWO are meant to protect? it seems there is only one, that being unprotectedExternalObjects. There is only one resource, but two classes of operations that need to be independently serialized. You can safely add and remove from the array concurrently, just concurrent adds or concurrent removes might lead to trouble. Using two semaphores in this case is probably less useful in practice than as an example of how more finely grained locks are sometimes possible.
I notice that "ExternalSemaphoreTable class>>safelyUnregisterExternalObject:" stores unprotectedExternalObjects in its local variable "objects" and then up removes an object from it.
The safely: should be moved to private category, probably renamed private* , and have a comment stating that they are the inner critical sections; calls must be protected by the appropriate locks.
But "ExternalSemaphoreTable class>>safelyRegisterExternalObject:" also stores unprotectedExternalObjects in its local variable "objects" which it then passes to "ExternalSemaphoreTable class>>collectionBasedOn:withRoomFor:" which takes a copy to grow a new collection to store into unprotectedExternalObjects.
Even though its a rare case that the external semaphore might grow at just the same time that an object is being unregistered, and also practically #safelyUnregisterExternalObject: might not be preemptable, this smells bad having two mutual exclusions covering one resource - thus no mutual exclusion at all. Is there something I'm missing?
Not really. The only valid caller of collectionBasedOn: is addExternalObject, so the method always executes inside ProtectAdd critical section. There is, however, a ProtectRemove critical section missing, should be one around: newObjects replaceFrom: 1 to: externalObjects size with: externalObjects startingAt: 1. self unprotectedExternalObjects: newObjects. Cheers, Henry