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.

6 files changed, 52 insertions, 70 deletions

diff --git a/cli/ops/repl.rs b/cli/ops/repl.rs
index 4a3ba68d4..ba63c5109 100644
--- a/cli/ops/repl.rs
+++ b/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)))
 }
 
diff --git a/cli/resources.rs b/cli/resources.rs
index ba7795f5d..2910d16b6 100644
--- a/cli/resources.rs
+++ b/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)
diff --git a/cli/state.rs b/cli/state.rs
index 544c199b8..8d14042aa 100644
--- a/cli/state.rs
+++ b/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() {
diff --git a/cli/worker.rs b/cli/worker.rs
index 90fb95af7..6b4970015 100644
--- a/cli/worker.rs
+++ b/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.
diff --git a/core/examples/http_bench.rs b/core/examples/http_bench.rs
index 764e04303..8635d4f23 100644
--- a/core/examples/http_bench.rs
+++ b/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))
 }
 
diff --git a/core/resources.rs b/core/resources.rs
index 368f83454..1ba061d0b 100644
--- a/core/resources.rs
+++ b/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);