Make magnus::Error impl std::error::Error

[macournoyer]

So it can be used with other error handling crates: thiserror, anyhow, etc.

[matsadler]

Hey, thanks for raising this.

magnus::Error may contain a Ruby object, which needs to be visible to Ruby's GC to avoid being collected. Usually this is accomplished by keeping the magnus::Error on the stack. The problem with implementing std::error::Error for magnus::Error is that it'll then readily convert to Box<dyn std::error::Error> with ?, and it's not safe to box magnus::Error because then the Ruby object it's wrapping can then no longer be found when the GC is marking objects. anyhow::Error is also effectively a Box.

I can't think of a good way around this that feels like it's a good fit for all use cases.

Magnus has a BoxValue type that magnus::Error could use internally. That can safely box a Ruby object using rb_gc_register_address/rb_gc_unregister_address under the hood. But this requires an extra allocation that's not needed in the majority of use cases.

rb_gc_register_mark_object (gc::register_mark_object in Magnus) allows permanently marking an object without the extra allocation, but there's no way to un-mark that object, so it's just a memory leak for this use case.

It would be possible to do something like what rb_gc_register_mark_object does internally, and create a global array that objects can be added to make sure they get marked, but also implement removing objects. But this would introduce some global state management to Magnus that I don't really want to do. It'd also add the overhead of an extra call into the Ruby VM with every error created, which again is not needed in the majority of use cases.

There could be another error type where we take a Ruby exception and turn it into a pair of Strings, the class name and the message, then that'd be safe to Box. However not every Ruby exception is representable by its class name and message, some exceptions contain other data, and it's possible to write an exception class that needs more than just a message to be constructed. You'd also lose the backtrace (or you'd have to store a third string, and I think backtraces are generated by Ruby on demand, so you'd be paying the cost of that even it it wasn't used later).

---

To throw another wrench into the works, with the next release of Magnus, Ruby objects won't be Send or Sync anymore, so magnus::Error won't be Send + Sync, so it won't be compatible with anyhow::Error as that requires Send + Sync.

---

I have been thinking of updating the ReturnValue trait (the thing that allows you to return Result<T, magnus::Error> and have the T converted to Ruby or the Error raised as an exception) to accept anything that converts to magnus::Error (probably via a magnus::IntoError trait, similar to IntoValue). That doesn't really address the problem raised here, but depending on your exact use case it might make things a bit easier?

[macournoyer]

I see. So that omission was intentional. Make sense.

What about impl std::error::Error on OpaqueError instead?

impl std::error::Error for OpaqueError {}

So you could do something like: .map_err(OpaqueError::into)?

To throw another wrench into the works, with the next release of Magnus, Ruby objects won't be Send or Sync anymore, so magnus::Error won't be Send + Sync, so it won't be compatible with anyhow::Error as that requires Send + Sync.

But std::error::Error does not require Send + Sync, so it could still be useful for lib, I don't think thiserror requires it for example.

[matsadler]

What about impl std::error::Error on OpaqueError instead?

Unfortunately OpaqueError is in the same boat as it still contains Ruby objects.

[macournoyer]

Got it! Closing