pmed / v8pp

Bind C++ functions and classes into V8 JavaScript engine
http://pmed.github.io/v8pp/
Other
898 stars 120 forks source link

Proposal: an option to use v8pp::class_ concurrently #28

Closed ghost closed 1 year ago

ghost commented 7 years ago

I wanted to use v8pp::class_ concurrently in separate threads/isolates and realized that it shouldn’t work because class_singleton had some static variables.

thread1 = std::thread([](){
  v8::Isolate *isolate = v8::Isolate::New(...);
  v8pp::class_<Layer> Layer_class(isolate);
});

thread2 = std::thread([](){
  v8::Isolate *isolate = v8::Isolate::New(...);
  v8pp::class_<Layer> Layer_class(isolate);
});

I tried replacing all static variables with thread_local ones and then my program seemed to work well, but worried that this change may break existing code. (e.g. Sharing a v8::Isolate among threads with v8::Locker)

Think up two solution:

  1. Creates an option to use thread_local instead of static. (e.g. #define V8PP_USE_THREAD_LOCAL)
  2. Adds an optional argument to all functions under v8pp::class_ to specify a custom shared variable with any storage class.

like this:

thread1 = std::thread([](){
  v8::Isolate *isolate = v8::Isolate::New(...);
  v8pp::environment env;
  v8pp::class_<Layer> Layer_class(isolate, &env);
});

thread2 = std::thread([](){
  v8::Isolate *isolate = v8::Isolate::New(...);
  v8pp::environment env;
  v8pp::class_<Layer> Layer_class(isolate, &env);
});

Solution 1 is ad-hoc but easy. Solution 2 is more general but affects many api.

pmed commented 7 years ago

Hi Ron,

Thanks for the proposal. I've never used V8 with multiple threads and isolates, and I hope it would work.

Do you mean a static type_index class_singleton<T>::class_type member variables? I've got another issue with it. These static variables don't work with multiple DLLs, since they are declared in a header file which has been included into each DLL.

To resolve this I'm going to revert back std::type_index and language RTTI for class idetification, as it was used some time ago (before 7c1eeb429259e87d72190dcdad22678bb5e03556). This would require an additional effort from a library user to enable RTTI for Node.js addon, because node-gyp has set -fno-rtti flag by default :-/ (see nodejs/node-gyp#26)

Another static variable is instances map in a function class_singleton::class_instances(v8::Isolate* isolate) when v8::Isolate slot is not used.

This global instances map can't be thread_local. As I remember, static variables initialization is thread-safe in C++11, so I only need to protect access to the instances with std::mutex.

pmed commented 1 year ago

No further interest.