Open kjvalencik opened 4 years ago
Running into this in the context of borrow
, and specifically JsBuffer
. I'd love to be able to safely borrow a JsBuffer's contents but still interact with the context in ways guaranteed not to run arbitrary code. Unfortunately I don't know how many of those operations there are in JavaScript…even something like accessing a property could turn out to be invoking a getter. (Maybe it'd be okay to assume that constructing the standard Error types isn't going to be a problem, though.)
Isn't it always the case that when you have a Context, you're either in the Locked or Throwing state? If that's so, then "&mut
for anything that can change that state" seems potentially useful (plus the ModuleContext example from above).
Any N-API function call may result in a pending JavaScript exception. This is the case for any of the API functions, even those that may not cause the execution of JavaScript.
Welp, so much for that (particular) idea.
Exclusive and Shared Context References
Purpose
The purpose of this pre-RFC is to start gathering feedback from maintainers and users about the perception of mutability inside the VM via a
Context<'_>
in order to guide a future RFC to update Neon's usage ofContext<'_>
.Summary
Neon provides the
Context<'_>
trait for abstracting interactions with the JavaScript engine. Most methods in Neon require a reference to aContext
. Some of these references are shared (&Context<'_>
) and others are exclusive (&mut Context<'_>
). However, there are some inconsistencies in when one or the other is required. For example, most methods that create JavaScript objects require&mut
while methods that tend to hold a reference to aContext<'_>
(or an underlyingIsolate
ornapi_env
) tend to use a&
.Neon should provide a consistent, opinionated API that guides users towards correct implementations with the help of the borrow checker. While I can't speak for all contributors, I know that I have lazily chose a safer exclusive reference (
&mut
) without thinking critically if it was necessary.Background
&
and&mut
in idiomatic RustIn idiomatic Rust, a shared reference
&
signals that it is safe for multiple pointers to the same data to exist concurrently. In contrast, only a single exclusive reference&mut
may exist. Typically, this aligns 1-to-1 with mutability. There can either be one mutable reference or many immutable reference.However, Rust also provides a concept of interior mutability. This allows mutating a value from a shared
&
reference. Examples include,Cell
,RefCell
, and atomics. AnAtomicU32
can be incremented with a shared&
reference because it provides internal consistency guarantees.In application code, it is generally best to only use internal mutability to abstract implementation details in implementations that appear immutable from the user. For example, memoization of a pure function.
JavaScript Engine Throwing State
Unlike Rust, JavaScript makes use of exceptions. When working with the JavaScript Engine, it can be in several states:
Calls into JavaScript from a native extension may trigger an exception. For example, calling a function that executes
throw
or attempting to read a property fromundefined
. Neon modules must be sensitive to the state of the VM.Current
Nan Backend
With the Nan backend, Neon does not prevent calls into the JavaScript engine when in a throwing exception. Making calls when in a throwing state is undefined behavior. The user must be careful to check for the
Throw
error type inResult<_, _>
and avoid using theContext<'_>
.N-API Backend
All N-API methods verify state before executing code that accesses the engine. If the engine is not in a valid state for the call, a status enum other than
napi_status::napi_ok
will be returned.Neon functions that use N-API validate the status with
assert_eq!(status, napi_status::napi_ok)
. The call to the engine will not occur, the Rust code will panic, the panic will be caught and returned to JavaScript and no undefined behavior will occur.Questions
Is throwing considered mutating
Context<'_>
?Under normal circumstances, creating a type (e.g.,
cx.string("hello")
) appears pure. A new type is created and the underlyingContext<'_>
does not change. However, it's possible for the call to transition the engine to the throwing state.In this case, an unrelated call may start failing (e.g., another
cx.string("hello")
). This may violate expectations of the user that one "pure" function should not cause another "pure" function to fail.Proposal
In my opinion, this behavior is acceptable and preferred because it mirrors patterns found in the standard library. For example,
Mutex
can be used with&
shared references, but if the lock becomes poisoned, future unrelated locks will fail. With this justification, I propose that most Neon methods should accept&Context<'_>
and not&mut Context<'_>
.Using shared references in more places improves Neon's ergonomics because user defined structs and functions may alias
Context<'_>
.When should exclusive references be used?
Context<'_>
is a trait and some implementations include additional methods and data. For example,ModuleContext<'a>
wraps the module as aJsObject
. Methods that mutate the underlyingJsObject
should take an exclusive&mut
reference. E.g.,export_function
.