Address comments

Author: coolreader18

crates/core/src/host/v8/mod.rs

@@ -6,7 +6,7 @@ use self::error::{
 use self::ser::serialize_to_js;
 use self::string::{str_from_ident, IntoJsString};
 use self::syscall::{
-    call_call_reducer, call_describe_module, get_hook, resolve_sys_module, FnRet, HookFunction, ModuleHook,
+    call_call_reducer, call_describe_module, get_hook, resolve_sys_module, FnRet, HookFunction, ModuleHookKey,
 };
 use super::module_common::{build_common_module_from_raw, run_describer, ModuleCommon};
 use super::module_host::{CallReducerParams, Module, ModuleInfo, ModuleRuntime};
@@ -331,7 +331,7 @@ fn startup_instance_worker<'scope>(
 
     // Find the `__call_reducer__` function.
     let call_reducer_fun =
-        get_hook(scope, ModuleHook::CallReducer).context("The `spacetimedb/server` module was never imported")?;
+        get_hook(scope, ModuleHookKey::CallReducer).context("The `spacetimedb/server` module was never imported")?;
 
     // If we don't have a module, make one.
     let module_common = match module_or_mcc {
@@ -652,7 +652,7 @@ fn extract_description<'scope>(
         |a, b, c| log_traceback(replica_ctx, a, b, c),
         || {
             catch_exception(scope, |scope| {
-                let Some(describe_module) = get_hook(scope, ModuleHook::DescribeModule) else {
+                let Some(describe_module) = get_hook(scope, ModuleHookKey::DescribeModule) else {
                     return Ok(RawModuleDef::V9(Default::default()));
                 };
                 let def = call_describe_module(scope, describe_module)?;
@@ -692,7 +692,7 @@ mod test {
     fn call_call_reducer_works() {
         let call = |code| {
             with_module_catch(code, |scope| {
-                let fun = get_hook(scope, ModuleHook::CallReducer).unwrap();
+                let fun = get_hook(scope, ModuleHookKey::CallReducer).unwrap();
                 let op = ReducerOp {
                     id: ReducerId(42),
                     name: "foobar",
@@ -771,7 +771,7 @@ js error Uncaught Error: foobar
             })
         "#;
         let raw_mod = with_module_catch(code, |scope| {
-            let describe_module = get_hook(scope, ModuleHook::DescribeModule).unwrap();
+            let describe_module = get_hook(scope, ModuleHookKey::DescribeModule).unwrap();
             call_describe_module(scope, describe_module)
         })
         .map_err(|e| e.to_string());

crates/core/src/host/v8/syscall/hooks.rs

@@ -28,67 +28,77 @@ pub(super) fn get_hook_function<'scope>(
 pub(super) fn set_hook_slots(
     scope: &mut PinScope<'_, '_>,
     abi: AbiVersion,
-    hooks: &[(ModuleHook, Local<'_, Function>)],
+    hooks: &[(ModuleHookKey, Local<'_, Function>)],
 ) -> ExcResult<()> {
     // Make sure to call `set_slot` first, as it creates the annex
     // and `set_embedder_data` is currently buggy.
     let ctx = scope.get_current_context();
-    let hooks_info = HooksInfo::get_or_create(&ctx);
+    let hooks_info = HooksInfo::get_or_create(&ctx, abi)
+        .map_err(|_| TypeError("cannot call `register_hooks` from different versions").throw(scope))?;
     for &(hook, func) in hooks {
         hooks_info
-            .register(hook, abi)
+            .register(hook)
             .map_err(|_| TypeError("cannot call `register_hooks` multiple times").throw(scope))?;
         ctx.set_embedder_data(hook.to_slot_index(), func.into());
     }
     Ok(())
 }
 
 #[derive(enum_map::Enum, Copy, Clone)]
-pub(in crate::host::v8) enum ModuleHook {
+pub(in super::super) enum ModuleHookKey {
     DescribeModule,
     CallReducer,
 }
 
-impl ModuleHook {
+impl ModuleHookKey {
     /// Returns the index for the slot that holds the module function hook.
     /// The index is passed to `v8::Context::{get,set}_embedder_data`.
     fn to_slot_index(self) -> i32 {
         match self {
-            ModuleHook::DescribeModule => 20,
-            ModuleHook::CallReducer => 21,
+            // high numbers to avoid overlapping with rusty_v8 - can be
+            // reverted to just 0, 1... once denoland/rusty_v8#1868 merges
+            ModuleHookKey::DescribeModule => 20,
+            ModuleHookKey::CallReducer => 21,
         }
     }
 }
 
 /// Holds the `AbiVersion` used by the module
 /// and the module hooks registered by the module
 /// for that version.
-#[derive(Default)]
 struct HooksInfo {
-    abi: OnceCell<AbiVersion>,
-    registered: EnumMap<ModuleHook, OnceCell<()>>,
+    abi: AbiVersion,
+    registered: EnumMap<ModuleHookKey, OnceCell<()>>,
 }
 
 impl HooksInfo {
     /// Returns, and possibly creates, the [`HooksInfo`] stored in `ctx`.
-    fn get_or_create(ctx: &Context) -> Rc<Self> {
-        ctx.get_slot().unwrap_or_else(|| {
-            let this = Rc::<Self>::default();
-            ctx.set_slot(this.clone());
-            this
-        })
+    ///
+    /// Returns an error if `abi` doesn't match the abi version in the
+    /// already existing `HooksInfo`.
+    fn get_or_create(ctx: &Context, abi: AbiVersion) -> Result<Rc<Self>, ()> {
+        match ctx.get_slot::<Self>() {
+            Some(this) if this.abi == abi => Ok(this),
+            Some(_) => Err(()),
+            None => {
+                let this = Rc::new(Self {
+                    abi,
+                    registered: EnumMap::default(),
+                });
+                ctx.set_slot(this.clone());
+                Ok(this)
+            }
+        }
     }
 
-    fn register(&self, hook: ModuleHook, abi: AbiVersion) -> Result<(), ()> {
-        if *self.abi.get_or_init(|| abi) != abi {
-            return Err(());
-        }
+    /// Mark down the given `hook` as registered, returning an error if it already was.
+    fn register(&self, hook: ModuleHookKey) -> Result<(), ()> {
         self.registered[hook].set(())
     }
 
     /// Returns the `AbiVersion` for the given `hook`, if any.
-    fn get(&self, hook: ModuleHook) -> Option<AbiVersion> {
-        self.registered[hook].get().and(self.abi.get().copied())
+    fn get(&self, hook: ModuleHookKey) -> Option<AbiVersion> {
+        self.registered[hook].get().map(|_| self.abi)
     }
 }
 
@@ -99,7 +109,7 @@ pub(in super::super) struct HookFunction<'scope>(pub AbiVersion, pub Local<'scop
 /// Returns the hook function previously registered in [`register_hooks`].
 pub(in super::super) fn get_hook<'scope>(
     scope: &mut PinScope<'scope, '_>,
-    hook: ModuleHook,
+    hook: ModuleHookKey,
 ) -> Option<HookFunction<'scope>> {
     let ctx = scope.get_current_context();
     let hooks = ctx.get_slot::<HooksInfo>()?;

crates/core/src/host/v8/syscall/mod.rs

@@ -1,16 +1,18 @@
 use spacetimedb_lib::RawModuleDef;
+use spacetimedb_lib::VersionTuple;
 use v8::{callback_scope, Context, FixedArray, Local, Module, PinScope};
 
 use crate::host::v8::de::scratch_buf;
 use crate::host::v8::error::ExcResult;
 use crate::host::v8::error::Throwable;
 use crate::host::v8::error::TypeError;
+use crate::host::wasm_common::abi::parse_abi_version;
 use crate::host::wasm_common::module_host_actor::{ReducerOp, ReducerResult};
 
 mod hooks;
 mod v1;
 
-pub(super) use self::hooks::{get_hook, HookFunction, ModuleHook};
+pub(super) use self::hooks::{get_hook, HookFunction, ModuleHookKey};
 
 /// The return type of a module -> host syscall.
 pub(super) type FnRet<'scope> = ExcResult<Local<'scope, v8::Value>>;
@@ -47,17 +49,15 @@ fn resolve_sys_module_inner<'scope>(
         .and_then(|spec| spec.split_once('@'))
         .ok_or_else(|| generic_error().throw(scope))?;
 
-    let (maj, min) = ver
-        .split_once('.')
-        .and_then(|(maj, min)| Option::zip(maj.parse::<u32>().ok(), min.parse::<u32>().ok()))
+    let VersionTuple { major, minor } = parse_abi_version(ver)
         .ok_or_else(|| TypeError(format!("Invalid version in module spec {spec:?}")).throw(scope))?;
 
     match module {
-        "sys" => match (maj, min) {
+        "sys" => match (major, minor) {
             (1, 0) => Ok(v1::sys_v1_0(scope)),
             _ => Err(TypeError(format!(
                 "Could not import {spec:?}, likely because this module was built for a newer version of SpacetimeDB.\n\
-                It requires sys module v{maj}.{min}, but that version is not supported by the database."
+                It requires sys module v{major}.{minor}, but that version is not supported by the database."
             ))
             .throw(scope)),
         },

crates/core/src/host/v8/syscall/v1.rs

@@ -1,5 +1,5 @@
 use super::hooks::{get_hook_function, set_hook_slots};
-use super::{AbiVersion, FnRet, ModuleHook};
+use super::{AbiVersion, FnRet, ModuleHookKey};
 use crate::database_logger::{LogLevel, Record};
 use crate::error::NodesError;
 use crate::host::instance_env::InstanceEnv;
@@ -323,8 +323,8 @@ fn register_hooks_v1_0<'scope>(scope: &mut PinScope<'scope, '_>, args: FunctionC
         scope,
         AbiVersion::V1,
         &[
-            (ModuleHook::DescribeModule, describe_module),
-            (ModuleHook::CallReducer, call_reducer),
+            (ModuleHookKey::DescribeModule, describe_module),
+            (ModuleHookKey::CallReducer, call_reducer),
         ],
     )?;
 

crates/core/src/host/wasm_common/abi.rs

@@ -8,19 +8,21 @@ pub fn determine_spacetime_abi<I>(
 ) -> Result<VersionTuple, AbiVersionError> {
     let it = imports.into_iter().filter_map(|imp| {
         let s = get_module(&imp);
-        let err = || AbiVersionError::Parse { module: s.to_owned() };
-        s.strip_prefix(MODULE_PREFIX).map(|ver| {
-            let (major, minor) = ver.split_once('.').ok_or_else(err)?;
-            let (major, minor) = Option::zip(major.parse().ok(), minor.parse().ok()).ok_or_else(err)?;
-            Ok(VersionTuple { major, minor })
-        })
+        s.strip_prefix(MODULE_PREFIX)
+            .map(|ver| parse_abi_version(ver).ok_or_else(|| AbiVersionError::Parse { module: s.to_owned() }))
     });
     itertools::process_results(it, |mut it| {
         let first = it.next().ok_or(AbiVersionError::NotDetected)?;
         it.try_fold(first, refine_ver_req)
     })?
 }
 
+pub fn parse_abi_version(ver: &str) -> Option<VersionTuple> {
+    let (major, minor) = ver.split_once('.')?;
+    let (major, minor) = Option::zip(major.parse().ok(), minor.parse().ok())?;
+    Some(VersionTuple { major, minor })
+}
+
 fn refine_ver_req(ver: VersionTuple, new: VersionTuple) -> Result<VersionTuple, AbiVersionError> {
     if ver.major != new.major {
         Err(AbiVersionError::MultipleMajor(ver.major, new.major))