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

Reintroduce Locker API (multi-thread support) #643

Open
bnoordhuis opened this issue Mar 8, 2021 · 6 comments
Open

Reintroduce Locker API (multi-thread support) #643

bnoordhuis opened this issue Mar 8, 2021 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@bnoordhuis
Copy link
Contributor

Lockers were removed in #272 but it's become clear they're necessary to access isolates from different threads and to make things like --prof work, that require an ambient isolate.

The straw-man proposal is something like this:

  1. Isolate becomes Send but is basically an opaque object with no functionality.

  2. Creating a Locker consumes or borrows the Isolate and gives you a LocalIsolate (name TBD) that's !Send + !Sync.

  3. Dropping the locker gives you back the Isolate.

There's a complementary Unlocker API for yielding the isolate. It'd be nice to have but it might be hard to model in Rust.

Stretch goal: make HandleScope::new() work with both Isolate and LocalIsolate so downstream users by and large won't have to update their code?

Would fix #486, #639, and some others. Stands a good chance of obsoleting #602. Removes the need for 83052c4.

@bnoordhuis bnoordhuis added enhancement New feature or request help wanted Extra attention is needed labels Mar 8, 2021
@nicksrandall
Copy link

I would love to help out and work on this but I'd new to this repo so I'd need some guidance. Would step 1 be reverting #272 to re-introduce the locker api?

@bnoordhuis
Copy link
Contributor Author

@nicksrandall That sounds like a good first step, yes. PR welcome!

@ry
Copy link
Member

ry commented Jun 4, 2021

@nicksrandall Help is much appreciated.

I think we need to figure out what the API would be still.

I was thinking we should have an API that consumes an OwnedIsolate to provide a Send object (which can then be sent to a different thread) and then something that consumes the object to turn it back into an OwnedIsolate.

Maybe something like

fn lock(i: v8::OwnedIsolate) -> v8::LockedIsolate;
fn unlock(i: v8::LockedIsolate) -> v8::OwnedIsolate;

@nicksrandall
Copy link

Thanks for the help, I'll get started on this. Just to make sure I understand, do we still want to use v8's locking mechanism defined here or do we want to implement the lock entirely on our end?

@ry I'll trust your opinion on this but I was thinking that for the API we might be able to attach a lock method to the v8::OwnedIsolate struct and create a new struct v8::LockedIsolate which would have a corresponding unlock method.

Something like:

let locked = owned.lock();
let owned = locked.unlock();

Should I try for the method approach or would you prefer that I stick with adding new functions to the crate?

@piscisaureus
Copy link
Member

Maybe something like

fn lock(i: v8::OwnedIsolate) -> v8::LockedIsolate;
fn unlock(i: v8::LockedIsolate) -> v8::OwnedIsolate;

IMO it’s dubious to use a consume operation for this - that kind of defeats the purpose of a lock, after all you want to be able to lock the isolate and also send it to another thread.

We have a type called IsolateHandle that implements Send/Sync. Maybe it’s better to build on that.

@ObsidianMinor
Copy link

There's a complementary Unlocker API for yielding the isolate. It'd be nice to have but it might be hard to model in Rust.

Could it modeled this using a method on a locked isolate that takes a mut ref to the lock? It wouldn't be as flexible, but it would satisfy what the API is designed for (perhaps an unsafe way to construct an unlocker on an isolate would also be provided?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants