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: remove cli::resources::Resource #3285

Merged
merged 6 commits into from Nov 7, 2019

Conversation

@bartlomieju
Copy link
Contributor

bartlomieju commented Nov 7, 2019

Blocked on #3271 (because based on that work)

Goal of this PR is to completely remove cli::resources::Resource and remove CoreResource type alias. Done

@bartlomieju bartlomieju force-pushed the bartlomieju:refactor-cli_resources branch from 3e32eb7 to a34b24d Nov 7, 2019
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

bartlomieju commented Nov 7, 2019

CC @ry

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

bartlomieju commented Nov 7, 2019

It'd be nice to remove this hack:

deno/cli/resources.rs

Lines 313 to 349 in 415d4c2

// TODO: revamp this after the following lands:
// https://github.com/tokio-rs/tokio/pull/785
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 (_name, repr) = table.map.remove(&rid).ok_or_else(bad_resource)?;
let repr = repr
.downcast::<CliResource>()
.or_else(|_| Err(bad_resource()))?;
match *repr {
CliResource::FsFile(r) => {
// Trait Clone not implemented on tokio::fs::File,
// so convert to std File first.
let std_file = r.into_std();
// Create a copy and immediately put back.
// We don't want to block other resource ops.
// try_clone() would yield a copy containing the same
// underlying fd, so operations on the copy would also
// affect the one in resource table, and we don't need
// to write back.
let maybe_std_file_copy = std_file.try_clone();
// Insert the entry back with the same rid.
table.map.insert(
rid,
(
"fsFile".to_string(),
Box::new(CliResource::FsFile(tokio_fs::File::from_std(std_file))),
),
);
maybe_std_file_copy.map_err(ErrBox::from)
}
_ => Err(bad_resource()),
}
}

It'd be possible if we changed Deno.run to be asynchronous function instead of sync one. WDYT?

@bartlomieju bartlomieju changed the title refactor: CLI resources again refactor: remove cli::resources::Resource Nov 7, 2019
@ry
ry approved these changes Nov 7, 2019
Copy link
Collaborator

ry left a comment

LGTM - good clean up

@ry ry merged commit 25c2760 into denoland:master Nov 7, 2019
9 checks passed
9 checks passed
test macOS-latest
Details
test_std macOS-latest
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
@bartlomieju bartlomieju deleted the bartlomieju:refactor-cli_resources branch Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.