fix(linter/plugins): handle errors sending data from JS back to Rust#16463
Closed
Conversation
github-actions
bot
added
A-linter
This was referenced Dec 3, 2025
Member
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves error handling in the JavaScript plugin integration by properly handling mpsc::channel send errors. Previously, these errors were silently ignored with let _ = tx.send(result). The changes replace this with proper error propagation using .map_err() that returns a NapiError, which will cause the NodeJS process to crash with an unhandled error - appropriate for these theoretically impossible but catastrophic failure cases.
- Replaces silent error ignoring with proper error handling in ThreadsafeFunction callbacks
- Adds descriptive error messages for send failures in both
setupConfigsandlintFilefunctions - Imports
NapiErrorfrom the napi crate to enable error construction
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camc314
approved these changes
Dec 3, 2025
overlookmotel
force-pushed
the
12-03-fix_linter_plugins_handle_errors_sending_data_from_js_back_to_rust
branch
from
a148597 to
71baec9
Compare
overlookmotel
force-pushed
the
12-03-fix_linter_plugins_handle_errors_sending_data_from_js_back_to_rust
branch
2 times, most recently
from
e958074 to
4bdc499
Compare
overlookmotel
changed the base branch from
12-03-refactor_linter_plugins_simplify_return_value_of_load_plugin_
to
graphite-base/16463
overlookmotel
force-pushed
the
12-03-fix_linter_plugins_handle_errors_sending_data_from_js_back_to_rust
branch
from
4bdc499 to
3da3238
Compare
overlookmotel
changed the base branch from
graphite-base/16463
to
12-03-docs_linter_plugins_improve_comment_on_error_branch
This was referenced Dec 3, 2025
Member
Author
|
Closing in favour of #16465. |
overlookmotel
deleted the
12-03-fix_linter_plugins_handle_errors_sending_data_from_js_back_to_rust
branch
graphite-app bot
pushed a commit
that referenced
this pull request
Dec 3, 2025
… callback (#16465) Alternative to #16463. I think I've understood the situation now, and why Brooooklyn's original code ignored the result of `tx.send()`. It is genuinely impossible for the call to `tx.send()` to fail, and that doesn't depend on there not being a bug in NAPI-RS - it's provable just from looking at the code in these functions, regardless of what NAPI-RS may do. Add comments explaining why we can safely ignore the result of `tx.send()`, and add a `debug_assert!` just to make sure. We want to absolutely minimize the overhead of moving between JS and Rust, so I think it's appropriate to leave out error handling for impossible cases like these.
taearls
pushed a commit
to taearls/oxc
that referenced
this pull request
Dec 11, 2025
… callback (oxc-project#16465) Alternative to oxc-project#16463. I think I've understood the situation now, and why Brooooklyn's original code ignored the result of `tx.send()`. It is genuinely impossible for the call to `tx.send()` to fail, and that doesn't depend on there not being a bug in NAPI-RS - it's provable just from looking at the code in these functions, regardless of what NAPI-RS may do. Add comments explaining why we can safely ignore the result of `tx.send()`, and add a `debug_assert!` just to make sure. We want to absolutely minimize the overhead of moving between JS and Rust, so I think it's appropriate to leave out error handling for impossible cases like these.
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
2 participants
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.
Previously if there was an error passing data back from a
ThreadsafeFunctioncallback into Rust calling thread using thempsc::channel, we'd just ignore it. Presumably then the later call torx.recv()would just hang forever waiting for a message.I'm not that familiar with
mpsc::channel, but from the docs it looks like this is impossible in these functions, because we definitely don't drop the receiver before thetx.send()call happens - because we block onrx.recv().But nonetheless, it seems wise to handle this, just in case there's a bug in NAPI-RS or something.
If
tx.send()fails, return anapi::Errorfrom the callback. This causes the error to be thrown on JS side, where it's unhandled and so will crash the NodeJS process. But this is probably what we want to happen, as there's no way to gracefully recover.(it's not documented what behavior is if you return
Errfrom the callback, but I've tried it out, and this is what happens)