Skip to content

Conversation

@bnoordhuis
Copy link
Collaborator

Before this commit exceptions from Ruby callbacks were translated to termination exceptions that cannot be caught by JS code. Turn them into regular exceptions that can be caught.

Fixes: #357

Before this commit exceptions from Ruby callbacks were translated to
termination exceptions that cannot be caught by JS code. Turn them
into regular exceptions that can be caught.

Fixes: rubyjs#357
@tisba
Copy link
Collaborator

tisba commented Oct 5, 2025

I think the truffelruby tests are not supposed to fail 🤔

@bnoordhuis
Copy link
Collaborator Author

I disabled the test for truffleruby. Ideally truffleruby and mruby should behave the same but I don't know enough about the former to say whether that's feasible.

@bnoordhuis
Copy link
Collaborator Author

That "corrupted double-linked list" test failure on the linux-ubuntu-24.04 - ruby-3.2 - gnu buildbot is worrying but...

I tried valgrind --trace-children=yes rake test but ruby itself is so valgrind-unclean, it's useless. Just loading an empty script emits thousands upon thousands of "Conditional jump or move depends on uninitialised value(s)" warnings coming from /usr/lib/x86_64-linux-gnu/libruby-3.3.so.3.3.4 🤦

@bnoordhuis
Copy link
Collaborator Author

The error went away after a rerun. I ran valgrind locally a couple of times but I've not been able to definitely pinpoint any problems in mini_racer itself.

As I mentioned earlier, there are plenty of warnings, and mini_racer.so shows up in some of the stack traces, but the top-most frame is always some internal ruby function operating on a VALUE. I've not been able to trace any of those back to mini_racer not initializing a VALUE properly.

I can't conclusively say it's not a mini_racer issue (the data is too tenuous) but there are no obvious smoking guns.

@tisba
Copy link
Collaborator

tisba commented Oct 6, 2025

IIRC, @eregon took a look from time to time to mini_racer's truffleruby support, so I'm just pinging him for FYI.

context = MiniRacer::Context.new
context.attach("test", proc { raise "boom" })
actual = context.eval("try { test() } catch (e) { e }")
assert_equal(actual.class, MiniRacer::ScriptError)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tweaked it so the JS exception is deserialized to a MiniRacer::ScriptError Ruby exception now.

That should be implementable easily in truffleruby too, I think?

@eregon
Copy link
Contributor

eregon commented Oct 14, 2025

I'll try to take a look.
In general the code in https://github.com/rubyjs/mini_racer/blob/main/lib/mini_racer/truffleruby.rb is pretty straightforward, for example the JS exception translation to Ruby exceptions is very simple there.
Though this PR seems the reverse case, catching a Ruby exception in JS.

@eregon
Copy link
Contributor

eregon commented Oct 14, 2025

I've pushed a commit to make the test pass on TruffleRuby: main...eregon:mini_racer:fix357-and-truffleruby
(that branch doesn't contain the commit squash! no checks if truffleruby since there is no point to keep it in the git history)

…alled from JS back to Ruby for the TruffleRuby backend
@bnoordhuis
Copy link
Collaborator Author

Thanks @eregon, works great. Anyone wants to LGTM this?

Copy link
Collaborator

@tisba tisba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C related changes are quite a bit out of my area of expertise. The Ruby tests look good, no more failing tests and since you two worked on this, I'd say, let's go!

This is a really nice addition to make mini_racer's behavior less surprising. Thank you! 🙇

@bnoordhuis bnoordhuis merged commit 23ab9cc into rubyjs:main Oct 16, 2025
28 of 32 checks passed
@bnoordhuis bnoordhuis deleted the fix357 branch October 16, 2025 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

catch ruby exception (raised in attached function) within JS: change in behaviour

3 participants