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

Partial reload #3109

Merged
merged 15 commits into from Oct 17, 2019
92 changes: 91 additions & 1 deletion cli/file_fetcher.rs
Expand Up @@ -70,6 +70,7 @@ pub struct SourceFileFetcher {
deps_cache: DiskCache,
progress: Progress,
source_file_cache: SourceFileCache,
cache_blacklist: Vec<String>,
use_disk_cache: bool,
no_remote_fetch: bool,
}
Expand All @@ -79,12 +80,14 @@ impl SourceFileFetcher {
deps_cache: DiskCache,
progress: Progress,
use_disk_cache: bool,
cache_blacklist: Vec<String>,
no_remote_fetch: bool,
) -> std::io::Result<Self> {
let file_fetcher = Self {
deps_cache,
progress,
source_file_cache: SourceFileCache::default(),
cache_blacklist,
use_disk_cache,
no_remote_fetch,
};
Expand Down Expand Up @@ -308,8 +311,10 @@ impl SourceFileFetcher {
return Box::new(futures::future::err(too_many_redirects()));
}

let is_blacklisted =
check_cache_blacklist(module_url, self.cache_blacklist.as_ref());
// First try local cache
if use_disk_cache {
if use_disk_cache && !is_blacklisted {
match self.fetch_cached_remote_source(&module_url) {
Ok(Some(source_file)) => {
return Box::new(futures::future::ok(source_file));
Expand Down Expand Up @@ -533,6 +538,26 @@ fn filter_shebang(bytes: Vec<u8>) -> Vec<u8> {
}
}

fn check_cache_blacklist(url: &Url, black_list: &[String]) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the black_list argument be a Vec<Url> instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we never utilize any of the Url properties by elements in the list, so I thought that maybe strings would be better (as in --allow-read whitelist)

but I agree that Vec<Url> would be more clean, and parsing few urls wouldn't harm performance

Copy link
Member

@ry ry Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I guess it's not a big deal.

I'm ready to land this patch.... but maybe you could add a short description of this feature to the website/manual.md somewhere? I think a sentence or two would do it in the section where --reload is described.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

let mut url_without_fragmets = url.clone();
url_without_fragmets.set_fragment(None);
if black_list.contains(&String::from(url_without_fragmets.as_str())) {
return true;
}
let mut url_without_query_strings = url_without_fragmets;
url_without_query_strings.set_query(None);
let mut path_buf = PathBuf::from(url_without_query_strings.as_str());
loop {
if black_list.contains(&String::from(path_buf.to_str().unwrap())) {
return true;
}
if !path_buf.pop() {
break;
}
}
false
}

#[derive(Debug, Default)]
/// Header metadata associated with a particular "symbolic" source code file.
/// (the associated source code file might not be cached, while remaining
Expand Down Expand Up @@ -617,6 +642,7 @@ mod tests {
DiskCache::new(&dir_path.to_path_buf().join("deps")),
Progress::new(),
true,
vec![],
false,
)
.expect("setup fail")
Expand All @@ -638,6 +664,70 @@ mod tests {
};
}

#[test]
fn test_cache_blacklist() {
let args = crate::flags::resolve_urls(vec![
String::from("http://deno.land/std"),
String::from("http://github.com/example/mod.ts"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhvsa please add test cases with query string (?foo=bar) and fragment (#something)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ay, forgot about that, thank you

String::from("http://fragment.com/mod.ts#fragment"),
String::from("http://query.com/mod.ts?foo=bar"),
String::from("http://queryandfragment.com/mod.ts?foo=bar#fragment"),
]);

let url1: Url = "http://deno.land/std/fs/mod.ts".parse().unwrap();
let url2: Url = "http://github.com/example/file.ts".parse().unwrap();
let url3: Url = "http://github.com/example/mod.ts".parse().unwrap();
let url4: Url = "http://github.com/example/mod.ts?foo=bar".parse().unwrap();
let url5: Url =
"http://github.com/example/mod.ts#fragment".parse().unwrap();
let url6: Url = "http://fragment.com/mod.ts".parse().unwrap();
let url7: Url = "http://query.com/mod.ts".parse().unwrap();
let url8: Url = "http://fragment.com/mod.ts#fragment".parse().unwrap();
let url9: Url = "http://query.com/mod.ts?foo=bar".parse().unwrap();
let url10: Url = "http://queryandfragment.com/mod.ts".parse().unwrap();
let url11: Url = "http://queryandfragment.com/mod.ts?foo=bar"
.parse()
.unwrap();
let url12: Url = "http://queryandfragment.com/mod.ts#fragment"
.parse()
.unwrap();
let url13: Url =
"http://query.com/mod.ts?foo=bar#fragment".parse().unwrap();
let url14: Url = "http://fragment.com/mod.ts?foo=bar#fragment"
.parse()
.unwrap();

let result1 = check_cache_blacklist(&url1, &args);
let result2 = check_cache_blacklist(&url2, &args);
let result3 = check_cache_blacklist(&url3, &args);
let result4 = check_cache_blacklist(&url4, &args);
let result5 = check_cache_blacklist(&url5, &args);
let result6 = check_cache_blacklist(&url6, &args);
let result7 = check_cache_blacklist(&url7, &args);
let result8 = check_cache_blacklist(&url8, &args);
let result9 = check_cache_blacklist(&url9, &args);
let result10 = check_cache_blacklist(&url10, &args);
let result11 = check_cache_blacklist(&url11, &args);
let result12 = check_cache_blacklist(&url12, &args);
let result13 = check_cache_blacklist(&url13, &args);
let result14 = check_cache_blacklist(&url14, &args);

assert_eq!(result1, true);
assert_eq!(result2, false);
assert_eq!(result3, true);
assert_eq!(result4, true);
assert_eq!(result5, true);
assert_eq!(result6, true);
assert_eq!(result7, false);
assert_eq!(result8, true);
assert_eq!(result9, true);
assert_eq!(result10, false);
assert_eq!(result11, true);
assert_eq!(result12, false);
assert_eq!(result13, true);
assert_eq!(result14, true);
ry marked this conversation as resolved.
Show resolved Hide resolved
}

#[test]
fn test_source_code_headers_get_and_save() {
let (_temp_dir, fetcher) = test_setup();
Expand Down
44 changes: 42 additions & 2 deletions cli/flags.rs
Expand Up @@ -11,6 +11,7 @@ use log::Level;
use std;
use std::str;
use std::str::FromStr;
use url::Url;

macro_rules! std_url {
($x:expr) => {
Expand Down Expand Up @@ -45,6 +46,7 @@ pub struct DenoFlags {
pub import_map_path: Option<String>,
pub allow_read: bool,
pub read_whitelist: Vec<String>,
pub cache_blacklist: Vec<String>,
pub allow_write: bool,
pub write_whitelist: Vec<String>,
pub allow_net: bool,
Expand Down Expand Up @@ -172,8 +174,20 @@ To get help on the another subcommands (run in this case):
).arg(
Arg::with_name("reload")
.short("r")
.min_values(0)
.takes_value(true)
.use_delimiter(true)
.require_equals(true)
Comment on lines +179 to +180
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those required? Would be nice to do --reload https://deno.land/std/fs/utils.ts https://deno.land/std/fmt/colors.ts

.long("reload")
.help("Reload source code cache (recompile TypeScript)")
.help("Reload source code cache (recompile TypeScript). Supports blacklist")
.value_name("blacklist")
.long_help("Reload source code cache (recompile TypeScript). Supports blacklist
--reload
Reload everything
--reload=https://deno.land/std
Reload everything from the standard module
--reload=https://deno.land/std/fs/utils.ts,https://deno.land/std/fmt/colors.ts
Reloads only fs/utils and fmt/colors modules")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a "whitelist" rather than a "blacklist" ?

Also I don't think it's useful to say "Supports blacklist" or "Supports whitelist" - the examples you give are sufficient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually a cache blacklist - urls passed to --reload will be forced to redownloaded and recompile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. I think "blacklist" makes sense elsewhere in the code, but not here in the user facing docs.

.global(true),
).arg(
Arg::with_name("config")
Expand Down Expand Up @@ -509,6 +523,23 @@ fn resolve_paths(paths: Vec<String>) -> Vec<String> {
out
}

pub fn resolve_urls(urls: Vec<String>) -> Vec<String> {
let mut out: Vec<String> = vec![];
for urlstr in urls.iter() {
let result = Url::from_str(urlstr);
if result.is_err() {
panic!("Bad Url: {}", urlstr);
}
let mut url = result.unwrap();
url.set_fragment(None);
let mut full_url = String::from(url.as_str());
if full_url.len() > 1 && full_url.ends_with('/') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this pop needed? Can you add a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for prefix matching, it's the same thing as in files whitelist
it removes everything after '/' from the address, so e.g /foo/bar -> /foo

full_url.pop();
}
out.push(full_url);
}
out
}
/// This function expands "bare port" paths (eg. ":8080")
/// into full paths with hosts. It expands to such paths
/// into 3 paths with following hosts: `0.0.0.0:port`, `127.0.0.1:port` and `localhost:port`.
Expand Down Expand Up @@ -566,7 +597,16 @@ pub fn parse_flags(
flags.version = true;
}
if matches.is_present("reload") {
flags.reload = true;
if matches.value_of("reload").is_some() {
let cache_bl = matches.values_of("reload").unwrap();
let raw_cache_blacklist: Vec<String> =
cache_bl.map(std::string::ToString::to_string).collect();
flags.cache_blacklist = resolve_urls(raw_cache_blacklist);
debug!("cache blacklist: {:#?}", &flags.cache_blacklist);
flags.reload = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superficial

} else {
flags.reload = true;
}
}
flags.config_path = matches.value_of("config").map(ToOwned::to_owned);
if matches.is_present("v8-options") {
Expand Down
1 change: 1 addition & 0 deletions cli/state.rs
Expand Up @@ -225,6 +225,7 @@ impl ThreadSafeState {
dir.deps_cache.clone(),
progress.clone(),
!flags.reload,
flags.cache_blacklist.clone(),
flags.no_fetch,
)?;

Expand Down