From f6b9648dd61cf1a2d079d7a41b0980f14023a512 Mon Sep 17 00:00:00 2001 From: Matej Pavlovic Date: Sat, 28 May 2022 11:15:09 +0200 Subject: [PATCH 1/4] Write proposal for module generalization Signed-off-by: Matej Pavlovic --- .../0003-generalize-modules.md | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 docs/architecture-decision-records/0003-generalize-modules.md diff --git a/docs/architecture-decision-records/0003-generalize-modules.md b/docs/architecture-decision-records/0003-generalize-modules.md new file mode 100644 index 000000000..f09b4cd46 --- /dev/null +++ b/docs/architecture-decision-records/0003-generalize-modules.md @@ -0,0 +1,112 @@ +# Generalize Modules + +* Status: proposed +* Deciders: @matejpavlovic, @sergefdrv, @dnkolegov +* Date: 2022-05-28 (last update) + +## Context and Problem Statement + +The current architecture defines several kinds of modules, each with a module-specific interface. +There is also exactly one of each kind of module +(i.e., one `Protocol` module instance, one `App`, one `ClientTracker`, etc.). +The event handling code then takes each module and wraps it almost the same simple interface +(consuming input events and producing output events) using a module-specific wrapper. +This wrapper almost always ends up being just a simple de-multiplexer that, +based on the incoming event type, calls the appropriate module-specific method of the module. + +This approach: +- Pollutes the Node's event handling logic by the above-mentioned wrappers (which can be considered boilerplate code). + Whenever a new type of module is being defined, the core Mir code needs to be modified + to add the corresponding wrapper. +- Unnecessarily constrains the user of Mir to use exactly one of each type of module, + even if a protocol might naturally be split up in several modules working in parallel. +- The selection of the types of modules supported is tailored to a SMR application, but not necessarily to other uses. + +## Considered Options + +* **Active and passive modules.** + Generalize the concept of modules, such that: + - Modules have a unified interface for processing events, + where each module internally inspects the incoming events and decides on the appropriate action. + The `Protocol` and some other modules already do have such a unified interface - use it for other modules as well. + - Instead of dedicated slots for each type of module, use a variable number of generic modules of only 2 types: + - `PassiveModule`: + A module that simply transforms input events to output events whenever invoked by Mir. + Never injects external events in the system. + A passive module has the following interface (corresponding to some existing modules, like `Protocol`): + ```go + type PassiveModule interface { + + // ApplyEvent applies a single input event to the module, making it advance its state + // and return a (potentially empty) list of output events that the application of the input event results in. + ApplyEvent(event *eventpb.Event) *events.EventList + + // Status returns the current state of the module. + // Mostly for debugging purposes. + Status() (s Status, err error) // The Status type is a placeholder not intended to be defined by this ADR. + } + ``` + - `ActiveModule`: + Like the `PassiveModule`, the `ActiveModule` consumes input events. However, + unlike the `PassiveModule`, the `ActiveModule` does not synchronously return receive input + and return output in a function call. + Instead, the `ActiveModule` asynchronously reads input events from a channel + and writes produced events to an output channel. + In an `ActiveModule`, the produced events might or might not depend on the input events. + This makes the `ActiveModule` suitable for injecting external events + (e.g., reception of a message over the network) in the Node. + To prevent the module from flooding the Node with too many external events, + the node might ask it to temporarily back off and function essentially as a `PassiveModule`. + ```go + type ActiveModule interface { + + // Run performs the module's processing. + // It usually reads events from eventsIn, processes them, and writes new events to eventsOut. + // Note, however, that the implementation may produce output events even without receiving any input. + // Before Run is called, ActiveModule guarantees not to read or write, + // respectively, from eventsIn and to eventsOut. + // Run must return after ctx is canceled. + Run(ctx context.Context, eventsIn <-chan *events.EventList, eventsOut chan<- *events.EventList) error + + // BackOff asks the module to back off, i.e., to not produce new events + // except in immediate reaction to input events, until ctx is canceled. + // After BackOff is called and before ctx is canceled, + // the module will only produce a bounded number of output events and only in reaction to processing input events. + // In other words, if BackOff has been called, ctx has not been canceled, and the input channel is not written to, + // the module will eventually stop producing output events. + // + // This function is necessary for guaranteeing that the internal Node event buffers do not grow indefinitely. + // This could potentially happen when external events are injected into the system through active modules + // faster than the system can process them. + // In such a case, the Node asks all active modules to back off + // until enough events in the internal buffers have been processed. + BackOff(ctx context.Context) + + // Status returns the current state of the module. + // If Run has been called and has not returned when calling Status, + // there is no guarantee about which input events are taken into account + // when creating the snapshot of the returned state. + // Mostly for debugging purposes. + Status() (s *statuspb.ProtocolStatus, err error) + } + ``` + Note that this approach still requires the user of Mir to define the module-specific events in Mir, + as well as the way they are dispatched. + Generalizing event dispatching is also desirable, but can be treated as a separate issue in a different ADR. +## Decision Outcome + +Chosen option: Active and passive modules, because it addresses the issue well, +can easily be implemented, and is the only proposed one. +Moreover, almost as a side effect, it explicitly addresses +the concern of growing event buffers in the Node's event loop. + +### Positive Consequences + +* Makes Mir's code more uniform, simpler, and cleaner. +* Increases the expressiveness of mir by giving the user significantly more flexibility in how to compose the modules. +* Does not impact the internal logic of the existing modules + (only the way they are attached to the node needs to be modified) + +### Negative Consequences + +* Requires some implementation overhead. From b2c9f36a1a7e6c1a35b0fdcd67ddd3a603c6baef Mon Sep 17 00:00:00 2001 From: Matej Pavlovic Date: Tue, 31 May 2022 10:55:14 +0200 Subject: [PATCH 2/4] Remove BackOff function from ActiveModule Instead, note that the Node might simply stop reading the ActiveModule's output. Signed-off-by: Matej Pavlovic --- .../0003-generalize-modules.md | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/docs/architecture-decision-records/0003-generalize-modules.md b/docs/architecture-decision-records/0003-generalize-modules.md index f09b4cd46..f2a6e6745 100644 --- a/docs/architecture-decision-records/0003-generalize-modules.md +++ b/docs/architecture-decision-records/0003-generalize-modules.md @@ -56,7 +56,10 @@ This approach: This makes the `ActiveModule` suitable for injecting external events (e.g., reception of a message over the network) in the Node. To prevent the module from flooding the Node with too many external events, - the node might ask it to temporarily back off and function essentially as a `PassiveModule`. + the node might, at any time, stop processing the `ActiveModule`s output events + for an arbitrarily long time period. + This is expected to happen when internal node buffers become full of unprocessed events + and the node needs to wait until they free up. ```go type ActiveModule interface { @@ -66,22 +69,17 @@ This approach: // Before Run is called, ActiveModule guarantees not to read or write, // respectively, from eventsIn and to eventsOut. // Run must return after ctx is canceled. - Run(ctx context.Context, eventsIn <-chan *events.EventList, eventsOut chan<- *events.EventList) error - - // BackOff asks the module to back off, i.e., to not produce new events - // except in immediate reaction to input events, until ctx is canceled. - // After BackOff is called and before ctx is canceled, - // the module will only produce a bounded number of output events and only in reaction to processing input events. - // In other words, if BackOff has been called, ctx has not been canceled, and the input channel is not written to, - // the module will eventually stop producing output events. // - // This function is necessary for guaranteeing that the internal Node event buffers do not grow indefinitely. - // This could potentially happen when external events are injected into the system through active modules - // faster than the system can process them. - // In such a case, the Node asks all active modules to back off - // until enough events in the internal buffers have been processed. - BackOff(ctx context.Context) - + // Note that the node does not guarantee to always read events from eventsOut. + // The node might decide at any moment to stop reading from eventsOut for an arbitrary amount of time + // (e.g. if the Node's internal event buffers become full and the Node needs to wait until they free up). + // + // The implementation of Run is required to always read from eventsIn. + // I.e., when an event is written to eventsIn, Run must eventually read it, regardless of the module's state + // or any external factors + // (e.g., the processing of events must not depend on some other goroutine reading from eventsOut). + Run(ctx context.Context, eventsIn <-chan *events.EventList, eventsOut chan<- *events.EventList) error + // Status returns the current state of the module. // If Run has been called and has not returned when calling Status, // there is no guarantee about which input events are taken into account From e900c992b45f57aaa86f6022a4d65919f33725ec Mon Sep 17 00:00:00 2001 From: Matej Pavlovic Date: Tue, 31 May 2022 10:59:38 +0200 Subject: [PATCH 3/4] Add comment on the blocking behavior of Run Signed-off-by: Matej Pavlovic --- .../0003-generalize-modules.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/architecture-decision-records/0003-generalize-modules.md b/docs/architecture-decision-records/0003-generalize-modules.md index f2a6e6745..8a87d961a 100644 --- a/docs/architecture-decision-records/0003-generalize-modules.md +++ b/docs/architecture-decision-records/0003-generalize-modules.md @@ -2,7 +2,7 @@ * Status: proposed * Deciders: @matejpavlovic, @sergefdrv, @dnkolegov -* Date: 2022-05-28 (last update) +* Date: 2022-05-31 (last update) ## Context and Problem Statement @@ -68,11 +68,15 @@ This approach: // Note, however, that the implementation may produce output events even without receiving any input. // Before Run is called, ActiveModule guarantees not to read or write, // respectively, from eventsIn and to eventsOut. + // + // In general, Run is expected to block until explicitly asked to stop + // (through a module-specific event or by canceling ctx) + // and thus should most likely be run in its own goroutine. // Run must return after ctx is canceled. // // Note that the node does not guarantee to always read events from eventsOut. // The node might decide at any moment to stop reading from eventsOut for an arbitrary amount of time - // (e.g. if the Node's internal event buffers become full and the Node needs to wait until they free up). + // (e.g. if the Node's internal event buffers become full and the Node needs to wait until they free up). // // The implementation of Run is required to always read from eventsIn. // I.e., when an event is written to eventsIn, Run must eventually read it, regardless of the module's state From 7de68da4bf2e3a5a5f7f1cfc008da7cf28b99d3d Mon Sep 17 00:00:00 2001 From: Matej Pavlovic Date: Tue, 31 May 2022 23:21:38 +0200 Subject: [PATCH 4/4] Update comments and explanations in ADR 0003 Signed-off-by: Matej Pavlovic --- .../0003-generalize-modules.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/docs/architecture-decision-records/0003-generalize-modules.md b/docs/architecture-decision-records/0003-generalize-modules.md index 8a87d961a..34509ea50 100644 --- a/docs/architecture-decision-records/0003-generalize-modules.md +++ b/docs/architecture-decision-records/0003-generalize-modules.md @@ -24,14 +24,14 @@ This approach: ## Considered Options -* **Active and passive modules.** +* **Active and passive module types.** Generalize the concept of modules, such that: - Modules have a unified interface for processing events, where each module internally inspects the incoming events and decides on the appropriate action. The `Protocol` and some other modules already do have such a unified interface - use it for other modules as well. - Instead of dedicated slots for each type of module, use a variable number of generic modules of only 2 types: - `PassiveModule`: - A module that simply transforms input events to output events whenever invoked by Mir. + A module that simply defines the logic for transforming input events to output events whenever invoked by Mir. Never injects external events in the system. A passive module has the following interface (corresponding to some existing modules, like `Protocol`): ```go @@ -47,11 +47,9 @@ This approach: } ``` - `ActiveModule`: - Like the `PassiveModule`, the `ActiveModule` consumes input events. However, - unlike the `PassiveModule`, the `ActiveModule` does not synchronously return receive input - and return output in a function call. - Instead, the `ActiveModule` asynchronously reads input events from a channel + The `ActiveModule` asynchronously reads input events from a channel and writes produced events to an output channel. + In an `ActiveModule`, the produced events might or might not depend on the input events. This makes the `ActiveModule` suitable for injecting external events (e.g., reception of a message over the network) in the Node.