Incorrect fatal exception throwing in panic handler

[uvlad7]

Comment in errror.rs states:

    /// The Ruby Exception will be `fatal`, terminating the Ruby process, but
    /// allowing cleanup code to run.

so I assume you wanted the same effect as C rb_fatal, but in fact you are calling rb_raise(rb_eFatal), which is not the same

[uvlad7]

I created an example for illustration inside rust_blank project

diff --git a/examples/rust_blank/ext/rust_blank/src/lib.rs b/examples/rust_blank/ext/rust_blank/src/lib.rs
index 26ba392..ac9dcb0 100644
--- a/examples/rust_blank/ext/rust_blank/src/lib.rs
+++ b/examples/rust_blank/ext/rust_blank/src/lib.rs
@@ -1,9 +1,5 @@
-use magnus::{
-    encoding::{CType, RbEncoding},
-    method,
-    prelude::*,
-    Error, RString, Ruby,
-};
+use magnus::{encoding::{CType, RbEncoding}, method, prelude::*, Error, RString, Ruby, function};
+use std::ffi::CString;
 
 fn is_blank(rb_self: RString) -> Result<bool, Error> {
     // RString::as_str is unsafe as it's possible for Ruby to invalidate the
@@ -35,9 +31,42 @@ fn is_blank(rb_self: RString) -> Result<bool, Error> {
     Ok(true)
 }
 
+fn rs_panic() -> Result<(), Error> {
+    // Creates exception_fatal under the hood
+    panic!("rust panic");
+}
+
+fn rs_fatal(ruby: &Ruby) -> Result<(), Error> {
+    // https://docs.ruby-lang.org/capi/en/master/d3/d57/eval_8c_source.html#l00676
+    //  void
+    //  rb_exc_raise(VALUE mesg)
+    //  {
+    //      rb_exc_exception(mesg, TAG_RAISE, Qundef);
+    //  }
+    //
+    //  void
+    //  rb_exc_fatal(VALUE mesg)
+    //  {
+    //      rb_exc_exception(mesg, TAG_FATAL, Qnil);
+    //  }
+    // rb_raise calls rb_exc_raise, rb_fatal calls rb_exc_fatal
+    // https://docs.ruby-lang.org/capi/en/master/db/dcc/error_8c_source.html#l03679
+    // https://docs.ruby-lang.org/capi/en/master/db/dcc/error_8c_source.html#l03628
+    // so this code is like calling rb_raise(rb_eFatal, ...), not rb_fatal(...)
+    Err(Error::new(ruby.exception_fatal(), "magnus fatal"))
+}
+
+fn rb_fatal() -> Result<(), Error> {
+    let s = CString::new("ruby fatal").unwrap();
+    unsafe { rb_sys::rb_fatal(s.as_ptr()) }
+}
+
 #[magnus::init]
 fn init(ruby: &Ruby) -> Result<(), Error> {
     let class = ruby.define_class("String", ruby.class_object())?;
+    ruby.define_global_function("rs_panic",  function!(rs_panic, 0));
+    ruby.define_global_function("rs_fatal",  function!(rs_fatal, 0));
+    ruby.define_global_function("rb_fatal",  function!(rb_fatal, 0));
     class.define_method("blank?", method!(is_blank, 0))?;
     Ok(())
 }

test this with magnus_fatal.rb

require 'rust_blank'

at_exit do
  puts '-----------------'
  puts 'at_exit'
end

def test(name, &block)
  puts '-----------------'
  puts "test: #{name}"
  begin
    block.call
  rescue Exception => e
    puts "rescued #{e.message} (#{e.class})"
  ensure
    puts "ensure: #{name}"
  end
  puts "done: #{name}"
end

test('1') { rs_panic }

test('2') { rs_fatal }

test('3') { rb_fatal }

outputs

$ ruby -I lib magnus_fatal.rb 
-----------------
test: 1
thread '<unnamed>' panicked at 'rust panic', src/lib.rs:36:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
rescued rust panic (fatal)
ensure: 1
done: 1
-----------------
test: 2
rescued magnus fatal (fatal)
ensure: 2
done: 2
-----------------
test: 3
ensure: 3
-----------------
at_exit
magnus_fatal.rb:25:in `rb_fatal': ruby fatal (fatal)
	from magnus_fatal.rb:25:in `block in <main>'
	from magnus_fatal.rb:12:in `test'
	from magnus_fatal.rb:25:in `<main>'

So, only the last method works like rb_fatal, the rest are rb_raise.

You can see the difference with fatal.rb

require 'ffi'
require 'fiddle'

module RbFFI
  extend FFI::Library
  ffi_lib FFI::CURRENT_PROCESS
end

RbFFI.attach_function :rb_define_class, [:string, :ulong], :ulong
RbFFI.attach_function :rb_raise, [:ulong, :string, :varargs], :void
RbFFI.attach_function :rb_fatal, [:string, :varargs], :void

rb_eException = Fiddle.dlwrap Exception # Just a hacky way to convert get VALUE representation of Ruby object
rb_eFatal = RbFFI.rb_define_class('fatal', rb_eException)
fatal = Fiddle.dlunwrap(rb_eFatal) # Just a hacky way to convert get Ruby object from VALUE representation

at_exit do
  puts '-----------------'
  puts 'at_exit'
end

def test(name, &block)
  puts '-----------------'
  puts "test: #{name}"
  begin
    block.call
  rescue Exception => e
    puts "rescued #{e.message} (#{e.class})"
  ensure
    puts "ensure: #{name}"
  end
  puts "done: #{name}"
end

test('1') { RbFFI.rb_raise rb_eFatal, 'rb_raise rb_eFatal' }

test('2') { raise fatal, 'raise fatal' }

test('3') { RbFFI.rb_fatal 'rb_fatal' }

outputs

$ ruby fatal.rb 
-----------------
test: 1
rescued rb_raise rb_eFatal (fatal)
ensure: 1
done: 1
-----------------
test: 2
rescued raise fatal (fatal)
ensure: 2
done: 2
-----------------
test: 3
ensure: 3
-----------------
at_exit
/path/to/ruby/3.0.2/lib/ruby/gems/3.0.0/gems/ffi-1.16.3/lib/ffi/variadic.rb:47:in `invoke': rb_fatal (fatal)
	from /path/to/ruby/3.0.2/lib/ruby/gems/3.0.0/gems/ffi-1.16.3/lib/ffi/variadic.rb:47:in `call'
	from /path/to/ruby/3.0.2/lib/ruby/gems/3.0.0/gems/ffi-1.16.3/lib/ffi/variadic.rb:62:in `rb_fatal'
	from fatal.rb:39:in `block in <main>'
	from fatal.rb:26:in `test'
	from fatal.rb:39:in `<main>'

[matsadler]

Thanks for reporting this, and thank you for the great example, it's so helpful to have a runnable example like that. I've addressed this in 65e35e5.