Comment by film42
8 months ago
Nice! In rails I end up using Rails.cache most of the time because it's always "right there" but I like how you break out the cache to be a per-method to minimize contention. Depending on your workload it might make sense to use a ReadWrite lock instead of a Monitor.
Only suggestion is to not wrap the error of the caller in your memo wrapper.
> raise MemoTTL::Error, "Failed to execute memoized method '#{method_name}': #{e.message}"
It doesn't look like you need to catch this for any operational or state tracking reason so IMO you should not catch and wrap. When errors are wrapped with a string like this (and caught/ re-raised) you lose the original stacktrace which make debugging challenging. Especially when your error is like, "pg condition failed for select" and you can't see where it failed in the driver.
I thought ruby would auto-wrap the original exception as long as you are raising from a rescue block (i.e. as long as $! is non-nil). So in that case you can just
And ruby will set `cause` for you
https://pablofernandez.tech/2014/02/05/wrapped-exceptions-in...
TIL! That's pretty cool. I still think if you have no reason to catch an error (i.e. state tracking, etc.) then you should not.
Thanks for the feedback! That's a very good point, I'll update the gem and let it bubble up.