fix(watch): preserve ProcState::file_fetcher between restarts
#15466
Conversation
There was a problem hiding this comment.
@nayeemrmn this looks good to me, but could we test it somehow?
|
Looks like the test from this PR is hanging frequently on either debug or release linux, |
|
Yeah I don't know what's happening with this test. It hangs on one different job each time, but when I put an explicit panic at the end of the function it gets hit. @bartlomieju Anything you can see that's wrong with the test? |
|
@bartlomieju Finally figured out the problem: 8d58477, debugging with timestamps shows that the file modification in the test can happen between these lines and I guess the watcher misses the event because of that. Files should be watched before printing the finished message |
|
@nayeemrmn nice, I will try to review the PR tonight |
| @@ -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 { | |||
There was a problem hiding this comment.
I'm not sure if this branch is needed anymore, IIRC I only had problems with it because of reload_on_watch
There was a problem hiding this comment.
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)
|
Thanks for fixing the PR Nayeem; I will review it after the holiday break. |
nayeemrmn
force-pushed
the
watch-file-fetcher
branch
from
9d0bed2
to
8e57648
Compare
| @@ -1355,12 +1354,14 @@ pub async fn run_tests_with_watch( | |||
| let no_check = ps.options.type_check_mode() == TypeCheckMode::None; | |||
| let test_options = &test_options; | |||
|
|
|||
| let ps = RefCell::new(ps); | |||
There was a problem hiding this comment.
Now that I'm seeing it I'm a bit worried this might cause unexpected bugs - since ProcState is internally an Arc a lot of APIs could have cloned it and hold onto a copy of that Arc. Even if we create a new Arc in reset_for_file_watcher some code might hold onto the old copy. Does that make sense? Are am I overanalyzing it and essentially anything that can hold onto ProcState gets torn down between restarts?
There was a problem hiding this comment.
Well ProcState's fields are either (1) immutable config, (2) mutable data that gets defaulted on reset, or (3) interior mutable fields with their own Arcs which are preserving-ly cloned on reset. If there were an old lingering ProcState inner, the issue would be if (2) and (3) started conflicting since (3) is being affected by a future run. That can be avoided by being strict about what fields are (3) -- should be strictly additive stuff that won't be overwritten between resets. If there's an bug it should just mean that more fields need to be defaulted on reset.. I think
There was a problem hiding this comment.
Okay, makes sense. Thanks for investigating
This commit changes "ProcState" to store "file_fetcher" field in an "Arc", allowing it to be preserved between restarts and thus keeping the state alive between the restarts. File watchers for "deno test" and "deno bench" now reset "ProcState" between restarts.
This commit changes "ProcState" to store "file_fetcher" field in an "Arc", allowing it to be preserved between restarts and thus keeping the state alive between the restarts. File watchers for "deno test" and "deno bench" now reset "ProcState" between restarts.
…ts (denoland#15466)" This reverts commit 3545bff.
Fixes #15465.