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

Allow terminating an Isolate from another thread. #1982

Merged
merged 4 commits into from
Mar 21, 2019

Conversation

fd
Copy link
Contributor

@fd fd commented Mar 21, 2019

This allows users of deno core to forcefully terminate the execution of any script by calling terminate_execution() from a different thread. This is also an API required to implement the Web Worker Worker.prototype.terminate() function.

Note: I'm not entirely sure about calling the thread safe isolate handle SharedIsolate. (update: renamed to IsolateHandle)

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Cool!

@@ -85,6 +85,7 @@ pub trait Behavior {
/// JavaScript.
pub struct Isolate<B: Behavior> {
libdeno_isolate: *const libdeno::isolate,
shared_libdeno_isolate: Arc<Mutex<Option<*const libdeno::isolate>>>,
Copy link
Member

Choose a reason for hiding this comment

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

Should/can the Option be the outermost wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Option is atomically set to None when the Isolate is dropped. This prevents the IsolateHandle from using a disposed v8 isolate. see L100-101

Copy link
Member

Choose a reason for hiding this comment

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

Ok sounds good.

core/isolate.rs Outdated
@@ -452,6 +463,28 @@ impl<B: Behavior> Future for Isolate<B> {
}
}

/// SharedIsolate is a thread safe handle on an Isolate. It exposed thread safe V8 functions.
#[derive(Clone)]
pub struct SharedIsolate {
Copy link
Member

Choose a reason for hiding this comment

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

What about “IsolateHandle” ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IsolateHandle sounds perfect.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@fd
Copy link
Contributor Author

fd commented Mar 21, 2019

I don't think this is related to this PR but there seems to be a flaky test. I can't figure out which exact test it is though https://ci.appveyor.com/project/deno/deno/builds/23251871

@ry ry merged commit 93793dc into denoland:master Mar 21, 2019
@fd fd deleted the terminate-execution branch March 21, 2019 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants