Yep. This was the difference between a silent, recoverable error and a loud failure.

It seems like they’re planning to remove all potential panics based on the end of their article. This would be a good idea considering the scale of the service’s usage.

(Also, for anyone who’s not reading the article, the unwrap caused the service to crash, but wasn’t the source of the issues to begin with. It was just what toppled over first.)

They also apparently didn’t do any input validation. This is why a faulty config was able to even trigger all of that.

I’ve read a lot of people saying “oh what a horror, they used unwrap in production, they should NEVER do that”. But I don’t think that’s true at all.

1. The rust type system is not 100% flawless. For example you have a mathematically proven never-errors algorithm, but inside some steps have provisional Nones. At the end you just unwrap, since there’s no point in changing the return type to an option to handle a non-existent error. You can’t represent “this is None but I promise that it won’t by the end” in rust’s type system.
2. Sometimes it’s not worth handling the error. For example a simple CLI tool, just throw expects everywhere that shows why it failed, since most CLI tools just close anyway on failure, might as well panic.

That being said, this was clearly a case where ? should have been used, since the function has a clear error case. And it also is a critical application that can’t afford panicking as an error handling method.

EDIT: Just to clarify, the way you would do this is by having a type Features that can error on creation/parsing which has a limit of 200 entries. And afterwards you can just use that type knowing that it will always have <= 200 entries. And you gracefully handle that one error case on creation/parsing.

EDIT2: Another point for why I disagree with “production should never have unwrap”:

1. Unwrap is basically the same as assert(my_variable.is_some). I don’t know a single person that would say “you should never have asserts in production code”, the only reason I disable some assertions for release builds is for performance. Asserts are statements that should never do anything, by that logic they pose no harm by being there, so no need to remove them, their only effect should be wasting a few CPU cycles. Since asserts are only put when the programmer assures that a certain precondition will always be met. And if it is not, it’s because the code changes and automatic tests should catch that.