Wire up error handling to isolate2 (#24731)

GitOrigin-RevId: 7214af8b4ad6faf6c204feec96b16d1009052763

Author: Sujay Jayakar

crates/isolate/src/environment/helpers/mod.rs

@@ -9,7 +9,6 @@ mod syscall_trace;
 pub mod validation;
 mod version;
 
-use common::runtime::Runtime;
 use deno_core::{
     serde_v8,
     v8,
@@ -28,10 +27,6 @@ pub use self::{
     syscall_trace::SyscallTrace,
     version::parse_version,
 };
-use crate::{
-    environment::IsolateEnvironment,
-    execution_scope::ExecutionScope,
-};
 
 pub const MAX_LOG_LINE_LENGTH: usize = 32768;
 pub const MAX_LOG_LINES: usize = 256;
@@ -65,8 +60,8 @@ pub enum Phase {
     Executing,
 }
 
-pub fn json_to_v8<'a, 'b: 'a, RT: Runtime, E: IsolateEnvironment<RT>>(
-    scope: &mut ExecutionScope<'a, 'b, RT, E>,
+pub fn json_to_v8<'a>(
+    scope: &mut v8::HandleScope<'a>,
     json: JsonValue,
 ) -> anyhow::Result<v8::Local<'a, v8::Value>> {
     let value_v8 = serde_v8::to_v8(scope, json)?;

crates/isolate/src/environment/helpers/promise.rs

@@ -1,24 +1,17 @@
 use anyhow::anyhow;
-use common::{
-    errors::{
-        report_error,
-        JsError,
-    },
-    runtime::Runtime,
+use common::errors::{
+    report_error,
+    JsError,
 };
 use deno_core::v8;
 use errors::ErrorMetadataAnyhowExt;
 use serde_json::Value as JsonValue;
 
 use super::json_to_v8;
-use crate::{
-    environment::IsolateEnvironment,
-    execution_scope::ExecutionScope,
-    strings,
-};
+use crate::strings;
 
-pub fn resolve_promise<RT: Runtime, E: IsolateEnvironment<RT>>(
-    scope: &mut ExecutionScope<RT, E>,
+pub fn resolve_promise(
+    scope: &mut v8::HandleScope<'_>,
     resolver: v8::Global<v8::PromiseResolver>,
     result: anyhow::Result<v8::Local<v8::Value>>,
 ) -> anyhow::Result<()> {
@@ -27,16 +20,16 @@ pub fn resolve_promise<RT: Runtime, E: IsolateEnvironment<RT>>(
 
 // Like `resolve_promise` but returns JS error even when the
 // error might have been caused by Convex, not by the user.
-pub fn resolve_promise_allow_all_errors<RT: Runtime, E: IsolateEnvironment<RT>>(
-    scope: &mut ExecutionScope<RT, E>,
+pub fn resolve_promise_allow_all_errors(
+    scope: &mut v8::HandleScope<'_>,
     resolver: v8::Global<v8::PromiseResolver>,
     result: anyhow::Result<v8::Local<v8::Value>>,
 ) -> anyhow::Result<()> {
     resolve_promise_inner(scope, resolver, result, true)
 }
 
-fn resolve_promise_inner<RT: Runtime, E: IsolateEnvironment<RT>>(
-    scope: &mut ExecutionScope<RT, E>,
+fn resolve_promise_inner(
+    scope: &mut v8::HandleScope<'_>,
     resolver: v8::Global<v8::PromiseResolver>,
     result: anyhow::Result<v8::Local<v8::Value>>,
     allow_all_errors: bool,

crates/isolate/src/error.rs

@@ -10,6 +10,7 @@ use deno_core::{
     ModuleSpecifier,
 };
 use sourcemap::SourceMap;
+use value::ConvexValue;
 
 use crate::{
     environment::IsolateEnvironment,
@@ -43,6 +44,16 @@ impl<'a, 'b, RT: Runtime, E: IsolateEnvironment<RT>> ExecutionScope<'a, 'b, RT,
         Ok(error)
     }
 
+    fn extract_source_mapped_error(
+        &mut self,
+        exception: v8::Local<v8::Value>,
+    ) -> anyhow::Result<JsError> {
+        let (message, frame_data, custom_data) = extract_source_mapped_error(self, exception)?;
+        JsError::from_frames(message, frame_data, custom_data, |s| {
+            self.lookup_source_map(s)
+        })
+    }
+
     pub fn lookup_source_map(
         &mut self,
         specifier: &ModuleSpecifier,
@@ -56,64 +67,59 @@ impl<'a, 'b, RT: Runtime, E: IsolateEnvironment<RT>> ExecutionScope<'a, 'b, RT,
         };
         Ok(Some(SourceMap::from_slice(source_map.as_bytes())?))
     }
+}
 
-    fn extract_source_mapped_error(
-        &mut self,
-        exception: v8::Local<v8::Value>,
-    ) -> anyhow::Result<JsError> {
-        if !(is_instance_of_error(self, exception)) {
-            anyhow::bail!("Exception wasn't an instance of `Error`");
-        }
-        let exception_obj: v8::Local<v8::Object> = exception.try_into()?;
-
-        // Get the message by formatting error.name and error.message.
-        let name = get_property(self, exception_obj, "name")?
-            .filter(|v| !v.is_undefined())
-            .and_then(|m| m.to_string(self))
-            .map(|s| s.to_rust_string_lossy(self))
-            .unwrap_or_else(|| "Error".to_string());
-        let message_prop = get_property(self, exception_obj, "message")?
-            .filter(|v| !v.is_undefined())
-            .and_then(|m| m.to_string(self))
-            .map(|s| s.to_rust_string_lossy(self))
-            .unwrap_or_else(|| "".to_string());
-        let message = format_uncaught_error(message_prop, name);
+pub fn extract_source_mapped_error(
+    scope: &mut v8::HandleScope<'_>,
+    exception: v8::Local<v8::Value>,
+) -> anyhow::Result<(String, Vec<FrameData>, Option<ConvexValue>)> {
+    if !(is_instance_of_error(scope, exception)) {
+        anyhow::bail!("Exception wasn't an instance of `Error`");
+    }
+    let exception_obj: v8::Local<v8::Object> = exception.try_into()?;
 
-        // Access the `stack` property to ensure `prepareStackTrace` has been called.
-        // NOTE if this is the first time accessing `stack`, it will call the op
-        // `error/stack` which does a redundant source map lookup.
-        let _stack: v8::Local<v8::String> = get_property(self, exception_obj, "stack")?
-            .ok_or_else(|| anyhow::anyhow!("Exception was missing the `stack` property"))?
-            .try_into()?;
+    // Get the message by formatting error.name and error.message.
+    let name = get_property(scope, exception_obj, "name")?
+        .filter(|v| !v.is_undefined())
+        .and_then(|m| m.to_string(scope))
+        .map(|s| s.to_rust_string_lossy(scope))
+        .unwrap_or_else(|| "Error".to_string());
+    let message_prop = get_property(scope, exception_obj, "message")?
+        .filter(|v| !v.is_undefined())
+        .and_then(|m| m.to_string(scope))
+        .map(|s| s.to_rust_string_lossy(scope))
+        .unwrap_or_else(|| "".to_string());
+    let message = format_uncaught_error(message_prop, name);
 
-        let frame_data: v8::Local<v8::String> = get_property(self, exception_obj, "__frameData")?
-            .ok_or_else(|| anyhow::anyhow!("Exception was missing the `__frameData` property"))?
-            .try_into()?;
-        let frame_data = to_rust_string(self, &frame_data)?;
-        let frame_data: Vec<FrameData> = serde_json::from_str(&frame_data)?;
+    // Access the `stack` property to ensure `prepareStackTrace` has been called.
+    // NOTE if this is the first time accessing `stack`, it will call the op
+    // `error/stack` which does a redundant source map lookup.
+    let _stack: v8::Local<v8::String> = get_property(scope, exception_obj, "stack")?
+        .ok_or_else(|| anyhow::anyhow!("Exception was missing the `stack` property"))?
+        .try_into()?;
 
-        // error[error.ConvexErrorSymbol] === true
-        let convex_error_symbol = get_property(self, exception_obj, "ConvexErrorSymbol")?;
-        let is_convex_error = convex_error_symbol.map_or(false, |symbol| {
-            exception_obj
-                .get(self, symbol)
-                .map_or(false, |v| v.is_true())
-        });
+    let frame_data: v8::Local<v8::String> = get_property(scope, exception_obj, "__frameData")?
+        .ok_or_else(|| anyhow::anyhow!("Exception was missing the `__frameData` property"))?
+        .try_into()?;
+    let frame_data = to_rust_string(scope, &frame_data)?;
+    let frame_data: Vec<FrameData> = serde_json::from_str(&frame_data)?;
 
-        let custom_data = if is_convex_error {
-            let custom_data: v8::Local<v8::String> = get_property(self, exception_obj, "data")?
-                .ok_or_else(|| {
-                    anyhow::anyhow!("The thrown ConvexError is missing `data` property")
-                })?
-                .try_into()?;
-            Some(to_rust_string(self, &custom_data)?)
-        } else {
-            None
-        };
-        let (message, custom_data) = deserialize_udf_custom_error(message, custom_data)?;
+    // error[error.ConvexErrorSymbol] === true
+    let convex_error_symbol = get_property(scope, exception_obj, "ConvexErrorSymbol")?;
+    let is_convex_error = convex_error_symbol.map_or(false, |symbol| {
+        exception_obj
+            .get(scope, symbol)
+            .map_or(false, |v| v.is_true())
+    });
 
-        JsError::from_frames(message, frame_data, custom_data, |s| {
-            self.lookup_source_map(s)
-        })
-    }
+    let custom_data = if is_convex_error {
+        let custom_data: v8::Local<v8::String> = get_property(scope, exception_obj, "data")?
+            .ok_or_else(|| anyhow::anyhow!("The thrown ConvexError is missing `data` property"))?
+            .try_into()?;
+        Some(to_rust_string(scope, &custom_data)?)
+    } else {
+        None
+    };
+    let (message, custom_data) = deserialize_udf_custom_error(message, custom_data)?;
+    Ok((message, frame_data, custom_data))
 }

crates/isolate/src/execution_scope.rs

@@ -19,7 +19,6 @@ use common::{
     types::UdfType,
 };
 use deno_core::{
-    serde_v8,
     v8::{
         self,
         HeapStatistics,
@@ -374,33 +373,6 @@ impl<'a, 'b: 'a, RT: Runtime, E: IsolateEnvironment<RT>> ExecutionScope<'a, 'b,
         Ok(id)
     }
 
-    pub fn start_dynamic_import<'s>(
-        &mut self,
-        resource_name: v8::Local<'s, v8::Value>,
-        specifier: v8::Local<'s, v8::String>,
-    ) -> anyhow::Result<v8::Local<'_, v8::Promise>> {
-        let promise_resolver = v8::PromiseResolver::new(self)
-            .ok_or_else(|| anyhow::anyhow!("Failed to create v8::PromiseResolver"))?;
-
-        let promise = promise_resolver.get_promise(self);
-        let resolver = v8::Global::new(self, promise_resolver);
-
-        let referrer_name: String = serde_v8::from_v8(self, resource_name)?;
-        let specifier_str = helpers::to_rust_string(self, &specifier)?;
-
-        let resolved_specifier = deno_core::resolve_import(&specifier_str, &referrer_name)
-            .map_err(|e| ErrorMetadata::bad_request("InvalidImport", e.to_string()))?;
-
-        let dynamic_imports = self.pending_dynamic_imports_mut();
-        anyhow::ensure!(
-            dynamic_imports.allow_dynamic_imports,
-            "dynamic_import_callback registered without allow_dynamic_imports?"
-        );
-        dynamic_imports.push(resolved_specifier, resolver);
-
-        Ok(promise)
-    }
-
     async fn lookup_source(
         &mut self,
         module_specifier: &ModuleSpecifier,

crates/isolate/src/isolate2/client.rs

@@ -7,7 +7,6 @@ use std::{
 };
 
 use common::{
-    errors::JsError,
     runtime::Runtime,
     types::UdfType,
 };
@@ -36,38 +35,41 @@ pub enum IsolateThreadRequest {
     RegisterModule {
         name: ModuleSpecifier,
         source: String,
-        response: oneshot::Sender<Vec<ModuleSpecifier>>,
+        source_map: Option<String>,
+        response: oneshot::Sender<anyhow::Result<Vec<ModuleSpecifier>>>,
     },
     EvaluateModule {
         name: ModuleSpecifier,
-        // XXX: how do we want to pipe through JS errors across threads?
-        response: oneshot::Sender<()>,
+        response: oneshot::Sender<anyhow::Result<()>>,
     },
     StartFunction {
         udf_type: UdfType,
         module: ModuleSpecifier,
         name: String,
         args: ConvexObject,
-        response: oneshot::Sender<(FunctionId, EvaluateResult)>,
+        response: oneshot::Sender<anyhow::Result<(FunctionId, EvaluateResult)>>,
     },
     PollFunction {
         function_id: FunctionId,
         completions: Vec<AsyncSyscallCompletion>,
-        response: oneshot::Sender<EvaluateResult>,
+        response: oneshot::Sender<anyhow::Result<EvaluateResult>>,
     },
 }
 
 #[derive(Debug)]
 pub enum EvaluateResult {
-    Ready {
-        result: ConvexValue,
-        outcome: EnvironmentOutcome,
-    },
+    Ready(ReadyEvaluateResult),
     Pending {
         async_syscalls: Vec<PendingAsyncSyscall>,
     },
 }
 
+#[derive(Debug)]
+pub struct ReadyEvaluateResult {
+    pub result: ConvexValue,
+    pub outcome: EnvironmentOutcome,
+}
+
 #[derive(Debug)]
 pub struct PendingAsyncSyscall {
     pub promise_id: PromiseId,
@@ -77,7 +79,7 @@ pub struct PendingAsyncSyscall {
 
 pub struct AsyncSyscallCompletion {
     pub promise_id: PromiseId,
-    pub result: Result<JsonValue, JsError>,
+    pub result: anyhow::Result<String>,
 }
 
 pub struct IsolateThreadClient<RT: Runtime> {
@@ -105,7 +107,7 @@ impl<RT: Runtime> IsolateThreadClient<RT> {
     pub async fn send<T>(
         &mut self,
         request: IsolateThreadRequest,
-        mut rx: oneshot::Receiver<T>,
+        mut rx: oneshot::Receiver<anyhow::Result<T>>,
     ) -> anyhow::Result<T> {
         if self.user_time_remaining.is_zero() {
             anyhow::bail!("User time exhausted");
@@ -136,19 +138,21 @@ impl<RT: Runtime> IsolateThreadClient<RT> {
         // Tokio thread to talk to its V8 thread.
         drop(permit);
 
-        Ok(result?)
+        result?
     }
 
     pub async fn register_module(
         &mut self,
         name: ModuleSpecifier,
         source: String,
+        source_map: Option<String>,
     ) -> anyhow::Result<Vec<ModuleSpecifier>> {
         let (tx, rx) = oneshot::channel();
         self.send(
             IsolateThreadRequest::RegisterModule {
                 name,
                 source,
+                source_map,
                 response: tx,
             },
             rx,