From 370f5efa39c70974cbb76bdd55090e8a1aec9b1a Mon Sep 17 00:00:00 2001 From: Anton Tarasenko Date: Sat, 17 Apr 2021 17:31:06 +0700 Subject: [PATCH] Refactor signals/slots Refactor signal/slots system for more efficieny and better user interface that allows to disconnect receiver like so: `OnEvent(receiver).Disconnect()`. --- sources/Events/Signal.uc | 235 ++++++++++++++----- sources/Events/Slot.uc | 53 ++++- sources/Events/Tests/MockSignalSlotClient.uc | 64 ++++- sources/Events/Tests/TEST_SignalsSlots.uc | 72 +++++- 4 files changed, 347 insertions(+), 77 deletions(-) diff --git a/sources/Events/Signal.uc b/sources/Events/Signal.uc index 3f683d1..56b1ce4 100644 --- a/sources/Events/Signal.uc +++ b/sources/Events/Signal.uc @@ -38,28 +38,53 @@ class Signal extends AcediaObject abstract; +/** + * `Signal` essentially has to provide functionality for + * connecting/disconnecting slots and iterating through them. The main + * challenge is that slots can also be connected / disconnected during + * the signal emission. And in such cases we want: + * 1. To not propagate a signal to `Slot`s that were added during + * it's emission; + * 2. To not propagate a signal to any removed `Slot`, even if it was + * connected to the `Signal` in question when signal started emitting. + * + * We store connected `Slot`s in array, so to iterate we will simply use + * internal index variable `nextSlotIndex`. To account for removal of `Slot`s + * we will simply have to appropriately correct `nextSlotIndex` variable. + * To account for adding `Slot`s during signal emission we will first add them + * to a temporary queue `slotQueueToAdd` and only dump signals stored there + * into actual connected `Slot`s array before next iteration starts. + */ + // Class of the slot that can catch your `Signal` class var public const class relatedSlotClass; -// We want to always return a non-`none` slot to avoid "Access 'none'" -// errors. -// So if provided signal receiver is invalid - we still create a `Slot` -// for it, but remember it in this array of slots we aren't going to use -// in order to dispose of them later. -var private array failedSlots; - -// Set to `true` when we are in the process of deleting our `Slot`s. +// Set to `true` when we are in the process of removing connected `Slot`s. // Once `Slot` is deallocated, it notifies it's `Signal` (us) that it // should be removed. -// But if it's deallocated because we are removing it we want to ignore -// their notification and this flag helps us do that. +// But if it's deallocated because we are removing it, then we want to +// ignore that notification and this flag helps us do that. var private bool doingSelfCleaning; // Used to provide iterating interface (`StartIterating()` / `GetNextSlot()`). // Points at the next slot to return. var private int nextSlotIndex; -// These arrays could be defined as one array of `struct`s with four -// elements. +// This record describes slot-receiver pair to be added, along with it's +// life versions at the moment of adding a slot. Life versions help us verify +// that slot/receiver were not re-allocated at some point +// (thus becoming different objects). +struct SlotRecord +{ + var Slot slotInstance; + var int slotLifeVersion; + var AcediaObject receiver; + var int receiverLifeVersion; +}; +// Slots to be added before the next iteration (signal emission). +// We ensure that any added record has `slotInstance != none`. +var array slotQueueToAdd; + +// These arrays could be defined as one array of `SlotRecord` structs. // We use four different arrays instead for performance reasons. // They must have the same length at all times and elements with the // same index correspond to the same "record". @@ -92,7 +117,7 @@ public final function Emit() /* TEMPLATE for handlers with returned values: -public final function int Emit() +public final function Emit() { local newValue; local Slot nextSlot; @@ -109,7 +134,8 @@ public final function int Emit() nextSlot = GetNextSlot(); } CleanEmptySlots(); - return value; + // Return whatever you see fit after handling all the slots + return ; } */ @@ -124,10 +150,21 @@ public final function OnMyEvent(AcediaObject receiver) protected function Finalizer() { + local int i; doingSelfCleaning = true; - _.memory.FreeMany(registeredSlots); - doingSelfCleaning = false; + // Free queue for slot addition + for (i = 0; i < slotQueueToAdd.length; i += 1) + { + slotQueueToAdd[i].slotInstance + .FreeSelf(slotQueueToAdd[i].slotLifeVersion); + } + slotQueueToAdd.length = 0; + // Free actually connected slots + for (i = 0; i < registeredSlots.length; i += 1) { + registeredSlots[i].FreeSelf(slotLifeVersions[i]); + } registeredSlots.length = 0; + doingSelfCleaning = false; slotLifeVersions.length = 0; slotReceivers.length = 0; slotReceiversLifeVersions.length = 0; @@ -150,7 +187,7 @@ public final function Slot NewSlot(AcediaObject receiver) { local Slot newSlot; newSlot = Slot(_.memory.Allocate(relatedSlotClass)); - newSlot.Initialize(self); + newSlot.Initialize(self, receiver); AddSlot(newSlot, receiver); return newSlot; } @@ -158,21 +195,38 @@ public final function Slot NewSlot(AcediaObject receiver) /** * Disconnects all of the `receiver`'s `Slot`s from the caller `Signal`. * + * Meant to only be used by the `Slot`s `Disconnect()` method. + * * @param receiver Object to disconnect from the caller `Signal`. + * If `none` is passed, does nothing. */ public final function Disconnect(AcediaObject receiver) { local int i; + if (receiver == none) { + return; + } doingSelfCleaning = true; - while (i < slotReceivers.length) + // Clean from the queue for addition + i = 0; + while (i < slotQueueToAdd.length) { - if (slotReceivers[i] == none || slotReceivers[i] == receiver) + if (slotQueueToAdd[i].receiver == receiver) { - _.memory.Free(registeredSlots[i]); - registeredSlots.Remove(i, 1); - slotLifeVersions.Remove(i, 1); - slotReceivers.Remove(i, 1); - slotReceiversLifeVersions.Remove(i, 1); + slotQueueToAdd[i].slotInstance + .FreeSelf(slotQueueToAdd[i].slotLifeVersion); + slotQueueToAdd.Remove(i, 1); + } + else { + i += 1; + } + } + // Clean from the active slots + i = 0; + while (i < slotReceivers.length) + { + if (slotReceivers[i] == receiver) { + RemoveSlotAtIndex(i); } else { i += 1; @@ -182,56 +236,91 @@ public final function Disconnect(AcediaObject receiver) } /** - * Adds new `Slot` `newSlot` with receiver `receiver` to the caller `Signal`. + * Adds new `Slot` (`newSlot`) with receiver `receiver` to the caller `Signal`. * - * Does nothing if `newSlot` is already added to the caller `Signal`. + * Does nothing if `newSlot` is already added to the caller `Signal` + * (even if it's added with a different receiver). * * @param newSlot Slot to add. Must be initialize for the caller `Signal`. * @param receiver Receiver to which new `Slot` would be connected. * Method connected to a `Slot` generated by this method must belong to * the `receiver`, otherwise behavior of `Signal`-`Slot` system is - * undefined. - * Must be a properly allocated `AcediaObject`. + * undefined. Must be a properly allocated `AcediaObject`. */ protected final function AddSlot(Slot newSlot, AcediaObject receiver) { - local int i; - local int newSlotIndex; - if (newSlot == none) return; - if (newSlot.class != relatedSlotClass) return; - if (!newSlot.IsOwnerSignal(self)) return; - if (receiver == none || !receiver.IsAllocated()) + local SlotRecord newRecord; + if (newSlot == none) { + return; + } + newRecord.slotInstance = newSlot; + newRecord.slotLifeVersion = newSlot.GetLifeVersion(); + newRecord.receiver = receiver; + if (receiver != none) { + newRecord.receiverLifeVersion = receiver.GetLifeVersion(); + } + slotQueueToAdd[slotQueueToAdd.length] = newRecord; +} + +// Attempts to add a `Slot` from a `SlotRecord` into array of currently +// connected `Slot`s. +// IMPORTANT: Must only be called right before a new iteration +// (signal emission) through the `Slot`s. Otherwise `Signal`'s behavior +// should be considered undefined. +private final function AddSlotRecord(SlotRecord record) +{ + local int i; + local int newSlotIndex; + local Slot newSlot; + local AcediaObject receiver; + newSlot = record.slotInstance; + receiver = record.receiver; + if (newSlot.class != relatedSlotClass) return; + if (!newSlot.IsOwnerSignal(self)) return; + // Slot got outdated while waiting in queue + if (newSlot.GetLifeVersion() != record.slotLifeVersion) return; + + // Receiver is outright invalid or got outdated + if ( receiver == none + || !receiver.IsAllocated() + || receiver.GetLifeVersion() != record.receiverLifeVersion) { - failedSlots[failedSlots.length] = newSlot; + doingSelfCleaning = true; + newSlot.FreeSelf(); + doingSelfCleaning = false; return; } + // Check if that slot is already added for (i = 0; i < registeredSlots.length; i += 1) { if (registeredSlots[i] != newSlot) { continue; } - if (slotLifeVersions[i] != newSlot.GetLifeVersion()) + // If we have the same instance recorded, but... + // 1. it was reallocated: update it's records; + // 2. it was not reallocated: leave the records intact. + // Neither would case issues with iterating along `Slot`s if this + // method is only called right before new iteration. + if (slotLifeVersions[i] != record.slotLifeVersion) { - slotLifeVersions[i] = newSlot.GetLifeVersion(); + slotLifeVersions[i] = record.slotLifeVersion; slotReceivers[i] = receiver; if (receiver != none) { - slotReceiversLifeVersions[i] = receiver.GetLifeVersion(); + slotReceiversLifeVersions[i] = record.receiverLifeVersion; } } return; } newSlotIndex = registeredSlots.length; registeredSlots[newSlotIndex] = newSlot; - slotLifeVersions[newSlotIndex] = newSlot.GetLifeVersion(); + slotLifeVersions[newSlotIndex] = record.slotLifeVersion; slotReceivers[newSlotIndex] = receiver; - slotReceiversLifeVersions[newSlotIndex] = receiver.GetLifeVersion(); + slotReceiversLifeVersions[newSlotIndex] = record.receiverLifeVersion; } /** * Removes given `slotToRemove` if it was connected to the caller `Signal`. * - * Does not deallocate `slotToRemove`. - * * Cannot fail. * * @param slotToRemove Slot to be removed. @@ -242,14 +331,24 @@ public final function RemoveSlot(Slot slotToRemove) if (slotToRemove == none) return; if (doingSelfCleaning) return; + // Remove from queue for addition + while (i < slotQueueToAdd.length) + { + if (slotQueueToAdd[i].slotInstance == slotToRemove) + { + slotToRemove.FreeSelf(slotQueueToAdd[i].slotLifeVersion); + slotQueueToAdd.Remove(i, 1); + } + else { + i += 1; + } + } + // Remove from active slots for (i = 0; i < registeredSlots.length; i += 1) { if (registeredSlots[i] == slotToRemove) { - registeredSlots.Remove(i, 1); - slotLifeVersions.Remove(i, 1); - slotReceivers.Remove(i, 1); - slotReceiversLifeVersions.Remove(i, 1); + RemoveSlotAtIndex(i); return; } } @@ -265,6 +364,11 @@ public final function RemoveSlot(Slot slotToRemove) */ protected final function StartIterating() { + local int i; + for (i = 0; i < slotQueueToAdd.length; i += 1) { + AddSlotRecord(slotQueueToAdd[i]); + } + slotQueueToAdd.length = 0; nextSlotIndex = 0; } @@ -298,15 +402,11 @@ protected final function Slot GetNextSlot() if (isNextSlotValid) { nextSlotIndex += 1; + doingSelfCleaning = false; return nextSlot; } - else - { - registeredSlots.Remove(nextSlotIndex, 1); - slotLifeVersions.Remove(nextSlotIndex, 1); - slotReceivers.Remove(nextSlotIndex, 1); - slotReceiversLifeVersions.Remove(nextSlotIndex, 1); - _.memory.Free(nextSlot); + else { + RemoveSlotAtIndex(nextSlotIndex); } } doingSelfCleaning = false; @@ -315,24 +415,16 @@ protected final function Slot GetNextSlot() /** * In case it's detected that some of the slots do not actually have any - * handler setup - this method will clean them up. + * delegate set - this method will clean them up. */ protected final function CleanEmptySlots() { local int index; - _.memory.FreeMany(failedSlots); - failedSlots.length = 0; doingSelfCleaning = true; while (index < registeredSlots.length) { - if (registeredSlots[index].IsEmpty()) - { - registeredSlots[index].FreeSelf(slotLifeVersions[index]); - _.memory.Free(registeredSlots[index]); - registeredSlots.Remove(index, 1); - slotLifeVersions.Remove(index, 1); - slotReceivers.Remove(index, 1); - slotReceiversLifeVersions.Remove(index, 1); + if (registeredSlots[index].IsEmpty()) { + RemoveSlotAtIndex(index); } else { index += 1; @@ -341,6 +433,21 @@ protected final function CleanEmptySlots() doingSelfCleaning = false; } +// Removes `Slot` at a given `index`. +// Assumes that passed index is within boundaries. +private final function RemoveSlotAtIndex(int index) +{ + registeredSlots[index].FreeSelf(slotLifeVersions[index]); + registeredSlots.Remove(index, 1); + slotLifeVersions.Remove(index, 1); + slotReceivers.Remove(index, 1); + slotReceiversLifeVersions.Remove(index, 1); + // Alter iteration index `nextSlotIndex` to account for this `Slot` removal + if (nextSlotIndex > index) { + nextSlotIndex -= 1; + } +} + defaultproperties { relatedSlotClass = class'Slot' diff --git a/sources/Events/Slot.uc b/sources/Events/Slot.uc index 2a72ce6..e75656a 100644 --- a/sources/Events/Slot.uc +++ b/sources/Events/Slot.uc @@ -33,8 +33,10 @@ class Slot extends AcediaObject abstract; -var private bool dummyMethodCalled; -var private Signal ownerSignal; +var private bool dummyMethodCalled; +var private AcediaObject myReceiver; +var private int myReceiverLifeVersion; +var private Signal mySignal; /* TEMPLATE for handlers without returned values: delegate connect() @@ -78,10 +80,11 @@ protected function Finalizer() protected function Finalizer() { dummyMethodCalled = false; - if (ownerSignal != none) { - ownerSignal.RemoveSlot(self); + if (mySignal != none) { + mySignal.RemoveSlot(self); } - ownerSignal = none; + mySignal = none; + myReceiver = none; } /** @@ -93,8 +96,8 @@ protected function Finalizer() protected final function DummyCall() { dummyMethodCalled = true; - // We do not want to call `ownerSignal.RemoveSlot(self)` here, since - // `ownerSignal` is likely in process of iterating through it's `Slot`s + // We do not want to call `mySignal.RemoveSlot(self)` here, since + // `mySignal` is likely in process of iterating through it's `Slot`s // and removing (or adding) `Slot`s from it can mess up that process. } @@ -104,21 +107,29 @@ protected final function DummyCall() * Can only be done once for every `Slot`. * * @param newOwnerSignal `Signal` we want to receive emitted signals from. + * @param receiver Receiver object that caused creation of this `Slot`. * @return `true` if initialization was successful and `false` otherwise * (if `newOwnerSignal` is invalid or caller `Slot` was * already initialized). */ -public final function bool Initialize(Signal newOwnerSignal) +public final function bool Initialize( + Signal newOwnerSignal, + AcediaObject receiver) { - if (ownerSignal != none) { + if (mySignal != none) { return false; } + if (receiver != none && receiver.IsAllocated()) + { + myReceiver = receiver; + myReceiverLifeVersion = receiver.GetLifeVersion(); + } if (newOwnerSignal == none || !newOwnerSignal.IsAllocated()) { FreeSelf(); return false; } - ownerSignal = newOwnerSignal; + mySignal = newOwnerSignal; return true; } @@ -131,7 +142,27 @@ public final function bool Initialize(Signal newOwnerSignal) */ public final function bool IsOwnerSignal(Signal testSignal) { - return (ownerSignal == testSignal); + return (mySignal == testSignal); +} + +/** + * Disconnects all the `Slot`s that belong to receiver of the caller `Slot` + * from our owner signal `mySignal`. + * + * Method's name does not reflect what it does well, however this is done + * deliberately to provide an overall more intuitive user interface to + * signals and slots that allows for receiver to disconnect from `Signal` in + * the similar way to how it connects: + * `eventSource.OnSomeEvent(self).connect = handler` + * `eventSource.OnSomeEvent(self).Disconnect()` + */ +public final function Disconnect() +{ + if (mySignal == none) return; + if (myReceiver == none) return; + if (myReceiver.GetLifeVersion() != myReceiverLifeVersion) return; + + mySignal.Disconnect(myReceiver); } /** diff --git a/sources/Events/Tests/MockSignalSlotClient.uc b/sources/Events/Tests/MockSignalSlotClient.uc index dbfdf69..b414b5d 100644 --- a/sources/Events/Tests/MockSignalSlotClient.uc +++ b/sources/Events/Tests/MockSignalSlotClient.uc @@ -20,22 +20,57 @@ */ class MockSignalSlotClient extends AcediaObject; -var private int value; +// Remember `Signal` and `Slot` for testing purposes +var private MockSignal usedSignal; +var private MockSlot usedSlot; +var private int value; public final function SetValue(int newValue) { value = newValue; } +public final function DisconnectMe(MockSignal signal) +{ + if (signal != none) { + signal.NewSlot(self).Disconnect(); + } +} + // Return `SMockSlot` for testing purposes public final function MockSlot AddToSignal(MockSignal signal) { local MockSlot slot; + if (signal == none) { + return none; + } slot = MockSlot(signal.NewSlot(self)); slot.connect = Handler; return slot; } +public final function MockSlot AddToSignal_AddNewSlot(MockSignal signal) +{ + local MockSlot slot; + if (signal == none) { + return none; + } + usedSignal = signal; + slot = MockSlot(signal.NewSlot(self)); + slot.connect = Handler_AddNewSlot; + return slot; +} + +public final function MockSlot AddToSignal_DestroySlot(MockSignal signal) +{ + if (signal == none) { + return none; + } + usedSlot = MockSlot(signal.NewSlot(self)); + usedSlot.connect = Handler_DestroySlot; + return usedSlot; +} + private final function int Handler(int inputValue, optional bool doSubtract) { if (doSubtract) { @@ -44,6 +79,33 @@ private final function int Handler(int inputValue, optional bool doSubtract) return inputValue + value; } +private final function int Handler_AddNewSlot( + int inputValue, + optional bool doSubtract) +{ + local MockSignalSlotClient newClient; + if (usedSignal == none) { + return inputValue; + } + newClient = MockSignalSlotClient( + _.memory.Allocate(class'MockSignalSlotClient')); + newClient.SetValue(value); + newClient.AddToSignal(usedSignal); + usedSignal = none; + return inputValue; +} + +private final function int Handler_DestroySlot( + int inputValue, + optional bool doSubtract) +{ + if (usedSlot != none) { + usedSlot.FreeSelf(); + } + usedSlot = none; + return inputValue; +} + defaultproperties { } \ No newline at end of file diff --git a/sources/Events/Tests/TEST_SignalsSlots.uc b/sources/Events/Tests/TEST_SignalsSlots.uc index 6920b6c..64df3f9 100644 --- a/sources/Events/Tests/TEST_SignalsSlots.uc +++ b/sources/Events/Tests/TEST_SignalsSlots.uc @@ -26,11 +26,16 @@ protected static function TESTS() @ "a signal."); Test_Connecting(); Test_Disconnecting(); + Test_DisconnectingThroughSlot(); Context("Testing how signals and slots system handles deallocations and" @ "unexpected changes to managed objects."); Test_DeallocSlots(); Test_EmptySlots(); Test_DeallocReceivers(); + Context("Testing how signals and slots system handles connecting and" + @ "disconnections of slots while a signal is being emitted."); + Test_ConnectSlotsDuringEmittion(); + Test_DisconnectSlotsDuringEmittion(); } protected static function Test_Connecting() @@ -95,6 +100,25 @@ protected static function Test_Disconnecting() __().memory.FreeMany(objects); } +protected static function Test_DisconnectingThroughSlot() +{ + local MockSignal signal; + local MockSlot slot1, slot2, slot3; + local MockSignalSlotClient client; + Issue("`Disconnect()` defined for `Slot()` does not properly disconnect all" + @ "of it's receiver's slots."); + signal = MockSignal(__().memory.Allocate(class'MockSignal')); + client = MockSignalSlotClient( + __().memory.Allocate(class'MockSignalSlotClient')); + slot1 = client.AddToSignal_AddNewSlot(signal); + slot2 = client.AddToSignal_DestroySlot(signal); + slot3 = client.AddToSignal(signal); + client.DisconnectMe(signal); + TEST_ExpectFalse(slot1.IsAllocated()); + TEST_ExpectFalse(slot2.IsAllocated()); + TEST_ExpectFalse(slot3.IsAllocated()); +} + protected static function Test_DeallocSlots() { local int i; @@ -202,11 +226,57 @@ protected static function Test_DeallocReceivers() } } Issue("Slots with deallocated receivers are not deallocated."); - TEST_ExpectFalse(true); + TEST_ExpectFalse(slotsAreNotDeallocated); __().memory.Free(signal); __().memory.FreeMany(objects); } +protected static function Test_ConnectSlotsDuringEmittion() +{ + local MockSignal signal; + local MockSignalSlotClient clientWithNewConnection, regularClient; + Issue("Slots added during signal emission also receive that signal."); + signal = MockSignal(__().memory.Allocate(class'MockSignal')); + clientWithNewConnection = MockSignalSlotClient( + __().memory.Allocate(class'MockSignalSlotClient')); + regularClient = MockSignalSlotClient( + __().memory.Allocate(class'MockSignalSlotClient')); + clientWithNewConnection.AddToSignal_AddNewSlot(signal); + regularClient.AddToSignal(signal); + clientWithNewConnection.SetValue(27); + regularClient.SetValue(23); + // Ideally `clientWithNewConnection` only adds a new slot to which + // an object with the same value as `clientWithNewConnection` is connected. + // That newly added object should not affect hat same signal emission. + // This test checks that it actually does not, which would mean that + // only `regularClient` alters resulting value and sets it to `23`. + TEST_ExpectTrue(signal.Emit(0) == 23); +} + +protected static function Test_DisconnectSlotsDuringEmittion() +{ + local MockSignal signal; + local MockSignalSlotClient clientThatDestroys, regularClient; + Issue("Some slots do not receive a signal if one of them was removed during" + @ "that signal's emission."); + signal = MockSignal(__().memory.Allocate(class'MockSignal')); + clientThatDestroys = MockSignalSlotClient( + __().memory.Allocate(class'MockSignalSlotClient')); + regularClient = MockSignalSlotClient( + __().memory.Allocate(class'MockSignalSlotClient')); + clientThatDestroys.AddToSignal_DestroySlot(signal); + regularClient.AddToSignal(signal); + regularClient.SetValue(23); + // Ideally `clientThatDestroys` only removes itself from a `signal`, + // without affecting whether other `Slot`s receive a signal. However, + // in one of the previous implementations that prevented the next `Slot` + // from receiving signal emission. + // This test checks that this does not actually happen, which would + // mean that `regularClient` still alters resulting value and sets it + // to `23`. + TEST_ExpectTrue(signal.Emit(0) == 23); +} + defaultproperties { caseGroup = "Events"