Thread safety

[chendo]

Is duktape.cr known to be thread safe?

I'm attempting to use duktape with -D preview_mt, where I have a pool of prepared Duktape::Runtimes that I check out for each HTTP::Server request. However, I eventually run into an Invalid memory access (signal 11) crash under parallel requests.

I understand -D preview_mt is still alpha. I'd like to help make it work (if it's within my skillset). I'm currently not sure if it's a bug in Crystal, the pool implementation I'm using (I've tried ysbaddaden/pool and pool.cr, but both also break in odd ways) or Duktape.cr. Duktape should be able to handle threading, according to https://github.com/svaarala/duktape/blob/master/doc/threading.rst

I have tried preparing the pool of Duktape::Runtimes in serial in advance, however I still get crashes.

I suspect it a Duktape instance must only be accessed by the Crystal thread that created it, but I'm not sure how to pin it to a particular Crystal thread at the moment.

EDIT:

I've found some success by using Crystal::ThreadLocalValue and ensuring one Duktape::Runtime per system thread, however it appears any kind of Fiber yield within a push_global_proc causes Duktape::TypeErrors or crashes, even when using state = env.suspend before the yield and env.resume(state).

[grkek]

@chendo Can you show an example usage of Crystal::ThreadLocalValue?

[chendo]

# prepare in init etc
def initialize
  @runtime = Crystal::ThreadLocalValue(Duktape::Runtime).new
end

# in the method that can be run on different threads and fibers
def work
  runtime_local = @runtime.get? || @runtime.set(prepare_runtime)
end

def prepare_runtime
  Duktape::Runtime.new do |ctx|
      # set up
  end
end

[jessedoyle]

Hey @chendo - thanks so much for using duktape.cr!

I'll start out by answering the initial question:

Is duktape.cr known to be thread safe?

Unfortunately it is not. The foundation for duktape.cr was written long before multithreading in Crystal was possible. For this reason, I suspect there's at least a few instances of non-thread safe code sprinkled throughout the shard.

With this being said, I'd love to be able to support multithreaded implementations with duktape.cr! The Duktape engine has great documentation, and you've linked the relevant article about threading behaviour in the engine itself.

Pull requests that make this shard compatible with multithreading in Crystal are definitely welcomed! It's an aspect of Crystal I'm not super familiar with - but I'm happy to review and merge any changes that move the needle in a positive direction!

Thanks again!

[grkek]

# prepare in init etc
def initialize
  @runtime = Crystal::ThreadLocalValue(Duktape::Runtime).new
end

# in the method that can be run on different threads and fibers
def work
  runtime_local = @runtime.get? || @runtime.set(prepare_runtime)
end

def prepare_runtime
  Duktape::Runtime.new do |ctx|
      # set up
  end
end

How would you mix this with for example GTK?

sandbox.push_global_proc("_createButton", 1) do |ptr|
  env = ::Duktape::Sandbox.new(ptr)
  begin
    pointer = ::Box(Gtk::Widget).unbox(env.require_pointer(0))
    element = Elements::Button.new({} of String => JSON::Any, [] of Elements::Node)

    widget = element.build_widget(pointer)

    env.push_string(widget.name)
    env.call_success
  rescue exception
    raise exception
  end
end

If I run this in a multi-threaded environment using -Dpreview_mt it crashes with a segfault because it can not reallocate a pointer which has not been allocated before, meaning that Duktape is running in a separate thread.

Also how would one preserve the globalThis across multiple Duktape instances?

[chendo]

@jessedoyle thanks for making duktape.cr!

I don't have that much experience with "true" parallelism as I primarily work with Ruby (and some Swift on the side), but I can have a crack. Theoretically, wrapping anything that touches the heap with a Mutex should get most of the way, although I suspect the remaining 20% would be a struggle. For my purposes, calling Crystal code from within JS is my goal, and I suspect there's some difficulties to ensure that works as intended.

I'll fork and have a crack when I get some time next.

[chendo]

@grkek My use case is different to yours so I'm not sure how you'd adapt the code I have provided... Have you confirmed the GTK library is threadsafe?

[grkek]

@grkek My use case is different to yours so I'm not sure how you'd adapt the code I have provided... Have you confirmed the GTK library is threadsafe?

I mean they mention something about sticking GTK to one thread

[jessedoyle]

I tried out the test suite with a multithreaded build and everything is passing right now.

i.e. Adding -Dpreview_mt to the build here, then running make spec.

Not sure if I'm missing something super basic, but I think a failing test case would really help isolate the problem areas. Do you think we'd be able to write a test case that fails (even if it segfaults)?

[chendo]

I've added a simple spec to mine that I was able to fix the segfaults by wrapping a mutex around the relevant call, although I suspect there might be an easier way to shoehorn locks in by putting a proxy in front of LibDUK: https://github.com/chendo/duktape.cr/tree/thread-safety

I've managed to avoid some segfaults so far on my project by wrapping Mutexes around the right calls so far, trying to see how far I can take it without needing to patch Duktape.cr, although I suspect I will run into issues sooner or later; will keep you posted.

[jessedoyle]

Thanks for the branch @chendo! I'm noticing a segfault when running the failing spec even without -D preview_mt. Make me wonder if there's an issue in the C bindings somewhere...