Skip to content

Drop the host's shadowing implementations of runtime Node-API functions #428

Description

@kraenhansen

Follow-up from the #56 sweep (see #61, #62, #63, #66, #68, #65, #67).

Since we adopted Hermes' first-party Node-API (#372) and implemented hermes_napi_host (#398), Hermes implements the entire runtime surface itself — including buffers, napi_fatal_error, napi_get_version and napi_get_node_version. But packages/host/cpp/RuntimeNodeApi.{cpp,hpp} still defines eight of those functions, and they win: the generated injector resolves its struct initializers inside namespace callstack::react_native_node_api (packages/host/scripts/generate-injector.mts), and RuntimeNodeApi.hpp is included there, so unqualified lookup finds the host's definition and never reaches the symbol Hermes exports.

In other words the shims — written when Hermes had no Node-API of its own — are what addons call today, and they now diverge from the engine we delegate everything else to.

What shadows what

Function Host (RuntimeNodeApi.cpp) Hermes (API/napi/) Suggestion
napi_create_buffer ArrayBuffer + Uint8Array view hermes_napi_buffer.cpp, same shape drop
napi_create_buffer_copy as above + memcpy, never writes result_data hermes_napi_buffer.cpp, writes it drop
napi_create_external_buffer external ArrayBuffer + view hermes_napi_buffer.cpp drop
napi_get_buffer_info accepts any ArrayBuffer/TypedArray, napi_ok + zeroes for anything else napi_invalid_arg for non-Uint8Array, like Node drop
napi_is_buffer true for any ArrayBuffer/TypedArray true only for Uint8Array, like Node drop (also tightens #171)
napi_get_version *result = NAPI_VERSION identical drop
napi_get_node_version napi_generic_failure Hermes version, release name "hermes" drop — decided in #67
napi_fatal_error logs via the host logger, then abort() hermes_fatalllvh::report_fatal_error → stderr keep, see below

Two bugs the shims carry today

Both disappear with the shims, which is the main reason to do this rather than leave it as tidying:

  1. ArrayType is a mutable global that napi_get_buffer_info overwrites. RuntimeNodeApi.cpp:6 declares auto ArrayType = napi_uint8_array; at global scope, and napi_get_buffer_info passes &ArrayType as the out type parameter of napi_get_typedarray_info (RuntimeNodeApi.cpp:98). Since the host's napi_is_buffer treats every typed array as a buffer, an addon calling napi_get_buffer_info on, say, a Float64Array leaves ArrayType == napi_float64_array — and every subsequent napi_create_buffer / napi_create_external_buffer then produces a Float64Array (with element counts read as byte lengths) instead of a Uint8Array. It is also a plain data race once more than one runtime is alive.
  2. napi_create_buffer_copy ignores result_data. The parameter is accepted and never written (RuntimeNodeApi.cpp:25-41), so an addon that passes a non-NULL result_data — Node documents it as optional, i.e. skippable by passing NULL, not as ignorable by the implementation — reads back uninitialized memory. Hermes writes it.

Why napi_fatal_error should stay

Hermes routes it to hermes::hermes_fatalllvh::report_fatal_error, which writes to stderr. On Android stderr is not logcat, so the message would be lost exactly when it matters most; the host's version goes through log_error and reaches logcat with the NodeApiHost tag (packages/host/cpp/Logger.cpp). Worth keeping the shim and commenting why it deliberately shadows Hermes', so the next sweep doesn't remove it as dead weight.

Suggested steps

  • Remove the five buffer functions and napi_get_version from RuntimeNodeApi.{cpp,hpp}.
  • Remove napi_get_node_version as well — decided in Implement napi_get_node_version #67: Hermes answers with its own version instead of the current napi_generic_failure. Closes Implement napi_get_node_version #67, and wants a changeset, since it is an observable behaviour change for addons.
  • Keep napi_fatal_error, with a comment explaining the shadowing is intentional (logcat).
  • Confirm packages/node-addon-examples/tests/buffers still passes on device — the stricter napi_is_buffer / napi_get_buffer_info are a behaviour change, and the port of Node's test_buffer is where that shows up.
  • If everything but napi_fatal_error goes, consider whether RuntimeNodeApi.{cpp,hpp} still earns its own file or folds into Logger-adjacent code, and update packages/host/android/CMakeLists.txt accordingly.

Not filed as a sub-issue of #56, since that umbrella tracks implementing the functions rather than cleaning up after them — happy to attach it if you'd rather keep them together.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

AutomatableAn issue we expect to be fixed using automation.C++Host 🏡Our `react-native-node-api-modules` package

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions