Remove CoreResource::inspect_repr method (#3274)

Towards simplifying (or better removing entirely) the CoreResource
trait. Resources should be any bit of privileged heap allocated memory
that needs to be referenced from JS, not very specific trait
implementations. Therefore CoreResource should be pushed towards being
as general as possible.

cli/ops/repl.rs

@@ -24,11 +24,7 @@ pub fn init(i: &mut Isolate, s: &ThreadSafeState) {
 
 struct ReplResource(Arc<Mutex<Repl>>);
 
-impl CoreResource for ReplResource {
-  fn inspect_repr(&self) -> &str {
-    "repl"
-  }
-}
+impl CoreResource for ReplResource {}
 
 #[derive(Deserialize)]
 #[serde(rename_all = "camelCase")]
@@ -49,7 +45,7 @@ fn op_repl_start(
   let repl = repl::Repl::new(history_path);
   let resource = ReplResource(Arc::new(Mutex::new(repl)));
   let mut table = resources::lock_resource_table();
-  let rid = table.add(Box::new(resource));
+  let rid = table.add("repl", Box::new(resource));
   Ok(JsonOp::Sync(json!(rid)))
 }
 

cli/resources.rs

@@ -49,9 +49,9 @@ lazy_static! {
     let mut table = ResourceTable::default();
 
     // TODO Load these lazily during lookup?
-    table.add(Box::new(CliResource::Stdin(tokio::io::stdin())));
+    table.add("stdin", Box::new(CliResource::Stdin(tokio::io::stdin())));
 
-    table.add(Box::new(CliResource::Stdout({
+    table.add("stdout", Box::new(CliResource::Stdout({
       #[cfg(not(windows))]
       let stdout = unsafe { std::fs::File::from_raw_fd(1) };
       #[cfg(windows)]
@@ -62,7 +62,7 @@ lazy_static! {
       tokio::fs::File::from_std(stdout)
     })));
 
-    table.add(Box::new(CliResource::Stderr(tokio::io::stderr())));
+    table.add("stderr", Box::new(CliResource::Stderr(tokio::io::stderr())));
     table
   });
 }
@@ -98,6 +98,9 @@ enum CliResource {
 }
 
 impl CoreResource for CliResource {
+  // TODO(ry) These task notifications are hacks to workaround various behaviors
+  // in Tokio. They should not influence the overall design of Deno. The
+  // CoreResource::close should be removed in favor of the drop trait.
   fn close(&self) {
     match self {
       CliResource::TcpListener(_, Some(t)) => {
@@ -109,25 +112,6 @@ impl CoreResource for CliResource {
       _ => {}
     }
   }
-
-  fn inspect_repr(&self) -> &str {
-    match self {
-      CliResource::Stdin(_) => "stdin",
-      CliResource::Stdout(_) => "stdout",
-      CliResource::Stderr(_) => "stderr",
-      CliResource::FsFile(_) => "fsFile",
-      CliResource::TcpListener(_, _) => "tcpListener",
-      CliResource::TlsListener(_, _, _) => "tlsListener",
-      CliResource::TcpStream(_) => "tcpStream",
-      CliResource::ClientTlsStream(_) => "clientTlsStream",
-      CliResource::ServerTlsStream(_) => "serverTlsStream",
-      CliResource::HttpBody(_) => "httpBody",
-      CliResource::Child(_) => "child",
-      CliResource::ChildStdin(_) => "childStdin",
-      CliResource::ChildStdout(_) => "childStdout",
-      CliResource::ChildStderr(_) => "childStderr",
-    }
-  }
 }
 
 pub fn lock_resource_table<'a>() -> MutexGuard<'a, ResourceTable> {
@@ -319,13 +303,16 @@ impl DenoAsyncWrite for Resource {
 
 pub fn add_fs_file(fs_file: tokio::fs::File) -> Resource {
   let mut table = lock_resource_table();
-  let rid = table.add(Box::new(CliResource::FsFile(fs_file)));
+  let rid = table.add("fsFile", Box::new(CliResource::FsFile(fs_file)));
   Resource { rid }
 }
 
 pub fn add_tcp_listener(listener: tokio::net::TcpListener) -> Resource {
   let mut table = lock_resource_table();
-  let rid = table.add(Box::new(CliResource::TcpListener(listener, None)));
+  let rid = table.add(
+    "tcpListener",
+    Box::new(CliResource::TcpListener(listener, None)),
+  );
   Resource { rid }
 }
 
@@ -334,33 +321,41 @@ pub fn add_tls_listener(
   acceptor: TlsAcceptor,
 ) -> Resource {
   let mut table = lock_resource_table();
-  let rid =
-    table.add(Box::new(CliResource::TlsListener(listener, acceptor, None)));
+  let rid = table.add(
+    "tlsListener",
+    Box::new(CliResource::TlsListener(listener, acceptor, None)),
+  );
   Resource { rid }
 }
 
 pub fn add_tcp_stream(stream: tokio::net::TcpStream) -> Resource {
   let mut table = lock_resource_table();
-  let rid = table.add(Box::new(CliResource::TcpStream(stream)));
+  let rid = table.add("tcpStream", Box::new(CliResource::TcpStream(stream)));
   Resource { rid }
 }
 
 pub fn add_tls_stream(stream: ClientTlsStream<TcpStream>) -> Resource {
   let mut table = lock_resource_table();
-  let rid = table.add(Box::new(CliResource::ClientTlsStream(Box::new(stream))));
+  let rid = table.add(
+    "clientTlsStream",
+    Box::new(CliResource::ClientTlsStream(Box::new(stream))),
+  );
   Resource { rid }
 }
 
 pub fn add_server_tls_stream(stream: ServerTlsStream<TcpStream>) -> Resource {
   let mut table = lock_resource_table();
-  let rid = table.add(Box::new(CliResource::ServerTlsStream(Box::new(stream))));
+  let rid = table.add(
+    "serverTlsStream",
+    Box::new(CliResource::ServerTlsStream(Box::new(stream))),
+  );
   Resource { rid }
 }
 
 pub fn add_reqwest_body(body: ReqwestDecoder) -> Resource {
   let body = HttpBody::from(body);
   let mut table = lock_resource_table();
-  let rid = table.add(Box::new(CliResource::HttpBody(body)));
+  let rid = table.add("httpBody", Box::new(CliResource::HttpBody(body)));
   Resource { rid }
 }
 
@@ -383,21 +378,23 @@ pub fn add_child(mut child: tokio_process::Child) -> ChildResources {
 
   if child.stdin().is_some() {
     let stdin = child.stdin().take().unwrap();
-    let rid = table.add(Box::new(CliResource::ChildStdin(stdin)));
+    let rid = table.add("childStdin", Box::new(CliResource::ChildStdin(stdin)));
     resources.stdin_rid = Some(rid);
   }
   if child.stdout().is_some() {
     let stdout = child.stdout().take().unwrap();
-    let rid = table.add(Box::new(CliResource::ChildStdout(stdout)));
+    let rid =
+      table.add("childStdout", Box::new(CliResource::ChildStdout(stdout)));
     resources.stdout_rid = Some(rid);
   }
   if child.stderr().is_some() {
     let stderr = child.stderr().take().unwrap();
-    let rid = table.add(Box::new(CliResource::ChildStderr(stderr)));
+    let rid =
+      table.add("childStderr", Box::new(CliResource::ChildStderr(stderr)));
     resources.stderr_rid = Some(rid);
   }
 
-  let rid = table.add(Box::new(CliResource::Child(Box::new(child))));
+  let rid = table.add("child", Box::new(CliResource::Child(Box::new(child))));
   resources.child_rid = Some(rid);
 
   resources
@@ -440,7 +437,7 @@ pub fn get_file(rid: ResourceId) -> Result<std::fs::File, ErrBox> {
   let mut table = lock_resource_table();
   // We take ownership of File here.
   // It is put back below while still holding the lock.
-  let repr = table.map.remove(&rid).ok_or_else(bad_resource)?;
+  let (_name, repr) = table.map.remove(&rid).ok_or_else(bad_resource)?;
   let repr = repr
     .downcast::<CliResource>()
     .or_else(|_| Err(bad_resource()))?;
@@ -460,7 +457,10 @@ pub fn get_file(rid: ResourceId) -> Result<std::fs::File, ErrBox> {
       // Insert the entry back with the same rid.
       table.map.insert(
         rid,
-        Box::new(CliResource::FsFile(tokio_fs::File::from_std(std_file))),
+        (
+          "fsFile".to_string(),
+          Box::new(CliResource::FsFile(tokio_fs::File::from_std(std_file))),
+        ),
       );
 
       maybe_std_file_copy.map_err(ErrBox::from)

cli/state.rs

@@ -196,7 +196,7 @@ impl ThreadSafeState {
     };
 
     let mut table = resources::lock_resource_table();
-    let rid = table.add(Box::new(external_channels));
+    let rid = table.add("worker", Box::new(external_channels));
 
     let import_map: Option<ImportMap> =
       match global_state.flags.import_map_path.as_ref() {

cli/worker.rs

@@ -31,11 +31,7 @@ pub struct WorkerChannels {
   pub receiver: mpsc::Receiver<Buf>,
 }
 
-impl CoreResource for WorkerChannels {
-  fn inspect_repr(&self) -> &str {
-    "worker"
-  }
-}
+impl CoreResource for WorkerChannels {}
 
 /// Wraps deno::Isolate to provide source maps, ops for the CLI, and
 /// high-level module loading.

core/examples/http_bench.rs

@@ -190,19 +190,11 @@ pub fn bad_resource() -> Error {
 
 struct TcpListener(tokio::net::TcpListener);
 
-impl Resource for TcpListener {
-  fn inspect_repr(&self) -> &str {
-    "tcpListener"
-  }
-}
+impl Resource for TcpListener {}
 
 struct TcpStream(tokio::net::TcpStream);
 
-impl Resource for TcpStream {
-  fn inspect_repr(&self) -> &str {
-    "tcpStream"
-  }
-}
+impl Resource for TcpStream {}
 
 lazy_static! {
   static ref RESOURCE_TABLE: Mutex<ResourceTable> =
@@ -225,7 +217,7 @@ fn op_accept(record: Record, _zero_copy_buf: Option<PinnedBuf>) -> Box<HttpOp> {
   .and_then(move |(stream, addr)| {
     debug!("accept success {}", addr);
     let mut table = lock_resource_table();
-    let rid = table.add(Box::new(TcpStream(stream)));
+    let rid = table.add("tcpStream", Box::new(TcpStream(stream)));
     Ok(rid as i32)
   });
   Box::new(fut)
@@ -239,7 +231,7 @@ fn op_listen(
   let addr = "127.0.0.1:4544".parse::<SocketAddr>().unwrap();
   let listener = tokio::net::TcpListener::bind(&addr).unwrap();
   let mut table = lock_resource_table();
-  let rid = table.add(Box::new(TcpListener(listener)));
+  let rid = table.add("tcpListener", Box::new(TcpListener(listener)));
   Box::new(futures::future::ok(rid as i32))
 }
 

core/resources.rs

@@ -17,7 +17,7 @@ pub type ResourceId = u32;
 
 /// These store Deno's file descriptors. These are not necessarily the operating
 /// system ones.
-type ResourceMap = HashMap<ResourceId, Box<dyn Resource>>;
+type ResourceMap = HashMap<ResourceId, (String, Box<dyn Resource>)>;
 
 #[derive(Default)]
 pub struct ResourceTable {
@@ -29,7 +29,7 @@ pub struct ResourceTable {
 
 impl ResourceTable {
   pub fn get<T: Resource>(&self, rid: ResourceId) -> Option<&T> {
-    if let Some(resource) = self.map.get(&rid) {
+    if let Some((_name, resource)) = self.map.get(&rid) {
       return resource.downcast_ref::<T>();
     }
 
@@ -37,7 +37,7 @@ impl ResourceTable {
   }
 
   pub fn get_mut<T: Resource>(&mut self, rid: ResourceId) -> Option<&mut T> {
-    if let Some(resource) = self.map.get_mut(&rid) {
+    if let Some((_name, resource)) = self.map.get_mut(&rid) {
       return resource.downcast_mut::<T>();
     }
 
@@ -51,9 +51,9 @@ impl ResourceTable {
     next_rid as ResourceId
   }
 
-  pub fn add(&mut self, resource: Box<dyn Resource>) -> ResourceId {
+  pub fn add(&mut self, name: &str, resource: Box<dyn Resource>) -> ResourceId {
     let rid = self.next_rid();
-    let r = self.map.insert(rid, resource);
+    let r = self.map.insert(rid, (name.to_string(), resource));
     assert!(r.is_none());
     rid
   }
@@ -62,18 +62,17 @@ impl ResourceTable {
     self
       .map
       .iter()
-      .map(|(key, value)| (*key, value.inspect_repr().to_string()))
+      .map(|(key, (name, _resource))| (*key, name.clone()))
       .collect()
   }
 
   // close(2) is done by dropping the value. Therefore we just need to remove
   // the resource from the RESOURCE_TABLE.
   pub fn close(&mut self, rid: ResourceId) -> Option<()> {
-    if let Some(resource) = self.map.remove(&rid) {
+    if let Some((_name, resource)) = self.map.remove(&rid) {
       resource.close();
       return Some(());
     }
-
     None
   }
 }
@@ -81,8 +80,7 @@ impl ResourceTable {
 /// Abstract type representing resource in Deno.
 pub trait Resource: Downcast + Any + Send {
   /// Method that allows to cleanup resource.
+  // TODO(ry) remove this method. Resources should rely on drop trait instead.
   fn close(&self) {}
-
-  fn inspect_repr(&self) -> &str;
 }
 impl_downcast!(Resource);