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 @@ - - + + - - - + + + - + + - + - +