From aecef2a8f4e92d6913a2d771cee8011ceb1b6afb Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Sun, 9 Apr 2017 19:11:40 +0200 Subject: [PATCH] Commands: refactor integration into SessionCommandService (#1089) It seems more adequate to push the somewhat intricate mechanics for the "fall back" onto generic commands down into the implementation level of CommandInstanceManager. The point is, we know the standard usage situation is to rely on the instance manager, and thus we want to avoid redundant table lookups, only to support the rare case of fallback to global commands. The latter is currently used only from unit-tests, but might in future also be used by scripts. Due to thread safety considerations, I have refrained from handing out a direct reference to the command token sitting in the registry, even while not doing so incurs a small runtime penalty (accessing the shared ref-count for creating a copy of the smart-handle). This is the typical situation where you'd be tempted to sacrifice sanity for the sake of an imaginary performance benefit, which in fact is dwarfed by all the machinery of UI-Bus and argument passing via GenNode. --- src/include/session-command-facade.h | 15 +++- src/proc/control/command-instance-manager.hpp | 5 +- src/proc/control/command-registry.hpp | 7 +- src/proc/control/command-setup.cpp | 53 ++++++----- src/proc/control/command.cpp | 3 + src/proc/control/session-command-service.cpp | 44 +++++----- src/proc/control/session-command-service.hpp | 10 ++- tests/12metaprogramming.tests | 2 +- .../control/command-instance-manager-test.cpp | 34 +++++++- wiki/thinkPad.ichthyo.mm | 87 ++++++++++++++++--- 10 files changed, 192 insertions(+), 68 deletions(-) diff --git a/src/include/session-command-facade.h b/src/include/session-command-facade.h index 5179a8c6b..59fb8baba 100644 --- a/src/include/session-command-facade.h +++ b/src/include/session-command-facade.h @@ -73,11 +73,22 @@ namespace control { * qualify and elaborate the action, before it can be actually triggered. * Commands then go through a queue to be invoked one by one. * - * This is a layer separation facade interface. Clients should use - * the embedded #facade factory, which yields a proxy to route any + * The service exposed through this facade offers dedicated support for + * the _standard command cycle_, as is typically performed from the UI. + * Such a usage cycle starts with ["opening"](#cycle) a local anonymous + * clone copy from the global command definition, which is then used + * in further calls to be outfitted with actual arguments and finally + * to be handed over to the dispatcher for execution. + * @warning this standard command cycle is intended for single-threaded + * use from the UI. It is not threadsafe. To the contrary, all + * operations with globally registered commands are threadsafe. + * + * @remark This is a layer separation facade interface. Clients should + * use the embedded #facade factory, which yields a proxy to route any * calls through the lumieraorg_SessionCommand interface * @throws lumiera::error::State when interface is not opened * @see [Command system](command.hpp) + * @see SessionCommandFunction_test */ class SessionCommand { diff --git a/src/proc/control/command-instance-manager.hpp b/src/proc/control/command-instance-manager.hpp index 2d92e5c30..713ce9bc8 100644 --- a/src/proc/control/command-instance-manager.hpp +++ b/src/proc/control/command-instance-manager.hpp @@ -58,6 +58,7 @@ #define CONTROL_COMMAND_INSTANCE_MANAGER_H #include "lib/error.hpp" +#include "proc/control/command.hpp" #include "proc/control/command-dispatch.hpp" #include "lib/symbol.hpp" @@ -91,6 +92,7 @@ namespace control { * handle, the enqueued instance will stay alive until execution and then go out of scope. But, after * #dispatch, it is no longer accessible from the CommandInstanceManger, and while it is still waiting * in the execution queue, the next instance for the same invocationID might already be opened. + * @warning CommandInstanceManager is *not threadsafe* */ class CommandInstanceManager : boost::noncopyable @@ -103,8 +105,7 @@ namespace control { ~CommandInstanceManager(); Symbol newInstance (Symbol prototypeID, string const& invocationID); - Command& getInstance(Symbol instanceID); - Command& maybeGetInstance (Symbol instanceID); + Command getInstance(Symbol instanceID); void dispatch (Symbol instanceID); bool contains (Symbol instanceID) const; diff --git a/src/proc/control/command-registry.hpp b/src/proc/control/command-registry.hpp index ffb7f767f..eab91db38 100644 --- a/src/proc/control/command-registry.hpp +++ b/src/proc/control/command-registry.hpp @@ -188,7 +188,12 @@ namespace control { /** query the command index by ID * @return the registered command, - * or an "invalid" token */ + * or an "invalid" token + * @remark this function deliberately returns by-value. + * Returning a reference into the global CommandRegistry + * would be dangerous under concurrent access, since the + * lock is only acquired within this function's body. + */ Command queryIndex (Symbol cmdID) { diff --git a/src/proc/control/command-setup.cpp b/src/proc/control/command-setup.cpp index 01eb7533f..02d302e74 100644 --- a/src/proc/control/command-setup.cpp +++ b/src/proc/control/command-setup.cpp @@ -38,6 +38,7 @@ #include "lib/error.hpp" #include "include/logging.h" #include "include/lifecycle.h" +#include "proc/control/command.hpp" #include "proc/control/command-setup.hpp" #include "proc/control/command-instance-manager.hpp" #include "proc/control/command-def.hpp" @@ -192,40 +193,48 @@ namespace control { } - /** @return handle to the currently "opened" instance with the given ID */ - Command& + /** access the currently "opened" instance with the given instanceID + * @param instanceID ID as returned from #newInstance, or a global commandID + * @return instance handle of handle of a global command as fallback + * @note when given a commandID, which is not known as (decorated) instanceID + * within our local registration table, just the globally registered + * Command instance is returned. + * @remark deliberately this function returns by-value. Returning a reference + * into the global command registry would be dangerous under concurrency. + * @throw error::Invalid when the given cmdID unknown both locally and globally. + * @throw error::Logic when accessing an instance that _was_ known but is + * currently no longer "open" (already dispatched command instance) + */ + Command CommandInstanceManager::getInstance (Symbol instanceID) { - Command& instance = table_[instanceID]; - if (not instance) - throw error::Invalid (_Fmt{"Command instance '%s' is not (yet/anymore) active"} - % instanceID - , LUMIERA_ERROR_INVALID_COMMAND); - return instance; - } - - - /** try to retrieve an currently active instance, but tolerate unknown IDs */ - Command& - CommandInstanceManager::maybeGetInstance (Symbol instanceID) - { - static Command NOT_FOUND; auto entry = table_.find(instanceID); if (entry == table_.end()) - return NOT_FOUND; + return Command::get(instanceID); if (not entry->second) - throw error::Invalid (_Fmt{"Command instance '%s' is not (yet/anymore) active"} - % instanceID - , LUMIERA_ERROR_INVALID_COMMAND); + throw error::Logic (_Fmt{"Command instance '%s' is not (yet/anymore) active"} + % instanceID + , error::LUMIERA_ERROR_LIFECYCLE); return entry->second; } - /** hand over the designated command instance to the dispatcher installed on construction */ + /** hand over the designated command instance to the dispatcher installed + * on construction. Either the given ID corresponds to a previously "opened" + * local instance (known only to this instance manager). In this case, the + * instance will really be _moved_ over into the dispatcher, which also means + * this instance is no longer "open" for parametrisation. In the other case + * we try to retrieve the given ID from the global CommandRegistry and + * dispatch it, _without removing_ it from the global registry of course. + */ void CommandInstanceManager::dispatch (Symbol instanceID) { - Command& instance = table_[instanceID]; + auto entry = table_.find(instanceID); + Command fallback; + Command& instance{entry == table_.end()? fallback = Command::get(instanceID) + : entry->second + }; if (not instance) throw error::Logic (_Fmt{"attempt to dispatch command instance '%s' " "without creating a new instance from prototype beforehand"} diff --git a/src/proc/control/command.cpp b/src/proc/control/command.cpp index 7fb0138c8..9c248ac2e 100644 --- a/src/proc/control/command.cpp +++ b/src/proc/control/command.cpp @@ -119,6 +119,9 @@ namespace control { /** Access existing command for use. * @throw error::Invalid if command not * registered or incompletely defined. + * @remark this function deliberately returns by-value. + * Returning a reference into the global CommandRegistry + * would be dangerous under concurrent access. */ Command Command::get (Symbol cmdID) diff --git a/src/proc/control/session-command-service.cpp b/src/proc/control/session-command-service.cpp index 3468f71e6..af552d0f6 100644 --- a/src/proc/control/session-command-service.cpp +++ b/src/proc/control/session-command-service.cpp @@ -55,25 +55,6 @@ namespace control { using util::cStr; - /** - * @internal access the command instance for further processing. - * The standard use case is to get an anonymous command instance, - * which was previously opened within the CommandInstanceManager. - * But for exceptional cases, we also allow to access a global - * command instance by name. - * @param cmdID either the instanceID or the global cmdID - * @throw error::Invalid when no suitable command definition exists - */ - Command - SessionCommandService::retrieveCommand (Symbol cmdID) - { - Command cmdFound = instanceManager_.maybeGetInstance (cmdID); - return cmdFound? cmdFound - : Command::get(cmdID); - } - - - Symbol SessionCommandService::cycle (Symbol cmdID, string const& invocationID) { @@ -81,20 +62,35 @@ namespace control { } + /** + * @param cmdID either the instanceID or the global cmdID + * @throw error::Invalid when no suitable command definition exists + * @throw error::Logic when the instance is already dispatched + * @note this function automatically _falls back_ on a global + * Command definition, in case the given ID is not known + * as a local command instance. This allows to use the + * SessionCommand service without explicit instantiation + */ void SessionCommandService::bindArg (Symbol cmdID, Rec const& argSeq) { - retrieveCommand(cmdID).bindArg(argSeq); + instanceManager_.getInstance(cmdID) + .bindArg(argSeq); } + /** + * @param cmdID either the instanceID or the global cmdID + * @throw error::Invalid when no suitable command definition exists + * @throw error::State when the command's arguments are not bound + * @throw error::Logic when the instance is already dispatched + * @note similar to #bindArg, this function _falls back_ on a global + * Command definition, in case the given ID is not known locally. + */ void SessionCommandService::invoke (Symbol cmdID) { - if (instanceManager_.contains (cmdID)) - instanceManager_.dispatch(cmdID); - else - dispatcher_.enqueue (Command::get(cmdID)); + instanceManager_.dispatch(cmdID); } diff --git a/src/proc/control/session-command-service.hpp b/src/proc/control/session-command-service.hpp index c31b4e306..399099cc8 100644 --- a/src/proc/control/session-command-service.hpp +++ b/src/proc/control/session-command-service.hpp @@ -31,7 +31,8 @@ ** This service is the implementation of a layer separation facade interface. Clients should use ** proc::control::SessionCommand::facade to access this service. This header defines the interface ** used to _provide_ this service, not to access it. - ** + ** + ** @see session-command-facade.h ** @see facade.hpp subsystems for the Proc-Layer ** @see guifacade.cpp starting this service */ @@ -69,6 +70,11 @@ namespace control { * * This service is backed by implementation facilities embedded within * the ProcDispatcher, exposed through the CommandDispatch interface. + * Additionally, it operates a CommandInstanceManager to allow the + * creation of local instances "opened" for argument binding. + * In fact, this is the standard "command cycle" and the + * intended usage pattern. + * @warning local command instances are not threadsafe * @see DispatcherLoop */ class SessionCommandService @@ -78,8 +84,6 @@ namespace control { CommandDispatch& dispatcher_; CommandInstanceManager instanceManager_; - Command retrieveCommand (Symbol); - /* === Implementation of the Facade Interface === */ diff --git a/tests/12metaprogramming.tests b/tests/12metaprogramming.tests index 3998028cd..c1414bba8 100644 --- a/tests/12metaprogramming.tests +++ b/tests/12metaprogramming.tests @@ -560,7 +560,7 @@ TEST "tuple initialisation from GenNode" TupleRecordInit_test <»──(lalü,42) -out-lit: «tuple, string, int, long, double, Duration>»──(ID-lal,lala,12,34,5.6,≺548s7ms≻) +out-lit: «tuple, Symbol, int, long, double, Duration>»──(ID-lal,lala,12,34,5.6,≺548s7ms≻) return: 0 END diff --git a/tests/core/proc/control/command-instance-manager-test.cpp b/tests/core/proc/control/command-instance-manager-test.cpp index 5def03414..7689f8a39 100644 --- a/tests/core/proc/control/command-instance-manager-test.cpp +++ b/tests/core/proc/control/command-instance-manager-test.cpp @@ -114,6 +114,7 @@ namespace test { verify_instanceIdentity(); verify_duplicates(); verify_lifecycle(); + verify_fallback(); } @@ -135,7 +136,7 @@ namespace test { iManager.dispatch (instanceID); CHECK (fixture.contains (cmd)); CHECK (not iManager.contains (instanceID)); - VERIFY_ERROR (INVALID_COMMAND, iManager.getInstance (instanceID)); + VERIFY_ERROR (LIFECYCLE, iManager.getInstance (instanceID)); } @@ -293,9 +294,10 @@ namespace test { Fixture fixture; CommandInstanceManager iManager{fixture}; + // a manually constructed ID is unknown of course Symbol instanceID{COMMAND_PROTOTYPE, INVOCATION_ID}; VERIFY_ERROR (INVALID_COMMAND, iManager.getInstance (instanceID)); - VERIFY_ERROR (LIFECYCLE, iManager.dispatch (instanceID)); + VERIFY_ERROR (INVALID_COMMAND, iManager.dispatch (instanceID)); Symbol i2 = iManager.newInstance (COMMAND_PROTOTYPE, INVOCATION_ID); CHECK (i2 == instanceID); @@ -313,9 +315,35 @@ namespace test { iManager.dispatch (instanceID); CHECK (not iManager.contains (instanceID)); - VERIFY_ERROR (INVALID_COMMAND, iManager.getInstance (instanceID)); + VERIFY_ERROR (LIFECYCLE, iManager.getInstance (instanceID)); + VERIFY_ERROR (LIFECYCLE, iManager.dispatch (instanceID)); CHECK (instanceID == iManager.newInstance (COMMAND_PROTOTYPE, INVOCATION_ID)); } + + + /** @test the instance manager automatically falls back on globally registered commands, + * when the given ID is not and was not known locally */ + void + verify_fallback() + { + Fixture fixture; + CommandInstanceManager iManager{fixture}; + + CHECK (not iManager.contains (COMMAND_PROTOTYPE)); + Command cmd = iManager.getInstance (COMMAND_PROTOTYPE); + + CHECK (cmd.isValid()); + CHECK (not cmd.isAnonymous()); + CHECK (cmd == Command::get(COMMAND_PROTOTYPE)); + CHECK (cmd == Command{COMMAND_PROTOTYPE}); + + cmd.bind(-12); + CHECK (cmd.canExec()); + CHECK (not fixture.contains(cmd)); + + iManager.dispatch (COMMAND_PROTOTYPE); + CHECK (fixture.contains(cmd)); + } }; diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 569fd99e6..c2fb61594 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -11791,10 +11791,76 @@ + + + + + + + + + + + + + + + +

+ ...aus gutem Grund +

+

+ (kann mich erinnern, daß ich mir das überlegt hatte). +

+

+ Sofern Definitionen wirklich concurrent geändert oder gelöscht werden, +

+

+ könnte es sein, daß jemand auf einer stale reference arbeitet, +

+

+ denn das Lock schützt nur den Aufruf innerhalb der CommandRegistry. +

+

+ Sicher ist der Zugriff nur, wenn im Schutzbereich dieses Locks ein +

+

+ neues Command-Objekt kopiert wird. Was allerdings den RefCount erhöht. +

+ + + + + + + + + + + + + + + + + + + + +

+ aber sich mit einem Refcount verrückt machen..... +

+ + + + + + + - - + + @@ -11845,24 +11911,25 @@ - - + + - - - + + + - + + - + - +