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

Support multiple Duktape contexts with a shared heap #4

Open
kylewlacy opened this issue Oct 19, 2019 · 4 comments
Open

Support multiple Duktape contexts with a shared heap #4

kylewlacy opened this issue Oct 19, 2019 · 4 comments

Comments

@kylewlacy
Copy link
Contributor

Currently, Ducc::new() always creates a new Duktape heap for the context. However, this means values cannot be shared across contexts except through serialization.

The Duktape guide has a section on contexts and heaps that describes how to create a context using a shared heap with duk_push_thread or duk_push_thread_new_globalenv.

For prior art, the v8-rs crate exposes a similar concept with the Context and Isolate types. So, library users would first call Isolate::new() to create a new isolated v8 instance (analogous to a Duktape heap), then Context::new(&isolate) to create the context (analogous to a Duktape context).

@kylewlacy
Copy link
Contributor Author

First, I want to say that I love Ducc! It's the Duktape library I didn't know I was waiting for, and it was really easy for me to pick up and use

Anyway, for motivation on this issue: I'm trying to implement a CommonJS/NodeJS-style module system in Duktape. Duktape has an official answer on modules, but I feel like the recommended approach is flawed (their official implementation literally wraps the module code in (function(require, exports, module) { /* module code */ })(...)). I think running each module in a separate context and using a shared heap for the module is a more promising avenue.

I'd be willing to take a stab at implementing this in a PR, but I think there's multiple different ways to tackle this from an API perspective (all of which have trade-offs). A few possible solutions I've thought about:

  1. Add a lifetime to Ducc, add new ducc.thread()/ducc.thread_new_global() instance methods that use temporary lifetimes. This would unfortunately be a breaking change since existing code would have to be updated to use Ducc<'static> throughout.
  2. Add a new Heap type, update Ducc with an Rc<Heap> field. Ducc::new() stays the same but ducc.thread() just clones Rc<Heap>. This should be a non-breaking change, but using reference counting is not zero-cost.
  3. Add a ducc.with_thread() method that takes a closure which would take &Ducc. I think this would feel a bit hacky, but dealing with a closure should make it easier to implement without significantly impacting the rest of the library.

I also don't know much about the safety implications of duk_push_thread so it's hard for me to say if there are any potential unsoundness issues with any particular approach.

@kylewlacy
Copy link
Contributor Author

I pushed up a WIP/proof-of-concept branch with a basic implementation for this issue: wip/duktape-threading. Unfortunately it segfaults in one of the tests I added so there's still some work to do before it's actually useable!

I based it on the with_thread style I mentioned above. It seems to work well enough for my use case, but I'm not convinced it's the best API interface

@SkylerLipthay
Copy link
Owner

Thanks for giving this thought. It's one of those things that never came up in my usage but I knew it'd be important in many other cases. I'll help give this some careful study over the next few days (sorry, just a bit busy here) but please feel free to keep iterating and to continue updating this issue!

@kylewlacy
Copy link
Contributor Author

Minor update: I fixed the segfault (it happened because I hadn't set udata or anymap in the newly-created thread context). So, the branch now works at least. I'll leave it at that for now, but do let me know if you have any thoughts/suggestions/proposals and I can work on prototyping it out! I can make a PR when I have a bit more certainty on what the final form of the API should look like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants