Repository navigation
Throw the named error type from JSError.Throw*Error(string) - #511
Open
René Larch (r-Larch) wants to merge 2 commits into
Open
René Larch (r-Larch) wants to merge 2 commits into
René Larch (r-Larch) wants to merge 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused fixes match the reported defects and include targeted regression coverage, with no unresolved findings.
No open issues 🎉
What changed in this PR
Fixes #509 by making typed JSError helpers throw the matching JavaScript error type. Includes the UTF-8 buffer fixes stacked from #510.
Changes:
- Routes typed throw helpers to their matching runtime methods.
- Explicitly terminates UTF-8 strings and disposes the module-name buffer.
- Adds regression tests for error types and buffer handling.
| File | Description |
|---|---|
| test/Utf8BufferTests.cs | Tests UTF-8 termination and null handling. |
| test/TestUtils.cs | Adds a helper to dirty pooled buffers. |
| test/NodejsEmbeddingTests.cs | Tests thrown error types, messages, and codes. |
| src/NodeApi/Runtime/Utf8StringArray.cs | Writes each string’s terminator explicitly. |
| src/NodeApi/Runtime/PooledBuffer.cs | Terminates UTF-8 buffers and distinguishes empty from null. |
| src/NodeApi/Runtime/NodejsRuntime.Embedding.cs | Disposes the module-name buffer. |
| src/NodeApi/JSError.cs | Calls the matching typed runtime throw methods. |
Review effort: Balanced
Contributor
Author
|
@microsoft-github-policy-service agree |
PooledBuffer.FromStringUtf8 and FromSpanUtf8 reserved a byte for the
terminator but never wrote it. On .NET the array comes from
ArrayPool<byte>.Shared, which is not zeroed, so native functions that take a
null-terminated const char* without a length read stale bytes past the string
(or past the array): napi_throw_error / napi_throw_type_error /
napi_throw_range_error / node_api_throw_syntax_error (code and msg),
node_embedding_runtime_config_add_module and
node_embedding_last_error_message_set. For example JSError.ThrowError("x")
produced a JS error message of "x" followed by garbage, and a linked module
could be registered under a corrupted name.
- Write the terminator explicitly in both helpers.
- Encode an empty string as just the terminator instead of returning
PooledBuffer.Empty, which pinned to a null pointer (ThrowError("") passed
msg = NULL, so napi_throw_error failed and nothing was thrown). A null
string still maps to a null pointer.
- Return the module-name buffer to the pool in
EmbeddingRuntimeConfigAddModule (Node.js copies the name).
Fixes microsoft#508.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
JSError.ThrowTypeError, ThrowRangeError and ThrowSyntaxError (with and without a code) all called JSRuntime.ThrowError, so JS received a plain Error instead of a TypeError / RangeError / SyntaxError. Call the matching runtime method instead. Fixes microsoft#509. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
René Larch (r-Larch)
force-pushed
the
fix/typed-throw-errors
branch
from
October 29, 2026 02:53
1c6e40e to
21fcb80
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #509.
JSError.ThrowTypeError,ThrowRangeErrorandThrowSyntaxError(with and withoutcode) all calledJSRuntime.ThrowError, so JS received a plainError. They now call the matching runtime method (napi_throw_type_error,napi_throw_range_error,node_api_throw_syntax_error).Tests
NodejsEmbeddingTests.ThrowTypedErrorchecksname,instanceof,messageandcodeforError,TypeError,RangeErrorandSyntaxError, with and without a code. The non-Errorcases fail onmain.Local run (Windows x64, .NET SDK 10.0.402): net9.0 134/135 and net8.0 134/135. On net10.0 everything passed except
napi-dotnet/overloads(hosted and AOT), which fails the same way onmain. I could not run net472 locally.🤖 Generated with Claude Code