Only one thread can access the same context at a given time

[jeromegn]

This was non-obvious to me since I have experience with V8 which takes care of synchronization for you.

Before setting a context, you might need to unset the current one. Which is what your ContextGuard does as it is dropped. Except it doesn't prevent another thread from trying to set the same context concurrently.

I was thinking an Arc<Mutex<()>> might work for entering a context.

I'm not sure what's the logic with setting the "previous" context as the current context from the Context#exit method. I'm assuming people won't be using more than 1 context per runtime, so inner guards are probably infrequent.

Any ideas?

[darfink]

One of the primary reasons this library is not yet stable is specifically due to its handling of execution in multi-threaded environments. This is indeed an issue, and using an Arc<Mutex<()>> for contexts may be a solution. I still have some uncertainty as to how ChakraCore behaves in different multi-threaded situations, which I need to investigate further.

I personally believe multiple contexts per runtime is a common use case. I have used it to isolate the execution environments between different "plugins" within the same runtime.

The current context implementation, as you may have noticed, acts as a stack, albeit I'm not certain this implementation is the one preferred.

[jeromegn]

OT: It's a good crate! It's been working pretty great for me. Thanks for the hard work on it :)

Oh maybe multiple contexts is a common thing. I'm assuming too much.

In V8, there's a Locker which you acquire before doing anything with a runtime (isolate) in a multi-threaded environment. It'll act like JsAdd in the sense that it increments a counter for the number of "enters" on the lock within the same thread. Then when you unlock (or the Locker drops), it decrements the counter (like JsRelease) and relinquishes the lock for any other thread.

A similar logic would probably work well here. Locking for every context use might get very slow and impractical (for nested uses of the same context for example.)

[darfink]

Thanks for the kind words :) That's actually a great suggestion, a ReentrantMutex might just be the solution for this use case. I'll see if I can come up with a viable implementation.

[jeromegn]

Oh, yes, that's the one! I didn't know how that kind of mutex was called.

[jeromegn]

Implemented this locally. Seems to help, but I'm not sure how to handle all the Context::from_raw which essentially would require me to make a new ReentrantMutex (because I'm storing it in the struct)

I suppose it would be possible to have a hash map of JsRef : ReentrantMutex instead of storing in the struct. Not entirely sure how to make JsRef hashable (I'm pretty new at this rust thing)

[darfink]

It would require a bit of an overhaul. Data can be associated with each context (see ContextData), which could in turn contain a reentrant mutex that is shared between all contexts. Although the user data functions and friends would also have to be moved to the context guard to ensure thread-safety.

[jeromegn]

I'm currently testing this with a tokio multi-threaded event loop. I made a setTimeout native function that uses it.

I added a lazy_static hash map like:

lazy_static! {
    static ref CONTEXT_MUTEXES: Mutex<HashMap<JsRef, ReentrantMutex<()>>> =
        Mutex::new(HashMap::new());
}

and I've changed Context#enter to this:

    /// Sets the current context.
    fn enter(&self) -> Result<()> {
        let mutexes = CONTEXT_MUTEXES.lock().unwrap();
        match mutexes.get(&self.as_raw()) {
            Some(m) => {
                let _guard = m.lock();
                jstry(unsafe { JsSetCurrentContext(self.as_raw()) })
            }
            None => panic!("mutex necessary"),
        }
    }

I insert the mutex in Context::new.

I believe the mutex works as intended, unfortunately I still get frequent JsErrorWrongThread.

I'm not sure how it's possible given this mutex!

Data can be associated with each context (see ContextData), which could in turn contain a reentrant mutex that is shared between all contexts.

All contexts? I think it has to be per-context. I might be wrong about that though.

Does the user data need to be context guarded to be accessed? If not, we could definitely use that for the mutex. If the user data functions needs to be guarded by the context guard, I'm not sure it's possible to it to store the mutex since we need to mutex to guard the context. Unless I misunderstand 😄

This is the kind of backtrace I get:

<backtrace stuff>
   5:        0x10b780ead - chakracore::error::Error::from_kind::hbc59a2660e1d8005
                        at /Users/jerome/projects/github.com/jeromegn/chakrafun/<error_chain_processed macros>:52
   6:        0x10b780ff6 - <chakracore::error::Error as core::convert::From<alloc::string::String>>::from::hf058b664a95fe616
                        at chakracore-rs/src/error.rs:2
   7:        0x10b779ece - <T as core::convert::Into<U>>::into::h4b936bf92e2f09e3
                        at /Users/travis/build/rust-lang/rust/src/libcore/convert.rs:396
   8:        0x10b783680 - chakracore::util::jstry::h907e4749c9ab3244
                        at chakracore-rs/src/util.rs:65
   9:        0x10b778413 - chakracore::context::Context::enter::h7d408589bdc03755
                        at chakracore-rs/src/context.rs:225
  10:        0x10b777c29 - chakracore::context::Context::make_current::hed0afe6d4c5c4154
                        at chakracore-rs/src/context.rs:79
  11:        0x10b528985 - chakracore::context::Context::exec_with::h72978845c41b1573
<snip -> some tokio and futures stuff>

[darfink]

In your case, the mutexts only lock the thread during the execution of enter (i.e it's released and dropped as soon as the enter function is finished), therefore another call to JsSetCurrentContext can be performed as soon as it exits.

From my understanding, only one context can be used at a time, from any thread. If that is the case, an Arc<ReentrantMutex<_>> should be used that is shared between all contexts, and locked as long as the ContextGuard is in scope.

[jeromegn]

In your case, the mutexts only lock the thread during the execution of enter (i.e it's released and dropped as soon as the enter function is finished), therefore another call to JsSetCurrentContext can be performed as soon as it exits.

Whoops, that's right. My brain must've been fried.

From my understanding, only one context can be used at a time, from any thread.

I think that might be per JsRuntime. For instance, if you had 2 runtimes, each with 1 context, you could use them from different threads at the same time. You couldn't use 2 contexts (or the 1 context) at the same time from different threads though.

I'm not sure about all of that, I'm asking around in https://gitter.im/Microsoft/ChakraCore

[darfink]

Sorry for being a bit vague, I meant of course that the Arc<ReentrantMutex<_>> should be shared between all contexts per runtime. But if you receive any feedback at their Gitter, it would be great to have it confirmed.

[jeromegn]

Sorry for being a bit vague

No worries, you weren't. I have a specific use-case in mind that uses multiple runtimes and a single context per runtime. A multi-threaded http server passing requests to Chakra. It uses a multi-threaded event loop.

If you'd like, I'm happy to implement this change with some guidance. Which reminds me I should probably create 1-2 PRs. Locally, I have a more complete Promise API implementation as well as a partial ES Modules implementation.

[jeromegn]

From Gitter: https://gitter.im/Microsoft/ChakraCore?at=5b757c64a6af14730b433141

TL;DR: only 1 context can be active per thread at any given time, regardless of which runtime it belongs to.

This means we'd need to share a mutex across all contexts (regardless of runtime.)

[darfink]

This was definitely a bit unexpected. With that said, a mutex across all contexts should not be required. It's enough to use thread-local variable (TLS) which tracks the active runtime on the current thread, combined with a shared Arc<ReentrantMutex<_>> between contexts (as before, i.e per runtime) to ensure a runtime is only accessed sequentially.

[jeromegn]

I'm not sure the Runtime matters here. Looks like it's only a Context thing.

Brain dump:

• We don't need to track which context is active on the current thread because we can always get it with JsGetCurrentContext
• We may need to unset the current context as soon as we're done with it with JsSetCurrentContext(JS_INVALID_REFERENCE) or else a lingering active context could be on the current thread (I'm not 100% sure how it works in this project yet though, I see it sets the previous context when it's done with the current one, until it gets to the end which is null, so that might be fine already)
• Probably need a ReentrantMutex for each thread still, maybe as in the TLS as you mentioned.
• Need a more global mutex to make sure the same context is not activated in a different thread already. That could deadlock though.

[jeromegn]

I fixed the issue for my own use case.

Using tokio 0.1.8 (just released). For every runtime:

• I spawn a new thread that runs a tokio with the CurrentThread "runtime"
• I use the new feature released to send out the tokio runtime's "Handle" and store it
• I spawn futures with that handle to queue things in the event loop which always runs futures in the JS runtime's thread, always the same one.