2022-12-16 15:32:01 +09:00
|
|
|
// use the commonjs syntax to prevent compiler from transpiling the module syntax
|
|
|
|
|
|
2023-09-20 17:18:01 +09:00
|
|
|
import { createRequire } from 'node:module'
|
|
|
|
|
import * as path from 'node:path'
|
fix: prevent crashing when napi_register_module_v1 is called twice (#1554)
* 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>
2023-04-09 00:08:48 +09:00
|
|
|
|
2024-02-26 16:46:07 +09:00
|
|
|
import { platformArchTriples } from '@napi-rs/triples'
|
2023-09-20 17:18:01 +09:00
|
|
|
import test from 'ava'
|
|
|
|
|
|
|
|
|
|
const require = createRequire(import.meta.url)
|
|
|
|
|
const __dirname = path.dirname(new URL(import.meta.url).pathname)
|
2022-12-16 15:32:01 +09:00
|
|
|
|
2024-02-26 16:46:07 +09:00
|
|
|
const platforms = platformArchTriples[process.platform][process.arch]
|
|
|
|
|
|
|
|
|
|
let binaryName
|
|
|
|
|
|
|
|
|
|
if (platforms.length() === 1) {
|
|
|
|
|
binaryName = `example.${platforms[0].platformArchABI}.node`
|
|
|
|
|
} else if (process.platform === 'linux') {
|
|
|
|
|
if (process.report?.getReport?.()?.header.glibcVersionRuntime) {
|
|
|
|
|
binaryName = `example.${platforms.find(({ abi }) => abi === 'gnu').platformArchABI}.node`
|
|
|
|
|
} else {
|
|
|
|
|
binaryName = `example.${platforms.find(({ abi }) => abi === 'musl').platformArchABI}.node`
|
|
|
|
|
}
|
|
|
|
|
} else {
|
|
|
|
|
throw new Error('unsupported platform')
|
|
|
|
|
}
|
|
|
|
|
|
2022-12-16 15:32:01 +09:00
|
|
|
test('unload module', (t) => {
|
2024-02-26 16:46:07 +09:00
|
|
|
const { add } = require(`../${binaryName}`)
|
2022-12-16 15:32:01 +09:00
|
|
|
t.is(add(1, 2), 3)
|
2024-02-26 16:46:07 +09:00
|
|
|
delete require.cache[require.resolve(`../${binaryName}`)]
|
|
|
|
|
const { add: add2 } = require(`../${binaryName}`)
|
2022-12-16 15:32:01 +09:00
|
|
|
t.is(add2(1, 2), 3)
|
|
|
|
|
})
|
fix: prevent crashing when napi_register_module_v1 is called twice (#1554)
* 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>
2023-04-09 00:08:48 +09:00
|
|
|
|
|
|
|
|
test('load module multi times', (t) => {
|
2024-02-26 16:46:07 +09:00
|
|
|
const { add } = require(`../${binaryName}`)
|
fix: prevent crashing when napi_register_module_v1 is called twice (#1554)
* 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>
2023-04-09 00:08:48 +09:00
|
|
|
t.is(add(1, 2), 3)
|
2023-11-19 16:13:06 +09:00
|
|
|
const { add: add2 } = require(
|
2024-02-26 16:46:07 +09:00
|
|
|
path.toNamespacedPath(path.join(__dirname, `../${binaryName}`)),
|
2023-11-19 16:13:06 +09:00
|
|
|
)
|
fix: prevent crashing when napi_register_module_v1 is called twice (#1554)
* 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>
2023-04-09 00:08:48 +09:00
|
|
|
t.is(add2(1, 2), 3)
|
|
|
|
|
})
|
