From 77303ad00736dd4b13b4a95c6d4442716b830d4f Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Wed, 4 Jan 2017 01:44:35 +0100 Subject: [PATCH] Session-Subsystem(#318): investigation of locking sanity (ongoing...) Found an inconsistency and a deadlock! See proc-dispatcher.cpp, the lambda embedded into the start() operation! --- src/common/subsys.hpp | 8 +- src/common/subsystem-runner.hpp | 2 +- src/proc/control/proc-dispatcher.cpp | 24 ++- src/proc/control/proc-dispatcher.hpp | 29 +++- src/proc/facade.cpp | 5 +- src/proc/facade.hpp | 8 +- .../proc/control/dispatcher-looper-test.cpp | 4 +- wiki/thinkPad.ichthyo.mm | 137 ++++++++---------- 8 files changed, 126 insertions(+), 91 deletions(-) diff --git a/src/common/subsys.hpp b/src/common/subsys.hpp index 67276f39e..04a395c39 100644 --- a/src/common/subsys.hpp +++ b/src/common/subsys.hpp @@ -95,9 +95,11 @@ namespace lumiera { virtual bool shouldStart (lumiera::Option&) =0; - /** how to start up this subsystem. Failure to start up - * usually terminates the whole application. When this subsystem - * ceases to work, it must assure to activate the given signal. + /** attempt to bring up this subsystem up. On return, the + * subsystem must be operational, according to checkRunningState(). + * Failure to start up usually terminates the whole application. + * When this subsystem ceases to work, it must ensure to activate + * the given callback signal. * @param options may be influencing the operation mode * @param termination to be signalled by the subsystem. * @warning termination must be signalled reliably. diff --git a/src/common/subsystem-runner.hpp b/src/common/subsystem-runner.hpp index e6ffe5a59..92d5800de 100644 --- a/src/common/subsystem-runner.hpp +++ b/src/common/subsystem-runner.hpp @@ -170,7 +170,7 @@ namespace lumiera { throw error::Logic("Subsystem "+string(*susy)+" failed to start"); } - if (!and_all (susy->getPrerequisites(), isRunning() )) + if (not and_all (susy->getPrerequisites(), isRunning() )) { susy->triggerShutdown(); throw error::State("Unable to start all prerequisites of Subsystem "+string(*susy)); diff --git a/src/proc/control/proc-dispatcher.cpp b/src/proc/control/proc-dispatcher.cpp index 5cbfb8432..711dc98ba 100644 --- a/src/proc/control/proc-dispatcher.cpp +++ b/src/proc/control/proc-dispatcher.cpp @@ -128,6 +128,14 @@ namespace control { public: + /** start the session loop thread + * @param notification callback to invoke on thread termination + * @remark _in theory_ this ctor could block, since it waits for the thread + * actually to get operational and it waits for the SessionCommand interface + * to be opened. The latter _should not_ run into any obstacles, because + * in case it does, the main application thread is deadlocked on startup. + * Such might happen indirectly, when something depends on "the Session" + */ DispatcherLoop (Subsys::SigTerm notification) : ThreadJoinable("Lumiera Session" , bind (&DispatcherLoop::runSessionThread, this, notification)) @@ -315,7 +323,14 @@ namespace control { - /** */ + /** starting the ProcDispatcher means to start the session subsystem. + * @return `false` when _starting_ failed since it is already running... + * @remark this function implements the start operation for the »session subsystem«. + * More specifically, this operation starts a new thread to perform the + * _session loop,_ which means to perform commands and trigger the builder. + * It might block temporarily for synchronisation with this new thread and + * while opening the SessionCommand facade. + */ bool ProcDispatcher::start (Subsys::SigTerm termNotification) { @@ -326,7 +341,7 @@ namespace control { new DispatcherLoop ( [=] (string* problemMessage) { - runningLoop_.reset(); + runningLoop_.reset(); //////////////////////TODO: (1) Race in ProcDispatcher (2) Deadlock because the Thread attempts to reap itself termNotification(problemMessage); })); @@ -336,7 +351,10 @@ namespace control { } - /** */ + /** whether the »session subsystem« is operational. + * @return `true` if the session loop thread has been fully + * started and is not (yet) completely terminated. + */ bool ProcDispatcher::isRunning() { diff --git a/src/proc/control/proc-dispatcher.hpp b/src/proc/control/proc-dispatcher.hpp index dd57fb48f..37a464bd5 100644 --- a/src/proc/control/proc-dispatcher.hpp +++ b/src/proc/control/proc-dispatcher.hpp @@ -23,10 +23,29 @@ /** @file proc-dispatcher.hpp ** Dispatch and execute mutation operations on the High-level model. - ** //TODO + ** The ProcDispatcher is the application facility to back and support working + ** on the Session. While the session itself is just a data structure, _operating_ + ** the session means to process commands changing that data structure and it means + ** to evaluate the _session model_ and _build_ a render nodes network in accordance + ** to that model's meaning. This _operation aspect_ of the session thus has a lifecycle + ** and this is what we call the "session subsystem" within the application. When this is + ** running, the SessionCommand facade is opened, accepting commands to work on the session. + ** Such commands are passed through a dispatch queue to ensure consistent session state + ** and to force strictly sequential processing of commands. Moreover, the ProcDispatcher's + ** responsibility is to care for triggering the Builder after any changes induced by those + ** commands. The Builder is guaranteed to run _eventually_, yet with some leeway. + ** + ** Talking of state, independent of the lifecycle running state, the ProcDispatcher can be + ** _activated or deactivated_. In active state, commands are dequeued and processed, while + ** in inactive state commands are just accepted and queued, assuming the SessionCommand + ** interface is currently opened. This activation state is controlled by the session + ** datastructure itself, more precisely the SessionManager. A session can be stored, + ** closed, reset and loaded with existing content from persistent storage, and only + ** when the data structure is complete and consistent, processing can be enabled. ** ** @see Command ** @see Session + ** @see proc-dispatcher.cpp for details of operational semantics ** */ @@ -54,7 +73,13 @@ namespace control { /** - * @todo Type-comment + * Guard to manage processing commands working on the session. + * A static application facility, actually backing and implementing + * the »session subsystem«. Embedded within the implementation of this + * class is the _»session loop thread«_ to perform any session mutation + * commands and to operate the Builder, which translates the session + * contents into a render nodes network. Also embedded herein is + * the implementation of proc::control::SessionCommandService */ class ProcDispatcher : public lib::Sync<> diff --git a/src/proc/facade.cpp b/src/proc/facade.cpp index 4378815f1..9bdfc328e 100644 --- a/src/proc/facade.cpp +++ b/src/proc/facade.cpp @@ -47,8 +47,9 @@ namespace proc { { operator string() const { return "Session"; } - /** @remarks there is no need explicitly to start the session core, - * since it will usually be pulled up as prerequisite */ + /** @remarks there is no need explicitly to start the session, + * since it is passive, waiting for invocations and will be + * pulled up as prerequisite of other subsystems. */ bool shouldStart (Option&) override { diff --git a/src/proc/facade.hpp b/src/proc/facade.hpp index d099e9513..534c4c5da 100644 --- a/src/proc/facade.hpp +++ b/src/proc/facade.hpp @@ -56,14 +56,14 @@ namespace proc { struct Facade { /** provide a descriptor for lumiera::AppState, - * wired accordingly to allow main to bring up - * a editing session, possibly by loading an - * existing session from storage. */ + * wired accordingly to allow `main()` to boot + * the support infrastructure for loading of + * and working on the editing session. */ static lumiera::Subsys& getSessionDescriptor(); /** provide a descriptor for lumiera::AppState, - * wired accordingly to allow main to bring up + * wired accordingly to allow `main()` to bring up * the render / playback coordination and * output management subsystem. */ static lumiera::Subsys& getPlayOutDescriptor(); diff --git a/tests/core/proc/control/dispatcher-looper-test.cpp b/tests/core/proc/control/dispatcher-looper-test.cpp index a1d0e9eef..2b56621aa 100644 --- a/tests/core/proc/control/dispatcher-looper-test.cpp +++ b/tests/core/proc/control/dispatcher-looper-test.cpp @@ -167,8 +167,8 @@ namespace test { CHECK (looper.shallLoop()); setup.has_commands_in_queue = false; - looper.markStateProcessed(); - looper.markStateProcessed(); + looper.markStateProcessed(); // after command processing + looper.markStateProcessed(); // after builder run CHECK (not looper.requireAction()); CHECK (not looper.isWorking()); diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 8c9618f13..1333588e5 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -330,8 +330,7 @@ meint: zwei gekoppelte Statusvariable

- - + @@ -358,8 +357,7 @@ oder muß PImpl als Interface exponieren

- - + @@ -371,8 +369,7 @@ meint: zwei gekoppelte Statusvariable

- - + @@ -463,8 +460,7 @@ ist nicht "die Session

- - + @@ -487,8 +483,7 @@ das Lock sorgt hier für konsistenten Zustand und Sichtbarkeit (memory barrier)

- - + @@ -500,12 +495,23 @@ Lock ist hier das Dispatcher-Lock

- - + + + + + + + + + + + + + @@ -532,8 +538,7 @@ ...wenn jemand zugreift

- - + @@ -558,8 +563,7 @@ - - + @@ -586,8 +590,7 @@ bevor die Dispatcher-loop angehalten wird

- - + @@ -604,8 +607,7 @@ - - + @@ -638,8 +640,7 @@ wenn in der UI ein InvocationTrail angelegt wird.

- - + @@ -658,8 +659,7 @@ kann offen bleiben

- - + @@ -676,8 +676,7 @@ reine ID-Wirtschaft wäre möglich

- - + @@ -11177,7 +11176,11 @@ - + + + + + @@ -11186,7 +11189,11 @@ - + + + + + @@ -11230,8 +11237,7 @@ Guard beim Zugang über das Interface

- - + @@ -11243,8 +11249,7 @@ nur sie ist atomar

- - + @@ -11259,8 +11264,7 @@ nur ein Thread für Commands und Builder

- - + @@ -11300,8 +11304,7 @@ Ticket #1054

- - + @@ -11348,8 +11351,7 @@ OO rocks!

- - + @@ -11362,8 +11364,8 @@ - - + + @@ -11387,42 +11389,33 @@ - - + + + - - - - - - - -

- ...meinte ich, so etwas wie den Sequence-Point von C++ -

-

- also die Stelle, an der "die Zahnräder ineinander greifen" -

- - - + + + - - + + + + + + +

- es scheint, als würde ich eine Implementierung hinbekommen, -

-

- die (fast) nur auf logischen Zusammenhängen beruht + Kontrollfluß ist nicht in einer Arbeitsfunktion

- - + + + @@ -11443,8 +11436,7 @@ - - + @@ -11505,8 +11497,7 @@ billig: unsere Zeit-Lib nutzen

- - + @@ -11572,8 +11563,7 @@ generischer Überlegungen implementiert

- - + @@ -12053,8 +12043,7 @@ Ticket #886

- - +