fix(hermes): exc_counter was being mismanaged in a thread unsafe way. (#545)

* fix(hermes): exc_counter was being mismanaged in a thread unsafe way. This PR fixes that and cleans up redundant parameters in the process as the calls changed subtly to make the fix reliable.

* fix(hermes): benchmark build error

* fix(hermes): replace drop() with `if let Err(err) =`

Author: stevenj

hermes/bin/src/app.rs

@@ -5,11 +5,6 @@ use std::{collections::HashMap, sync::Arc};
 use crate::{
     event::HermesEventPayload,
     pool,
-    runtime_context::HermesRuntimeContext,
-    runtime_extensions::{
-        init::trait_event::{RteEvent, RteInitEvent},
-        new_context,
-    },
     vfs::Vfs,
     wasm::module::{Module, ModuleId},
 };
@@ -83,20 +78,14 @@ impl Application {
         event: &Arc<dyn HermesEventPayload>,
     ) {
         for module in self.indexed_modules.values() {
-            module_dispatch_event(
-                module.clone(),
-                self.name.clone(),
-                module.id().clone(),
-                self.vfs.clone(),
-                event.clone(),
-            );
+            module_dispatch_event(module.clone(), self.vfs.clone(), event.clone());
         }
     }
 
     /// Initialize every module.
     pub(crate) fn init(&self) -> anyhow::Result<()> {
         for module in self.indexed_modules.values() {
-            if let Err(e) = module.init(self.name.clone(), Arc::clone(&self.vfs)) {
+            if let Err(e) = module.init(self.vfs.clone()) {
                 anyhow::bail!("Failed to initialize module {}: {}", module.id(), e)
             }
         }
@@ -106,62 +95,28 @@ impl Application {
     /// Dispatch event for the target module by the `module_id`.
     pub(crate) fn dispatch_event_for_target_module(
         &self,
-        module_id: ModuleId,
+        module_id: &ModuleId,
         event: Arc<dyn HermesEventPayload>,
     ) -> anyhow::Result<()> {
         let module = self
             .indexed_modules
-            .get(&module_id)
+            .get(module_id)
             .ok_or(anyhow::anyhow!("Module {module_id} not found"))?;
-        module_dispatch_event(
-            module.clone(),
-            self.name.clone(),
-            module_id,
-            self.vfs.clone(),
-            event,
-        );
+        module_dispatch_event(module.clone(), self.vfs.clone(), event);
         Ok(())
     }
 }
 
 /// Dispatch event
 pub(crate) fn module_dispatch_event(
     module: Arc<Module>,
-    app_name: ApplicationName,
-    module_id: ModuleId,
     vfs: Arc<Vfs>,
     event: Arc<dyn HermesEventPayload>,
 ) {
     // TODO(@aido-mth): fix how init is processed. https://github.com/input-output-hk/hermes/issues/490
     pool::execute(move || {
-        let runtime_ctx = HermesRuntimeContext::new(
-            app_name,
-            module_id,
-            event.event_name().to_string(),
-            module.exec_counter(),
-            vfs,
-        );
-
-        // Advise Runtime Extensions of a new context
-        // TODO: Better handle errors.
-        if let Err(err) = RteEvent::new().init(&runtime_ctx) {
-            tracing::error!("module event initialization failed: {err}");
-            return;
-        }
-
-        // TODO: (SJ) Remove when all RTE's are migrated.
-        new_context(&runtime_ctx);
-
-        if let Err(err) = module.execute_event(event.as_ref(), runtime_ctx.clone()) {
+        if let Err(err) = module.execute_event(event.as_ref(), vfs) {
             tracing::error!("module event execution failed: {err}");
-            return;
         }
-
-        // Advise Runtime Extensions that context can be cleaned up.
-        drop(
-            RteEvent::new()
-                .fini(&runtime_ctx)
-                .inspect_err(|err| tracing::error!("module event finalization failed: {err}")),
-        );
     });
 }

hermes/bin/src/event/queue.rs

@@ -100,10 +100,9 @@ fn targeted_module_event_execution(
         },
         TargetModule::List(target_modules) => {
             for target_module_id in target_modules {
-                if let Err(err) = app.dispatch_event_for_target_module(
-                    target_module_id.clone(),
-                    event.payload().clone(),
-                ) {
+                if let Err(err) =
+                    app.dispatch_event_for_target_module(target_module_id, event.payload().clone())
+                {
                     tracing::error!("{err}");
                 }
             }

hermes/bin/src/runtime_extensions/hermes/integration_test/event.rs

@@ -118,16 +118,15 @@ pub fn execute_event(
     let vfs =
         Arc::new(VfsBootstrapper::new(hermes_home_dir.path(), app_name.to_string()).bootstrap()?);
 
-    let module_id = module.id().clone();
     let result = match event_type {
         EventType::Bench => {
             let on_bench_event = Arc::new(OnBenchEvent { test, run });
-            module_dispatch_event(module, app_name, module_id, vfs.clone(), on_bench_event);
+            module_dispatch_event(module, vfs.clone(), on_bench_event);
             BENCH_RESULT_QUEUE.get_or_init(SegQueue::new).pop()
         },
         EventType::Test => {
             let on_test_event = Arc::new(OnTestEvent { test, run });
-            module_dispatch_event(module, app_name, module_id, vfs.clone(), on_test_event);
+            module_dispatch_event(module, vfs.clone(), on_test_event);
             TEST_RESULT_QUEUE.get_or_init(SegQueue::new).pop()
         },
     };

hermes/bin/src/wasm/module.rs

@@ -28,7 +28,10 @@ use crate::{
     runtime_extensions::{
         bindings::{self, unchecked_exports, LinkOptions},
         hermes::init::ComponentInstanceExt as _,
-        init::trait_module::{RteInitModule, RteModule},
+        init::{
+            trait_event::{RteEvent, RteInitEvent},
+            trait_module::{RteInitModule, RteModule},
+        },
         new_context,
     },
     vfs::Vfs,
@@ -83,7 +86,13 @@ pub struct Module {
     id: ModuleId,
 
     /// Module's execution counter
+    /// CAN NEVER be used outside this structs methods.
+    /// NEVER add a getter for it.
+    /// ONLY ever use `fetch_add` never read its value otherwise.
     exc_counter: AtomicU32,
+
+    /// The name of the application which owns this module.
+    app_name: ApplicationName,
 }
 
 impl Module {
@@ -123,22 +132,33 @@ impl Module {
             engine,
             id,
             exc_counter: AtomicU32::new(0),
+            app_name: app_name.clone(),
         })
     }
 
+    /// Make a new context, deliberately private.  Do not make public.
+    fn new_context(
+        &self,
+        event_name: &str,
+        vfs: Arc<Vfs>,
+    ) -> HermesRuntimeContext {
+        HermesRuntimeContext::new(
+            self.app_name.clone(),
+            self.id.clone(),
+            event_name.to_string(),
+            // **MUST** be the only place exc_counter is read and updated.
+            // NEVER read it anywhere else, and never update it anywhere else.
+            self.exc_counter.fetch_add(1, Ordering::SeqCst),
+            vfs,
+        )
+    }
+
     /// Initializes the WASM module by calling its `init` function.
     pub(crate) fn init(
         &self,
-        app_name: ApplicationName,
         vfs: Arc<Vfs>,
     ) -> anyhow::Result<()> {
-        let runtime_ctx = HermesRuntimeContext::new(
-            app_name,
-            self.id.clone(),
-            "init_function_call".to_string(),
-            0,
-            vfs,
-        );
+        let runtime_ctx = self.new_context("init_function_call", vfs);
 
         new_context(&runtime_ctx);
 
@@ -180,15 +200,6 @@ impl Module {
         &self.id
     }
 
-    /// Get the module's execution counter
-    pub(crate) fn exec_counter(&self) -> u32 {
-        // Using the highest memory ordering constraint.
-        // It provides a highest consistency guarantee and in some cases could decrease
-        // performance.
-        // We could revise ordering approach for this case in future.
-        self.exc_counter.load(Ordering::SeqCst)
-    }
-
     /// Executes a Hermes event by calling some WASM function.
     /// This function abstraction over actual execution of the WASM function,
     /// actual definition is inside `HermesEventPayload` trait implementation.
@@ -201,9 +212,21 @@ impl Module {
     pub(crate) fn execute_event(
         &self,
         event: &dyn HermesEventPayload,
-        state: HermesRuntimeContext,
+        vfs: Arc<Vfs>,
     ) -> anyhow::Result<()> {
-        let mut store = WasmStore::new(&self.engine, state);
+        let runtime_ctx = self.new_context(event.event_name(), vfs);
+
+        // Advise Runtime Extensions of a new context
+        // TODO: Better handle errors.
+        if let Err(err) = RteEvent::new().init(&runtime_ctx) {
+            tracing::error!("module event initialization failed: {err}");
+            return Err(err.into());
+        }
+
+        // TODO: (SJ) Remove when all RTE's are migrated.
+        new_context(&runtime_ctx);
+
+        let mut store = WasmStore::new(&self.engine, runtime_ctx.clone());
         let instance = self
             .pre_instance
             .clone()
@@ -212,11 +235,12 @@ impl Module {
 
         event.execute(&mut ModuleInstance { store, instance })?;
 
-        // Using the highest memory ordering constraint.
-        // It provides a highest consistency guarantee and in some cases could decrease
-        // performance.
-        // We could revise ordering approach for this case in future.
-        self.exc_counter.fetch_add(1, Ordering::SeqCst);
+        // Advise Runtime Extensions that context can be cleaned up.
+        if let Err(err) = RteEvent::new().fini(&runtime_ctx) {
+            //TODO(SJ): Maybe need better error handling...
+            tracing::error!("module event finalization failed: {err}");
+        }
+
         Ok(())
     }
 }
@@ -247,7 +271,7 @@ pub mod bench {
         );
 
         b.iter(|| {
-            module.init(app_name.clone(), vfs.clone()).unwrap();
+            module.init(vfs.clone()).unwrap();
         });
     }