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: dynamic import panic #2792

Merged
merged 10 commits into from Aug 21, 2019
@@ -440,10 +440,7 @@ impl Isolate {
libdeno::deno_mod_new(self.libdeno_isolate, main, name_ptr, source_ptr)
};

self.check_last_exception().map(|_| id).map_err(|err| {
assert_eq!(id, 0);
err
})
self.check_last_exception().map(|_| id)
This conversation was marked as resolved by bartlomieju

This comment has been minimized.

Copy link
@ry

ry Aug 19, 2019

Collaborator

Since this is a bug in core, it should be testable with a unit test in core.

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Aug 19, 2019

Author Contributor

I extended existing dyn_import_err unit test to demonstrate that you can do multiple dynamic imports that end with error.

}

pub fn mod_get_imports(&self, id: deno_mod) -> Vec<String> {
@@ -503,10 +500,7 @@ impl Isolate {
err_str_ptr,
)
};
self.check_last_exception().map_err(|err| {
assert_eq!(id, 0);
err
})
self.check_last_exception()
}

fn poll_dyn_imports(&mut self) -> Poll<(), ErrBox> {
@@ -1012,6 +1006,60 @@ pub mod tests {
})
}

#[test]
fn dyn_import_err2() {
This conversation was marked as resolved by ry

This comment has been minimized.

Copy link
@ry

ry Aug 19, 2019

Collaborator

This test passes on master

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Aug 21, 2019

Author Contributor

Yes... So the error I encountered was raised during dyn_import_done (SyntaxError), I'll rewrite this test case to raise the same error.

use std::convert::TryInto;
// Import multiple modules to demonstrate that after failed dynamic import
// another dynamic import can still be run
run_in_task(|| {
let count = Arc::new(AtomicUsize::new(0));
let count_ = count.clone();
let mut isolate = Isolate::new(StartupData::None, false);
isolate.set_dyn_import(move |_, specifier, referrer| {
let c = count_.fetch_add(1, Ordering::Relaxed);
match c {
0 => assert_eq!(specifier, "foo1.js"),
1 => assert_eq!(specifier, "foo2.js"),
2 => assert_eq!(specifier, "foo3.js"),
_ => unreachable!(),
}
assert_eq!(referrer, "dyn_import_error.js");

let source_code_info = SourceCodeInfo {
module_url_specified: specifier.to_owned(),
module_url_found: specifier.to_owned(),
code: "# not valid JS".to_owned(),
This conversation was marked as resolved by ry

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Aug 21, 2019

Author Contributor

@ry I updated the test and verified that it fails on master. This bit # not valid JS represents the same situation as when that panic was encountered, ie. importing non-JS file

};
let stream = MockImportStream(vec![
Ok(RecursiveLoadEvent::Fetch(source_code_info)),
Ok(RecursiveLoadEvent::Instantiate(c.try_into().unwrap())),
]);
Box::new(stream)
});

js_check(isolate.execute(
"dyn_import_error.js",
r#"
(async () => {
await import("foo1.js");
})();
(async () => {
await import("foo2.js");
})();
(async () => {
await import("foo3.js");
})();
"#,
));

assert_eq!(count.load(Ordering::Relaxed), 3);
// Now each poll should return error
assert!(isolate.poll().is_err());
assert!(isolate.poll().is_err());
assert!(isolate.poll().is_err());
This conversation was marked as resolved by ry

This comment has been minimized.

Copy link
@ry

ry Aug 19, 2019

Collaborator

Great thanks! But can we leave the existing test and just copy&paste this into a new one called dyn_import_err2 ?

})
}

#[test]
fn dyn_import_ok() {
run_in_task(|| {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.