Remix.run Logo
pdimitar 2 hours ago

While I heavily frown upon using `unwrap` and `expect` in Rust code and make sure to have Clippy tell me about every single usage of them, I also understand that without them Rust might have been seen as an academic curiosity language.

They are escape hatches. Without those your language would never take off.

But here's the thing. Escape hatches are like emergency exits. They are not to be used by your team to go to lunch in a nearby restaurant.

---

Cloudflare should likely invest in better linting and CI/CD alerts. Not to mention isolated testing i.e. deploy this change only to a small subset and monitor, and only then do a wider deployment.

Hindsight is 20/20 and we can all be smartasses after the fact of course. But I am really surprised because lately I am only using Rust for hobby projects and even I know I should not use `unwrap` and `expect` beyond the first iteration phases.

---

I have advocated for this before but IMO Rust at this point will benefit greatly from disallowing those unsafe APIs by default in release mode. Though I understand why they don't want to do it -- likely millions of CI/CD pipelines will break overnight. But in the interim, maybe a rustc flag we can put in our `Cargo.toml` that enables such a stricter mode? Or have that flag just remove all the panicky API _at compile time_ though I believe this might be a Gargantuan effort and is likely never happening (sadly).

In any case, I would expect many other failures from Cloudflare but not _this_ one in particular.

duped an hour ago | parent [-]

This is not a reasonable take to me. unwrap/expect are the idiomatic way to express code paths returning Option/Result as unreachable.

Bubbling up the error or None does not make the program correct. Panicking may be the only reasonable thing to do.

If panicking is guaranteed because of some input mistake to the system your failure is in testing.

pdimitar an hour ago | parent [-]

I agree the failure is in testing but what you can and should do is raise in alert in your APM system before the runtime panic, in the code path that is deemed impossible to hit.

I am not trashing on them, I've made such mistakes in the past, but I do expect more from them is all.

And you will not believe how many alerts I got for the "impossible" errors.

I do agree there was not too much that could have been done, yes. But they should have invested in more visibility and be more thorough. I mean, hobbyist Rust devs seem to do that better.

It was just a bit disappointing for me. As mentioned above, I'd understand and sympathise with many other mistakes but this one stung a bit.

duped an hour ago | parent [-]

There's certainly a discipline involved here, but it's usually something like guaranteeing all threads are unwind safe (via AssertUnwindSafe) and logging stack traces when your process keeps dying/can't be started after a fixed number of retries. Which would lead you to the culprit immediately.

I'm just pushing back a bit on the idea that unwrap() is unsafe - it's not, and I wouldn't even call it a foot gun. The code did what it was written to do, when it saw the input was garbage it crashed because it couldn't make sense of what to do next. That's a desirable property in reliable systems (of course monitoring that and testing it is what makes it reliable/fixable in the first place).

pdimitar an hour ago | parent [-]

We don't disagree, my main point was a bit broader and admittedly hijacked the original topic a bit, namely: `unwrap` and `expect` make many Rust devs too comfortable and these two are very tempting mistresses.

Using those should be done in an extremely disciplined manner. I agree that there are many legitimate uses but in the production Rust code I've seen this has rarely been the case. People just want to move on and then forget to circle back and add proper error handling. But yes, in this case that's not quite true. Still, my point that an APM alert should have been raised on the "impossible" code path before panicking, stands.