fix(rb_loader): return TYPE_THROWABLE from invoke path on Ruby exception#689
Draft
devs6186 wants to merge 1 commit intometacall:developfrom
Draft
fix(rb_loader): return TYPE_THROWABLE from invoke path on Ruby exception#689devs6186 wants to merge 1 commit intometacall:developfrom
devs6186 wants to merge 1 commit intometacall:developfrom
Conversation
When a Ruby function raises an exception, rb_protect() returns a non-zero state but the invoke path fell through to rb_type_deserialize() on Qnil, silently returning a TYPE_NULL value to the caller. The four TODO comments marking this were unfixed since the issue was opened. Replace the rb_loader_impl_print_last_exception macro (which only logged) with a static helper rb_loader_impl_exception_value() that extracts the pending exception from rb_errinfo(), builds a TYPE_THROWABLE value using exception_create_const/throwable_create/value_create_throwable — the same pattern used by py_loader_impl_error_value_from_exception() — and returns it immediately. The error is cleared with rb_set_errinfo(Qnil) so it does not propagate further up the Ruby stack. All four invoke branches (TYPED, DUCKTYPED, MIXED, zero-args) now return a structured throwable carrying the exception class name, message, and first backtrace line. Node.js callers receive a native JS Error via the existing TYPE_THROWABLE handling in node_loader_impl_value_to_napi(), and C callers can extract the exception with metacall_error_from_value(). Fixes metacall#141
df1f45c to
2fef942
Compare
Member
|
Add tests and I will merge it. |
Member
|
Can you show a screenshot of how the error looks now? |
Member
|
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.
Description
When a Ruby function raises an exception during a MetaCall invocation, the Ruby loader caught it via
rb_protect()but then fell through torb_type_deserialize()onQnil— returning a TYPE_NULL value to the caller as if the function returned nothing. Four separate// TODO: Throw exception?comments have marked this as unfixed since issue #141 was opened.This also explains issue #516 where calling a Ruby function that throws produces
[null, 0]in the Node.js caller: the Node loader receives TYPE_NULL instead of a structured error it can re-raise.Fixes #141
Root Cause
In
function_rb_interface_invoke, afterrb_protect()returnsstate != 0, the existingrb_loader_impl_print_last_exception()macro only logged the error to console. Control then fell torb_type_deserialize(rb_function->impl, result_value, &v)whereresult_valueisQnil(the fallback fromrb_protect), producing TYPE_NULL. The calling port (e.g. node_loader) had no way to distinguish "function returned nil" from "function threw an exception".Solution
Replace the log-only macro with a static helper
rb_loader_impl_exception_value()that builds aTYPE_THROWABLEvalue from the pending exception and returns it immediately, following the exact same pattern aspy_loader_impl_error_value_from_exception()inpy_loader_impl.c.The helper:
rb_errinfo()rb_obj_classname), message (#message), and backtrace entry (#backtrace[0])exception_create_const/throwable_create/value_create_throwablerb_set_errinfo(Qnil)before returningAll four
rb_protectbranches (TYPED, DUCKTYPED, MIXED, zero-args) now return early with the throwable value instead of continuing torb_type_deserialize.Changes Made
source/loaders/rb_loader/source/rb_loader_impl.crb_loader_impl_print_last_exceptionmacro (lines 348–363)rb_loader_impl_exception_value()static function using the same exception extraction pattern as the Python loaderstate != 0blocks withreturn rb_loader_impl_exception_value()Testing
The change follows the pattern already tested by
metacall_python_exception_test(which callsmetacall_error_from_value()on a TYPE_THROWABLE return from the Python loader). The equivalent Ruby test would load a script with a function that callsraise "something"and verify thatmetacall_error_from_value(ret, &ex)returns 0 andex.label == "RuntimeError". The existingmetacall_node_python_exception_testalso exercises the Node loader's TYPE_THROWABLE reception path that now applies to Ruby as well.Edge Cases
rb_errinfo()returnsQnil(exception already cleared or state was 0), the helper returns NULL — same behavior as beforerb_set_errinfo(Qnil)is called before building the exception value to avoid the pending error interfering with subsequent Ruby API callsmetacall_value_destroy()when done, consistent with other metacall return valuesType of change
Checklist