Attempted to implement Locker for Isolate.
#1411
Conversation
This adds a wrapper `SharedIsolate` for `OwnedIsolate` (this way all the support code in OwnedIsolate doesn't need to be re-created). Originally an Unlocker was added as well, but an Unlocker being used inside a `Locker` would result in HandleScope instance failing to be created once the Unlocker dropped. This may be indicating that some part of the Locker api itself isn't working properly with rusty_v8's OwnedIsolate overhead, but the example code does appear to work as expected. As `Unlocker` isn't a direct needed in my use case, it was removed in lue of implementing a bare minimum locker.
| @@ -158,6 +158,10 @@ pub struct Global<T> { | |||
| isolate_handle: IsolateHandle, | |||
| } | |||
|
|
|||
| // Globals can go between threads as long as a locker was used to acquire a SharedIsolate. | |||
| unsafe impl<T> Send for Global<T> {} | |||
| unsafe impl<T> Sync for Global<T> {} | |||
There was a problem hiding this comment.
NonNull<T> isn't send or sync, leading to this explicit implementation. IsolateHandle is already OK to my understanding.
There was a problem hiding this comment.
I'm realizing this doesn't help with the really nasty case of dropping a global while not in a locked isolate... I'm not sure what to do about that besides adding a big angry sign.
There was a problem hiding this comment.
This definitely won't work -- there's a pretty big risk to globals being passed around between threads for isolates that are single-threaded (which is the main use case for rusty_v8).
There was a problem hiding this comment.
I think there might be a concern already around globals in the same thread being used in incorrect isolates, (say one thread has two isolates, and uses the wrong global on the wrong isolate).
I think that's a use issue though, where this would have the same use risk so I'm not sure that's valid here. This does have an additional risk of disposal in a different thread, which I agree is risk though.
Is the risk here that a global was moved to another thread, then disposed while the original isolate is still alive?
src/isolate.rs
Outdated
| } | ||
|
|
||
| /// If this isolate is currently locked by the locker api to the current thread. | ||
| pub fn is_locked(&self) -> bool { |
There was a problem hiding this comment.
If someone has access to the isolate in a shared isolate, they are doing that through the locker api already. I'll remove this from OwnedIsolate.
I'll add it to IsolateHandle instead, that way it's easier to check if an isolate is entered via the thread safe handle.
…here it would be used in a thread-questionable context.
|
This should partially resolve #643, unlockers and recursive lockers are not part of this proposal. |
|
Turns out there are issues found in CI! I'll poke at that for now. |
…tack root. This makes lockers not re-entrant, but it should mean the cross-thread resources of a shared isolate are cross thread, and per-thread resources are cleaned up as lockers are dropped.
| cxx_isolate | ||
| } | ||
|
|
||
| pub(crate) fn initialize(&mut self, create_param_allocations: Box<dyn Any>) { |
There was a problem hiding this comment.
This method now handles general isolate initialization shared by snapshots, unique owned isolates, and shared isolates.
Previously the embedder data slots check was not done on snapshot isolates, which seemed odd and unintentional.
There was a problem hiding this comment.
This is probably worth splitting out into a separate PR as it seems reasonable on its own.
There was a problem hiding this comment.
I split the construction and disposal changes into #1442
| @@ -646,6 +656,32 @@ impl Isolate { | |||
| self.set_data_internal(Self::ANNEX_SLOT, annex_ptr as *mut _); | |||
| } | |||
|
|
|||
| unsafe fn dispose_annex(&mut self) { | |||
There was a problem hiding this comment.
This moved up to be near the other annex methods, and removing stacks was removed as shared and owned isolates deal with stacks differently.
|
#1442 should deal with the construction changes, simplifying this PR. |
This should make the git diff automagically update if/when denoland#1442 goes through
This adds a wrapper
SharedIsolateforOwnedIsolate(this way all the support code in OwnedIsolate doesn't need to be re-created).Originally an Unlocker was added as well, but an Unlocker being used inside a
Lockerwould result in HandleScope instance failing to be created once the Unlocker dropped.This may be indicating that some part of the Locker api itself isn't working properly with rusty_v8's OwnedIsolate overhead, but the example code does appear to work as expected.
As
Unlockerisn't a direct needed in my use case, it was removed in lue of implementing a bare minimum locker.I can investigate more into what happened to the unlocker if needed, and there are some open questions about how OwnedIsolate is dropped, where it isn't technically locked, even though drop can only happen on one thread once all lockers have been themselves dropped.