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

Add APIs that convert between Context and napi_env #801

Closed
wants to merge 9 commits into from

Conversation

branchseer
Copy link
Contributor

Closes #611.

with_raw_env is useful when neon is not used as the "entry" of a native module, so after getting a raw napi_env we need the unsafe API to bootstrap a neon::Context.

Copy link
Member

@kjvalencik kjvalencik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patr0nus Thanks for the PR! Can you share a little more details about how you see with_raw_env being used? My concern is that it locks Neon into fairly strict stability guarantees. Neon would no longer be able to guarantee it's the entry point and therefore needs to be careful about destructive operations performed when the scope drops.

If the only current need is for testing purposes, we should be able to test using some neon_runtime APIs directly.

@@ -294,6 +294,12 @@ impl<'a> Lock<'a> {
///
/// A context has a lifetime `'a`, which ensures the safety of handles managed by the JS garbage collector. All handles created during the lifetime of a context are kept alive for that duration and cannot outlive the context.
pub trait Context<'a>: ContextInternal<'a> {
/// Get the underlying `napi-env` of the context
#[cfg(feature = "napi-1")]
fn to_raw_env(&self) -> *mut c_void {
Copy link
Member

@kjvalencik kjvalencik Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would favor a more idiomatic name on the public API instead of matching internal naming. What do you think of as_mut_ptr?

Additionally, this function should take &mut self. Even though the caller still needs to uphold borrow rules, it would always be undefined behavior to use the napi_env while a mutable reference to Context is held.

@branchseer
Copy link
Contributor Author

@kjvalencik

My usage of with_raw_env is to pass Context across the ffi boundary. I am writing a native module that depends two static libraries, one targeting msvc and the other targeting mingw. Since they are two different ABIs I have to split the module into 2 shared libraries. Let's called them "a.node" and "b.dll".

  • "a.node" is compiled with msvc so it can use the msvc library. Also it's the entry of the native module.
  • "b.dll" is compiled with mingw so it can use the mingw library. It exports c functions which are called by "a.node".

I want to use neon in both the shared libraries. But "b.dll" isn't the entry point, so to create Context on its side I must pass a raw pointer from "a.node" and use with_raw_env the reconstruct the context.

I understand your concern about the entry point but for my case it's not a problem (neon is still used as the entry point in a.node). What do you think we remove the mentions of napi_env in the docs, and requires the ptr passed in with_raw_env (should rename to with_raw?) to be the one that returned from as_mut_ptr, not just any napi_env?

@kjvalencik
Copy link
Member

kjvalencik commented Sep 20, 2021

That's an interesting use case. Unfortunately, Context is more than a simple wrapper around napi_env. A good portion of that is unnecessary with the Node-API backend, but it would need to be removed before constructing a context across FFI could be safe. I also wouldn't want to overload TaskContext for this, but would want to create some type of FFI context dedicated to it where we can explicitly mark some guarantees. Ideally a user would only get &mut access to it.

In your example, it seems like one crate could own all Neon functions while the other exposes C FFI, but I'm sure the actual code is much more complicated.

@branchseer
Copy link
Contributor Author

Yeah it's not practical for my case to isolate neon in one crate :(

But I think I come up with a solution. The general idea is to represent napi_env with an FFI-safe opaque type RawEnv, and of course create contexts dedicate to FFI (RawContext). The bonus part is no more unsafe APIs, although callers would most definitely use unsafe.

I have pushed the commits. Let me know what you think :)

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

Successfully merging this pull request may close these issues.

Get a raw napi_env from Context for use in N-API methods not implemented by Neon.
2 participants