As of before this commit, there was a lock in the codegen preventing Reference
from being used as a function argument outside of a Reference<Self>.
This changes it, allowing Reference of any class to be added as a class method
argument anywhere. It has the same limitations as reference, as in it requires
the class to have been created with a factory or constructor. This change
implements FromNapiValue on Reference, which will unwrap the class and call the
existing from_value_ptr method. It also updated typegen so that we only emit
the reference type if we're in an impl block that doesn't match the Reference
we're getting. This ensures that typegen works as expected with the previous
behaviour.
* feat(napi): keep stack traces in deferred context
* chore: reformat code
Signed-off-by: Markus <28785953+MarkusJx@users.noreply.github.com>
* chore: use napi wrappers
Signed-off-by: Markus <28785953+MarkusJx@users.noreply.github.com>
* test(napi): add test for deferred trace
Signed-off-by: Markus <28785953+MarkusJx@users.noreply.github.com>
* chore: fix format
Signed-off-by: Markus <28785953+MarkusJx@users.noreply.github.com>
---------
Signed-off-by: Markus <28785953+MarkusJx@users.noreply.github.com>
* fix: prevent crashing when napi_register_module_v1 is called twice
Currently napi-rs addons can lead to the Node.js process aborting with
the following error when initialising the addon on Windows:
```
c:\ws\src\cleanup_queue-inl.h:32: Assertion `(insertion_info.second)
== (true)' failed.
```
This happens because `napi_add_env_cleanup_hook` must not be called
with the same arguments multiple times unless the previously scheduled
cleanup hook with the same arguments was already executed. However,
the cleanup hook added by `napi_register_module_v1` in napi-rs on
Windows was always created with `ptr::null_mut()` as an argument.
One case where this causes a problem is when using the addon from
multiple contexts (e.g. Node.js worker threads) at the same
time. However, Node.js doesn't provide any guarantees that the N-API
addon initialisation code will run only once even per thread and
context. In fact, it's totally valid to run `process.dlopen()`
multiple times from JavaScript land in Node.js, and this will lead to
the initialisation code being run multiple times as different
`exports` objects may need to be populated. This may happen in
numerous cases, e.g.:
- When it's not possible or not desirable to use `require()` and users
must resort to using `process.dlopen()` (one use case is passing
non-default flags to `dlopen(3)`, another is ES modules). Caching
the results of `process.dlopen()` to avoid running it more than once
may not always be possible reliably in all cases (for example,
because of Jest sandbox).
- When the `require` cache is cleared.
- On Windows: `require("./addon.node")` and then
`require(path.toNamespacedPath("./addon.node"))`.
Another issue is fixed inside `napi::tokio_runtime::drop_runtime`:
there's no need to call `napi_remove_env_cleanup_hook` (it's only
useful to cancel the hooks that haven't been executed yet). Null
pointer retrieved from `arg` was being passed as the `env` argument of
that function, so it didn't do anything and just returned
`napi_invalid_arg`.
This patch makes `napi_register_module_v1` use a counter as the
cleanup hook argument, so that the value is always different. An
alternative might have been to use a higher-level abstraction around
`sys::napi_env_cleanup_hook` that would take ownership of a boxed
closure, if there is something like this in the API already. Another
alternative could have been to heap-allocate a value so that we would
have a unique valid memory address.
The patch also contains a minor code cleanup related to
`RT_REFERENCE_COUNT` along the way: the counter is encapsulated inside
its module and `ensure_runtime` takes care of incrementing it, and
less strict memory ordering is now used as there's no need for
`SeqCst` here. If desired, it can be further optimised to
`Ordering::Release` and a separate acquire fence inside the if
statement in `drop_runtime`, as `AcqRel` for every decrement is also a
bit stricter than necessary (although simpler). These changes are not
necessary to fix the issue and can be extracted to a separate patch.
At first it was tempting to use the loaded value of
`RT_REFERENCE_COUNT` as the argument for the cleanup hook but it would
have been wrong: a simple counterexample is the following sequence:
1. init in the first context (queue: 0)
2. init in the second context (queue: 0, 1)
3. destroy the first context (queue: 1)
4. init in the third context (queue: 1, 1)
* test(napi): unload test was excluded unexpected
---------
Co-authored-by: LongYinan <lynweklm@gmail.com>
