Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Resource table #3134

Closed
bartlomieju opened this issue Oct 16, 2019 · 1 comment · Fixed by #3150
Closed

refactor: Resource table #3134

bartlomieju opened this issue Oct 16, 2019 · 1 comment · Fixed by #3150

Comments

@bartlomieju
Copy link
Member

bartlomieju commented Oct 16, 2019

Before working on #2180 we need to address situation with resource table. Resource table is used to store resource ids with their corresponding representations.

deno/cli/resources.rs

Lines 56 to 104 in b1685ce

lazy_static! {
// Starts at 3 because stdio is [0-2].
static ref NEXT_RID: AtomicUsize = AtomicUsize::new(3);
static ref RESOURCE_TABLE: Mutex<ResourceTable> = Mutex::new({
let mut m = BTreeMap::new();
// TODO Load these lazily during lookup?
m.insert(0, Repr::Stdin(tokio::io::stdin()));
m.insert(1, Repr::Stdout({
#[cfg(not(windows))]
let stdout = unsafe { std::fs::File::from_raw_fd(1) };
#[cfg(windows)]
let stdout = unsafe {
std::fs::File::from_raw_handle(winapi::um::processenv::GetStdHandle(
winapi::um::winbase::STD_OUTPUT_HANDLE))
};
tokio::fs::File::from_std(stdout)
}));
m.insert(2, Repr::Stderr(tokio::io::stderr()));
m
});
}
// Internal representation of Resource.
enum Repr {
Stdin(tokio::io::Stdin),
Stdout(tokio::fs::File),
Stderr(tokio::io::Stderr),
FsFile(tokio::fs::File),
// Since TcpListener might be closed while there is a pending accept task,
// we need to track the task so that when the listener is closed,
// this pending task could be notified and die.
// Currently TcpListener itself does not take care of this issue.
// See: https://github.com/tokio-rs/tokio/issues/846
TcpListener(tokio::net::TcpListener, Option<futures::task::Task>),
TcpStream(tokio::net::TcpStream),
TlsStream(Box<TlsStream<TcpStream>>),
HttpBody(HttpBody),
Repl(Arc<Mutex<Repl>>),
// Enum size is bounded by the largest variant.
// Use `Box` around large `Child` struct.
// https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
Child(Box<tokio_process::Child>),
ChildStdin(tokio_process::ChildStdin),
ChildStdout(tokio_process::ChildStdout),
ChildStderr(tokio_process::ChildStderr),
Worker(WorkerChannels),
}

There are two problems with this approach:

  • all resource related logic lives in cli/resources.rs, instead logic for each kind of resource should live alongside ops code that utilizes it
  • all representations are hard coded which effectively prohibits splitting ops into separate crates - CLI would still have to be aware what kind of resource each op can use

We want ResourceTable to expose public API allowing op crates to register any resources they see fit to utilize (eg. process crate would implement resources analogous to Repr::Child, Repr:ChildStdin, Repr:ChildStdout and Repr:ChildStderr).

That means that ResourceTable should be a struct mapping resource ids to boxed trait objects. The trait could be as simple as:

trait Resource {
  pub fn close(&self);
}

struct ResourceTable(HashMap<Rid, Box<dyn Resource>>);

Trait objects from ResourceTable would then be upcasted into concrete type inside the op.

New resource table should be a core concept. It should be first implemented for deno_core_http_bench, then we should refactor cli::resources::RESOURCE_TABLE.

I'll be working on it.

CC @ry @piscisaureus

@ry
Copy link
Member

ry commented Oct 16, 2019

Good write up. There are a couple bits of machinery implicit in this that are worth mention explicitly:

  1. There should be a way to register new resources at runtime, obtaining a new rid.
  2. Resource objects should be up-casted inside the op implementations.
  3. resource.close() removes a resource from the table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants