refactor(ops): return BadResource errors in ResourceTable calls (#11710)

* refactor(ops): return BadResource errors in ResourceTable calls

Instead of relying on callers to map Options to Results via `.ok_or_else(bad_resource_id)` at over 176 different call sites ...

4 files changed, 38 insertions, 41 deletions

diff --git a/core/bindings.rs b/core/bindings.rs
index 935397480..9bfe767c3 100644
--- a/core/bindings.rs
+++ b/core/bindings.rs
@@ -611,13 +611,14 @@ fn wasm_streaming_feed(
     let state = state_rc.borrow();
     // If message_type is not Bytes, we'll be consuming the WasmStreaming
     // instance, so let's also remove it from the resource table.
-    let wasm_streaming: Option<Rc<WasmStreamingResource>> = match message_type {
-      MessageType::Bytes => state.op_state.borrow().resource_table.get(rid),
-      _ => state.op_state.borrow_mut().resource_table.take(rid),
-    };
+    let wasm_streaming: Result<Rc<WasmStreamingResource>, AnyError> =
+      match message_type {
+        MessageType::Bytes => state.op_state.borrow().resource_table.get(rid),
+        _ => state.op_state.borrow_mut().resource_table.take(rid),
+      };
     match wasm_streaming {
-      Some(wasm_streaming) => wasm_streaming,
-      None => return throw_type_error(scope, "Invalid resource ID."),
+      Ok(wasm_streaming) => wasm_streaming,
+      Err(e) => return throw_type_error(scope, e.to_string()),
     }
   };
 
diff --git a/core/examples/http_bench_json_ops.rs b/core/examples/http_bench_json_ops.rs
index 82eb18d6b..d273c2c88 100644
--- a/core/examples/http_bench_json_ops.rs
+++ b/core/examples/http_bench_json_ops.rs
@@ -1,5 +1,4 @@
 // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.
-use deno_core::error::bad_resource_id;
 use deno_core::error::null_opbuf;
 use deno_core::error::AnyError;
 use deno_core::AsyncRefCell;
@@ -140,11 +139,7 @@ fn op_close(
   _: (),
 ) -> Result<(), AnyError> {
   log::debug!("close rid={}", rid);
-  state
-    .resource_table
-    .close(rid)
-    .map(|_| ())
-    .ok_or_else(bad_resource_id)
+  state.resource_table.close(rid).map(|_| ())
 }
 
 async fn op_accept(
@@ -154,11 +149,7 @@ async fn op_accept(
 ) -> Result<ResourceId, AnyError> {
   log::debug!("accept rid={}", rid);
 
-  let listener = state
-    .borrow()
-    .resource_table
-    .get::<TcpListener>(rid)
-    .ok_or_else(bad_resource_id)?;
+  let listener = state.borrow().resource_table.get::<TcpListener>(rid)?;
   let stream = listener.accept().await?;
   let rid = state.borrow_mut().resource_table.add(stream);
   Ok(rid)
@@ -172,11 +163,7 @@ async fn op_read(
   let mut buf = buf.ok_or_else(null_opbuf)?;
   log::debug!("read rid={}", rid);
 
-  let stream = state
-    .borrow()
-    .resource_table
-    .get::<TcpStream>(rid)
-    .ok_or_else(bad_resource_id)?;
+  let stream = state.borrow().resource_table.get::<TcpStream>(rid)?;
   let nread = stream.read(&mut buf).await?;
   Ok(nread)
 }
@@ -189,11 +176,7 @@ async fn op_write(
   let buf = buf.ok_or_else(null_opbuf)?;
   log::debug!("write rid={}", rid);
 
-  let stream = state
-    .borrow()
-    .resource_table
-    .get::<TcpStream>(rid)
-    .ok_or_else(bad_resource_id)?;
+  let stream = state.borrow().resource_table.get::<TcpStream>(rid)?;
   let nwritten = stream.write(&buf).await?;
   Ok(nwritten)
 }
diff --git a/core/ops_builtin.rs b/core/ops_builtin.rs
index b00bb9c28..459b2a967 100644
--- a/core/ops_builtin.rs
+++ b/core/ops_builtin.rs
@@ -1,4 +1,3 @@
-use crate::error::bad_resource_id;
 use crate::error::type_error;
 use crate::error::AnyError;
 use crate::include_js_files;
@@ -47,10 +46,7 @@ pub fn op_close(
 ) -> Result<(), AnyError> {
   // TODO(@AaronO): drop Option after improving type-strictness balance in serde_v8
   let rid = rid.ok_or_else(|| type_error("missing or invalid `rid`"))?;
-  state
-    .resource_table
-    .close(rid)
-    .ok_or_else(bad_resource_id)?;
+  state.resource_table.close(rid)?;
 
   Ok(())
 }
diff --git a/core/resources.rs b/core/resources.rs
index 37f2b8edd..c5e6684a4 100644
--- a/core/resources.rs
+++ b/core/resources.rs
@@ -6,6 +6,8 @@
 // resources. Resources may or may not correspond to a real operating system
 // file descriptor (hence the different name).
 
+use crate::error::bad_resource_id;
+use crate::error::AnyError;
 use std::any::type_name;
 use std::any::Any;
 use std::any::TypeId;
@@ -105,32 +107,43 @@ impl ResourceTable {
   /// Returns a reference counted pointer to the resource of type `T` with the
   /// given `rid`. If `rid` is not present or has a type different than `T`,
   /// this function returns `None`.
-  pub fn get<T: Resource>(&self, rid: ResourceId) -> Option<Rc<T>> {
+  pub fn get<T: Resource>(&self, rid: ResourceId) -> Result<Rc<T>, AnyError> {
     self
       .index
       .get(&rid)
       .and_then(|rc| rc.downcast_rc::<T>())
       .map(Clone::clone)
+      .ok_or_else(bad_resource_id)
   }
 
-  pub fn get_any(&self, rid: ResourceId) -> Option<Rc<dyn Resource>> {
-    self.index.get(&rid).map(Clone::clone)
+  pub fn get_any(&self, rid: ResourceId) -> Result<Rc<dyn Resource>, AnyError> {
+    self
+      .index
+      .get(&rid)
+      .map(Clone::clone)
+      .ok_or_else(bad_resource_id)
   }
 
   /// Removes a resource of type `T` from the resource table and returns it.
   /// If a resource with the given `rid` exists but its type does not match `T`,
   /// it is not removed from the resource table. Note that the resource's
   /// `close()` method is *not* called.
-  pub fn take<T: Resource>(&mut self, rid: ResourceId) -> Option<Rc<T>> {
+  pub fn take<T: Resource>(
+    &mut self,
+    rid: ResourceId,
+  ) -> Result<Rc<T>, AnyError> {
     let resource = self.get::<T>(rid)?;
     self.index.remove(&rid);
-    Some(resource)
+    Ok(resource)
   }
 
   /// Removes a resource from the resource table and returns it. Note that the
   /// resource's `close()` method is *not* called.
-  pub fn take_any(&mut self, rid: ResourceId) -> Option<Rc<dyn Resource>> {
-    self.index.remove(&rid)
+  pub fn take_any(
+    &mut self,
+    rid: ResourceId,
+  ) -> Result<Rc<dyn Resource>, AnyError> {
+    self.index.remove(&rid).ok_or_else(bad_resource_id)
   }
 
   /// Removes the resource with the given `rid` from the resource table. If the
@@ -139,8 +152,12 @@ impl ResourceTable {
   /// counted, therefore pending ops are not automatically cancelled. A resource
   /// may implement the `close()` method to perform clean-ups such as canceling
   /// ops.
-  pub fn close(&mut self, rid: ResourceId) -> Option<()> {
-    self.index.remove(&rid).map(|resource| resource.close())
+  pub fn close(&mut self, rid: ResourceId) -> Result<(), AnyError> {
+    self
+      .index
+      .remove(&rid)
+      .ok_or_else(bad_resource_id)
+      .map(|resource| resource.close())
   }
 
   /// Returns an iterator that yields a `(id, name)` pair for every resource