-
Notifications
You must be signed in to change notification settings - Fork 453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Serializer: reraise all .load errors as UnmarshalError #1011
Serializer: reraise all .load errors as UnmarshalError #1011
Conversation
In order to avoid missing an error message to filter out, treat any Marshal.load error as a failed serialization, and trust Ruby's e.cause system to provide a lineage of the error's true beginning.
9f6bce2
to
6a781c8
Compare
b0f2e61
to
d19285c
Compare
@olleolleolle So is this an alternative to #1008 ? Are there any concerns on how this works with older Ruby versions? Will they see a meaningful change in behavior? |
@olleolleolle It would helpful to understand how these different PRs relate, since they seem to touch similar code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the best option by far. cause
is supported as far back as 2.1 (https://docs.ruby-lang.org/en/2.1.0/Exception.html#method-i-cause) so I don't think depending on it for debugging failures is a problem, and in general, this PR should be much safer, since the alternative is unexpected and thus unhandled exceptions bubbling up through cache implementations (ultimately resulting in 🔥 in production).
Ah, thanks for asking, they're sort of the evolution of my thinking on this. This is the more thorough change, relying on the Ruby feature of "cause" in exception handling. The previous ones were about "not editing too much in the project", trying to use a light touch. |
In order to avoid missing an error message to filter out, treat any Marshal.load error as a failed serialization, and trust Ruby's e.cause system (edit: introduced in Ruby 2.1) to provide a lineage of the error's true beginning.
This change was inspired by a comment relating to #1008.
With this rescue, it's possible to dismantle the other regular expressions.
This PR supersedes #1009, #1008