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

fix(watch): preserve ProcState::file_fetcher between restarts #15466

Merged
merged 43 commits into from Jan 10, 2023
Merged
Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
ba0c423
fix(watch): preserve `ProcState::file_fetcher` between restarts
nayeemrmn Aug 12, 2022
b25bc37
Remove reload option from GraphData::add_graph()
nayeemrmn Aug 12, 2022
1da5b7d
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Aug 15, 2022
e2943e0
Add comment
nayeemrmn Aug 15, 2022
ddb3638
reset CI
bartlomieju Aug 16, 2022
30bc5f5
Merge branch 'main' into watch-file-fetcher
bartlomieju Aug 17, 2022
b3b55e0
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Aug 17, 2022
bc1f8af
Add test
nayeemrmn Aug 17, 2022
2856dcc
reset CI
bartlomieju Aug 17, 2022
aab10e2
reset CI
bartlomieju Aug 18, 2022
0ba49dc
Merge branch 'main' into watch-file-fetcher
bartlomieju Aug 18, 2022
b6cae90
Change content on second write
nayeemrmn Aug 18, 2022
9d42636
try debug test
nayeemrmn Aug 18, 2022
ef9ae2f
fixup! try debug test
nayeemrmn Aug 18, 2022
ae98309
try different panic
nayeemrmn Aug 18, 2022
ebdc5ec
try different panic
nayeemrmn Aug 18, 2022
2c443eb
try different panic
nayeemrmn Aug 18, 2022
8457d9b
explicit server drop
nayeemrmn Aug 18, 2022
28c1d46
drop function
nayeemrmn Aug 18, 2022
6f99160
remove debug
nayeemrmn Aug 18, 2022
a40dc6b
drop everything before explicit panic
nayeemrmn Aug 18, 2022
a2e79f5
fixup! drop everything before explicit panic
nayeemrmn Aug 18, 2022
bd241a7
remove debug again
nayeemrmn Aug 18, 2022
b1a47ee
Use #[flaky_test]
nayeemrmn Aug 18, 2022
a09c853
Revert "Use #[flaky_test]"
nayeemrmn Aug 18, 2022
d31f84f
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Aug 19, 2022
954ff8d
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Aug 29, 2022
e80dcdf
fix merge
nayeemrmn Aug 29, 2022
f52a7eb
update std
nayeemrmn Aug 29, 2022
a4c79bf
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Oct 18, 2022
e5ea22d
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Dec 8, 2022
6ada81e
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Dec 8, 2022
f57ff1a
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Dec 17, 2022
bfc639e
Write twice to address hang
nayeemrmn Dec 17, 2022
d39ae89
Revert double write
nayeemrmn Dec 17, 2022
8d58477
Watch new paths before printing finished
nayeemrmn Dec 17, 2022
bd4f2cc
Swap expected output as well
nayeemrmn Dec 17, 2022
91deee2
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Dec 17, 2022
d625be8
Merge branch 'main' into watch-file-fetcher
bartlomieju Dec 17, 2022
a2e8172
Make ProcState::reset_for_file_watcher() mutate
nayeemrmn Dec 18, 2022
08d454a
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Dec 18, 2022
8e57648
Merge remote-tracking branch 'upstream/main' into watch-file-fetcher
nayeemrmn Jan 9, 2023
fa894bf
reset ci
nayeemrmn Jan 9, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions cli/cache/mod.rs
Expand Up @@ -50,12 +50,10 @@ pub struct FetchCacher {
impl FetchCacher {
pub fn new(
emit_cache: EmitCache,
file_fetcher: FileFetcher,
file_fetcher: Arc<FileFetcher>,
root_permissions: Permissions,
dynamic_permissions: Permissions,
) -> Self {
let file_fetcher = Arc::new(file_fetcher);

Self {
emit_cache,
dynamic_permissions,
Expand Down
1 change: 1 addition & 0 deletions cli/cache/node.rs
Expand Up @@ -24,6 +24,7 @@ struct CjsAnalysisData {
pub reexports: Vec<String>,
}

#[derive(Clone)]
pub struct NodeAnalysisCache {
db_file_path: Option<PathBuf>,
inner: Arc<Mutex<Option<Option<NodeAnalysisCacheInner>>>>,
Expand Down
8 changes: 8 additions & 0 deletions cli/cache/parsed_source.rs
Expand Up @@ -65,6 +65,14 @@ impl ParsedSourceCache {
}
}

pub fn reset_for_file_watcher(&self) -> Self {
Self {
db_cache_path: self.db_cache_path.clone(),
cli_version: self.cli_version.clone(),
sources: Default::default(),
}
}

pub fn get_parsed_source_from_module(
&self,
module: &deno_graph::Module,
Expand Down
8 changes: 4 additions & 4 deletions cli/graph_util.rs
Expand Up @@ -72,7 +72,7 @@ pub struct GraphData {

impl GraphData {
/// Store data from `graph` into `self`.
pub fn add_graph(&mut self, graph: &ModuleGraph, reload: bool) {
pub fn add_graph(&mut self, graph: &ModuleGraph) {
for graph_import in &graph.imports {
for dep in graph_import.dependencies.values() {
for resolved in [&dep.maybe_code, &dep.maybe_type] {
Expand All @@ -96,7 +96,7 @@ impl GraphData {
continue;
}

if !reload && self.modules.contains_key(specifier) {
if self.modules.contains_key(specifier) {
continue;
}

Expand Down Expand Up @@ -470,7 +470,7 @@ impl GraphData {
impl From<&ModuleGraph> for GraphData {
fn from(graph: &ModuleGraph) -> Self {
let mut graph_data = GraphData::default();
graph_data.add_graph(graph, false);
graph_data.add_graph(graph);
graph_data
}
}
Expand Down Expand Up @@ -542,7 +542,7 @@ pub async fn create_graph_and_maybe_check(

let check_js = ps.options.check_js();
let mut graph_data = GraphData::default();
graph_data.add_graph(&graph, false);
graph_data.add_graph(&graph);
graph_data
.check(
&graph.roots,
Expand Down
1 change: 0 additions & 1 deletion cli/module_loader.rs
Expand Up @@ -252,7 +252,6 @@ impl ModuleLoader for CliModuleLoader {
lib,
root_permissions,
dynamic_permissions,
false,
)
.await
}
Expand Down
52 changes: 39 additions & 13 deletions cli/proc_state.rs
Expand Up @@ -77,7 +77,7 @@ pub struct ProcState(Arc<Inner>);

pub struct Inner {
pub dir: DenoDir,
pub file_fetcher: FileFetcher,
pub file_fetcher: Arc<FileFetcher>,
pub http_client: HttpClient,
pub options: Arc<CliOptions>,
pub emit_cache: EmitCache,
Expand Down Expand Up @@ -147,6 +147,38 @@ impl ProcState {
Ok(ps)
}

/// Return a cloned `self` with all runtime state reset to its default. This
/// should be used on file watcher restarts.
pub fn reset_for_file_watcher(&self) -> Self {
nayeemrmn marked this conversation as resolved.
Show resolved Hide resolved
nayeemrmn marked this conversation as resolved.
Show resolved Hide resolved
ProcState(Arc::new(Inner {
dir: self.dir.clone(),
options: self.options.clone(),
emit_cache: self.emit_cache.clone(),
emit_options_hash: self.emit_options_hash,
emit_options: self.emit_options.clone(),
file_fetcher: self.file_fetcher.clone(),
http_client: self.http_client.clone(),
graph_data: Default::default(),
lockfile: self.lockfile.clone(),
maybe_import_map: self.maybe_import_map.clone(),
maybe_inspector_server: self.maybe_inspector_server.clone(),
root_cert_store: self.root_cert_store.clone(),
blob_store: Default::default(),
broadcast_channel: Default::default(),
shared_array_buffer_store: Default::default(),
compiled_wasm_module_store: Default::default(),
parsed_source_cache: self.parsed_source_cache.reset_for_file_watcher(),
maybe_resolver: self.maybe_resolver.clone(),
maybe_file_watcher_reporter: self.maybe_file_watcher_reporter.clone(),
node_analysis_cache: self.node_analysis_cache.clone(),
npm_cache: self.npm_cache.clone(),
npm_resolver: self.npm_resolver.clone(),
cjs_resolutions: Default::default(),
progress_bar: self.progress_bar.clone(),
node_std_graph_prepared: AtomicBool::new(false),
}))
}

async fn build_with_sender(
cli_options: Arc<CliOptions>,
maybe_sender: Option<tokio::sync::mpsc::UnboundedSender<Vec<PathBuf>>>,
Expand Down Expand Up @@ -256,7 +288,7 @@ impl ProcState {
.write_hashable(&emit_options)
.finish(),
emit_options,
file_fetcher,
file_fetcher: Arc::new(file_fetcher),
http_client,
graph_data: Default::default(),
lockfile,
Expand Down Expand Up @@ -291,7 +323,6 @@ impl ProcState {
lib: TsTypeLib,
root_permissions: Permissions,
dynamic_permissions: Permissions,
reload_on_watch: bool,
) -> Result<(), AnyError> {
log::debug!("Preparing module load.");
let _pb_clear_guard = self.progress_bar.clear_guard();
Expand All @@ -304,7 +335,7 @@ impl ProcState {
.map(|s| (s, ModuleKind::Esm))
.collect::<Vec<_>>();

if !reload_on_watch && !has_root_npm_specifier {
if !has_root_npm_specifier {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this branch is needed anymore, IIRC I only had problems with it because of reload_on_watch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The alternative would be making this an unconditional block. I believe we want to skip this short circuiting logic if there are npm roots since GraphData::check() doesn't account for those (this could be cleaned up in a separate PR)

let graph_data = self.graph_data.read();
if self.options.type_check_mode() == TypeCheckMode::None
|| graph_data.is_type_checked(&roots, &lib)
Expand Down Expand Up @@ -338,7 +369,6 @@ impl ProcState {
struct ProcStateLoader<'a> {
inner: &'a mut cache::FetchCacher,
graph_data: Arc<RwLock<GraphData>>,
reload: bool,
}
impl Loader for ProcStateLoader<'_> {
fn get_cache_info(
Expand All @@ -355,17 +385,14 @@ impl ProcState {
let graph_data = self.graph_data.read();
let found_specifier = graph_data.follow_redirect(specifier);
match graph_data.get(&found_specifier) {
Some(_) if !self.reload => {
Box::pin(futures::future::ready(Err(anyhow!(""))))
}
Some(_) => Box::pin(futures::future::ready(Err(anyhow!("")))),
_ => self.inner.load(specifier, is_dynamic),
}
}
}
let mut loader = ProcStateLoader {
inner: &mut cache,
graph_data: self.graph_data.clone(),
reload: reload_on_watch,
};

let maybe_file_watcher_reporter: Option<&dyn deno_graph::source::Reporter> =
Expand Down Expand Up @@ -404,7 +431,7 @@ impl ProcState {

let npm_package_reqs = {
let mut graph_data = self.graph_data.write();
graph_data.add_graph(&graph, reload_on_watch);
graph_data.add_graph(&graph);
let check_js = self.options.check_js();
graph_data
.check(
Expand Down Expand Up @@ -492,7 +519,6 @@ impl ProcState {
lib,
Permissions::allow_all(),
Permissions::allow_all(),
false,
)
.await
}
Expand All @@ -506,7 +532,7 @@ impl ProcState {
let node_std_graph = self
.create_graph(vec![(node::MODULE_ALL_URL.clone(), ModuleKind::Esm)])
.await?;
self.graph_data.write().add_graph(&node_std_graph, false);
self.graph_data.write().add_graph(&node_std_graph);
self.node_std_graph_prepared.store(true, Ordering::Relaxed);
Ok(())
}
Expand Down Expand Up @@ -743,7 +769,7 @@ pub fn import_map_from_text(
Ok(result.import_map)
}

#[derive(Debug)]
#[derive(Clone, Debug)]
struct FileWatcherReporter {
sender: tokio::sync::mpsc::UnboundedSender<Vec<PathBuf>>,
file_paths: Arc<Mutex<Vec<PathBuf>>>,
Expand Down
40 changes: 39 additions & 1 deletion cli/tests/watcher_tests.rs
Expand Up @@ -1088,6 +1088,44 @@ mod watcher {
check_alive_then_kill(child);
}

// Regression test for https://github.com/denoland/deno/issues/15465.
#[test]
fn run_watch_reload_once() {
let _g = util::http_server();
let t = TempDir::new();
let file_to_watch = t.path().join("file_to_watch.js");
let file_content = r#"
import { time } from "http://localhost:4545/dynamic_module.ts";
console.log(time);
"#;
write(&file_to_watch, file_content).unwrap();

let mut child = util::deno_cmd()
.current_dir(util::testdata_path())
.arg("run")
.arg("--watch")
.arg("--reload")
.arg(&file_to_watch)
.env("NO_COLOR", "1")
.stdout(std::process::Stdio::piped())
.stderr(std::process::Stdio::piped())
.spawn()
.unwrap();
let (mut stdout_lines, mut stderr_lines) = child_lines(&mut child);

wait_contains("finished", &mut stderr_lines);
let first_output = stdout_lines.next().unwrap();

write(&file_to_watch, file_content).unwrap();
// The remote dynamic module should not have been reloaded again.

wait_contains("finished", &mut stderr_lines);
let second_output = stdout_lines.next().unwrap();
assert_eq!(second_output, first_output);

check_alive_then_kill(child);
}

#[test]
fn run_watch_dynamic_imports() {
let t = TempDir::new();
Expand Down Expand Up @@ -1149,11 +1187,11 @@ mod watcher {
&mut stdout_lines,
);

wait_contains("finished", &mut stderr_lines);
wait_for(
|m| m.contains("Watching paths") && m.contains("imported2.js"),
&mut stderr_lines,
);
wait_contains("finished", &mut stderr_lines);

write(
&file_to_watch3,
Expand Down
3 changes: 1 addition & 2 deletions cli/tools/bench.rs
Expand Up @@ -339,7 +339,6 @@ async fn check_specifiers(
lib,
Permissions::allow_all(),
permissions,
true,
)
.await?;

Expand Down Expand Up @@ -661,7 +660,7 @@ pub async fn run_benchmarks_with_watch(
let include = selection.include.clone();
let ignore = selection.ignore.clone();
let permissions = permissions.clone();
let ps = ps.clone();
let ps = ps.reset_for_file_watcher();

async move {
let specifiers =
Expand Down
15 changes: 5 additions & 10 deletions cli/tools/run.rs
@@ -1,7 +1,6 @@
// Copyright 2018-2022 the Deno authors. All rights reserved. MIT license.

use std::io::Read;
use std::path::PathBuf;
use std::sync::Arc;

use deno_ast::MediaType;
Expand Down Expand Up @@ -100,16 +99,12 @@ async fn run_with_watch(flags: Flags, script: String) -> Result<i32, AnyError> {
let flags = Arc::new(flags);
let main_module = resolve_url_or_path(&script)?;
let (sender, receiver) = tokio::sync::mpsc::unbounded_channel();
let ps =
ProcState::build_for_file_watcher((*flags).clone(), sender.clone()).await?;

let operation = |(sender, main_module): (
tokio::sync::mpsc::UnboundedSender<Vec<PathBuf>>,
ModuleSpecifier,
)| {
let flags = flags.clone();
let operation = |main_module: ModuleSpecifier| {
let ps = ps.reset_for_file_watcher();
Ok(async move {
let ps =
ProcState::build_for_file_watcher((*flags).clone(), sender.clone())
.await?;
let permissions =
Permissions::from_options(&ps.options.permissions_options())?;
let worker =
Expand All @@ -123,7 +118,7 @@ async fn run_with_watch(flags: Flags, script: String) -> Result<i32, AnyError> {
util::file_watcher::watch_func2(
receiver,
operation,
(sender, main_module),
main_module,
util::file_watcher::PrintConfig {
job_name: "Process".to_string(),
clear_screen: !flags.no_clear_screen,
Expand Down
4 changes: 1 addition & 3 deletions cli/tools/test.rs
Expand Up @@ -959,7 +959,6 @@ pub async fn check_specifiers(
lib,
Permissions::allow_all(),
permissions.clone(),
false,
)
.await?;
}
Expand All @@ -981,7 +980,6 @@ pub async fn check_specifiers(
lib,
Permissions::allow_all(),
permissions,
true,
)
.await?;

Expand Down Expand Up @@ -1519,7 +1517,7 @@ pub async fn run_tests_with_watch(
let include = include.clone();
let ignore = ignore.clone();
let permissions = permissions.clone();
let ps = ps.clone();
let ps = ps.reset_for_file_watcher();

async move {
let specifiers_with_mode = fetch_specifiers_with_test_mode(
Expand Down
2 changes: 1 addition & 1 deletion cli/util/file_watcher.rs
Expand Up @@ -314,13 +314,13 @@ where
continue;
},
_ = operation_future => {
consume_paths_to_watch(&mut watcher, &mut paths_to_watch_receiver);
// TODO(bartlomieju): print exit code here?
info!(
"{} {} finished. Restarting on file change...",
colors::intense_blue("Watcher"),
job_name,
);
consume_paths_to_watch(&mut watcher, &mut paths_to_watch_receiver);
},
};

Expand Down
11 changes: 11 additions & 0 deletions test_util/src/lib.rs
Expand Up @@ -971,6 +971,17 @@ async fn main_server(
);
Ok(res)
}
(_, "/dynamic_module.ts") => {
let mut res = Response::new(Body::from(format!(
r#"export const time = {};"#,
std::time::SystemTime::now().elapsed().unwrap().as_nanos()
)));
res.headers_mut().insert(
"Content-type",
HeaderValue::from_static("application/typescript"),
);
Ok(res)
}
(_, "/echo_accept") => {
let accept = req.headers().get("accept").map(|v| v.to_str().unwrap());
let res = Response::new(Body::from(
Expand Down