3 ms·

Why does cloudflare allow unwraps in their code? I would've assumed they'd have clippy lints stopping that sort of thing. Why not just match with { ok(value) =>

by vsgherzi 11mo ago

Why does cloudflare allow unwraps in their code? I would've assumed they'd have clippy lints stopping that sort of thing. Why not just match with { ok(value) => {}, Err(error) => {} } the function already has a Result type. At the bare minimum they could've used an expect("this should never happen, if it does database schema is incorrect"). The whole point of errors as values is preventing this kind of thing.... It wouldn't have stopped the outage but it would've made it easy to diagnose. If anyone at cloudflare is here please let me in that codebase :)

waterTanuki 11mo ago
Not a cloudflare employee but I do write a lot of Rust. The amount of things that can go wrong with any code that needs to make a network call is staggeringly high. unwrap() is normal during development phase but there are a number of times I leave an expect() for production because sometimes there's no way to move forward.
- vsgherzi 11mo ago
  I'm in a similar boat, at the very leas an expect can give hits to what happened. However this can also be problematic if your a library developer. Sometimes rust is expected to never panic especially in situations like WASM. This is a major problem for companies like Amazon Prime Video since they run in a WASM context for their TV APP. Any panic crashes everything. Personally I usually just either create a custom error type (preferred) or erase it away with Dyn Box Error (no other option). Random unwraps and expects haunt my dreams.
- SchemaLoad 11mo ago
  Yeah it seems likely that even if there wasn't an unwrap, there would have been some error handling that wouldn't have panicked the process, but would have still left it inoperable if every request was instead going through an error path.
- frumplestlatz 11mo ago
  At risk of sounding harsh, that’s a huge failure in your modeling of invariants that should not be permitted in development. Permitting it in development is why one ends up in the position of having to use an `expect()` in production code, because your API surfaces are wrong and can’t model your actual invariants.
ozgrakkurt 11mo ago
And the error magically disappears when the function returns it?
- csomar 11mo ago
  It doesn’t disappear, it forces you to handle it.
  - VMG 11mo ago
    Propagating upwards a valid way of handling it and often the correct answer. There needs to be something at the top level that can handle a crashing process.
    - ozgrakkurt 11mo ago
      You mean like kubernetes that restarts your program when it crashes?
    - hu3 11mo ago
      can Rust handle global panics? Or can a unwrap be stopped? This is just a normal Tuesday for languages with Exception and try/catch.
      - frumplestlatz 11mo ago
        > This is just a normal Tuesday for languages with Exception and try/catch. Yes, unfortunately, random stack unrolls and weird state bugs as a result are a normal Tuesday for languages with (unchecked) Exception and try/catch
pornel 11mo ago
unwrap() is only the most superficial part of the problem. Merely replacing `unwrap()` with `return Err(code)` wouldn't have changed the behavior. Instead of "error 500 due to panic" the proxy would fail with "error 500 due to $code". Unwrap gives you a stack trace, while retuned Err doesn't, so simply using a Result for that line of code could have been even harder to diagnose. `unwrap_or_default()` or other ways of silently eating the error would be less catastrophic immediately, but could still end up breaking the system down the line, and likely make it harder to trace the problem to the root cause. The problem is deeper than an unwrap(), related to handling rollouts of invalid configurations, but that's not a 1-line change.
- frumplestlatz 11mo ago
  We don't know what the surrounding code looks like, but I'd expect it handles the error case that's expressed in the type signature (unless they `.unwrap()` there too). The problem is that they didn't surface a failure case, which means they couldn't handle rollouts of invalid configurations correctly. The use of `.unwrap()` isn't superficial at all -- it hid an invariant that should have been handled above this code. The failure to correctly account for and handle those true invariants is exactly what caused this failure mode.

waterTanuki 11mo ago

Not a cloudflare employee but I do write a lot of Rust. The amount of things that can go wrong with any code that needs to make a network call is staggeringly high. unwrap() is normal during development phase but there are a number of times I leave an expect() for production because sometimes there's no way to move forward.

vsgherzi 11mo ago
I'm in a similar boat, at the very leas an expect can give hits to what happened. However this can also be problematic if your a library developer. Sometimes rust is expected to never panic especially in situations like WASM. This is a major problem for companies like Amazon Prime Video since they run in a WASM context for their TV APP. Any panic crashes everything. Personally I usually just either create a custom error type (preferred) or erase it away with Dyn Box Error (no other option). Random unwraps and expects haunt my dreams.
SchemaLoad 11mo ago
Yeah it seems likely that even if there wasn't an unwrap, there would have been some error handling that wouldn't have panicked the process, but would have still left it inoperable if every request was instead going through an error path.
frumplestlatz 11mo ago
At risk of sounding harsh, that’s a huge failure in your modeling of invariants that should not be permitted in development. Permitting it in development is why one ends up in the position of having to use an `expect()` in production code, because your API surfaces are wrong and can’t model your actual invariants.

ozgrakkurt 11mo ago

And the error magically disappears when the function returns it?

csomar 11mo ago
It doesn’t disappear, it forces you to handle it.
- VMG 11mo ago
  Propagating upwards a valid way of handling it and often the correct answer. There needs to be something at the top level that can handle a crashing process.
  - ozgrakkurt 11mo ago
    You mean like kubernetes that restarts your program when it crashes?
  - hu3 11mo ago
    can Rust handle global panics? Or can a unwrap be stopped? This is just a normal Tuesday for languages with Exception and try/catch.
    - frumplestlatz 11mo ago
      > This is just a normal Tuesday for languages with Exception and try/catch. Yes, unfortunately, random stack unrolls and weird state bugs as a result are a normal Tuesday for languages with (unchecked) Exception and try/catch

pornel 11mo ago

unwrap() is only the most superficial part of the problem. Merely replacing `unwrap()` with `return Err(code)` wouldn't have changed the behavior. Instead of "error 500 due to panic" the proxy would fail with "error 500 due to $code". Unwrap gives you a stack trace, while retuned Err doesn't, so simply using a Result for that line of code could have been even harder to diagnose. `unwrap_or_default()` or other ways of silently eating the error would be less catastrophic immediately, but could still end up breaking the system down the line, and likely make it harder to trace the problem to the root cause. The problem is deeper than an unwrap(), related to handling rollouts of invalid configurations, but that's not a 1-line change.

frumplestlatz 11mo ago
We don't know what the surrounding code looks like, but I'd expect it handles the error case that's expressed in the type signature (unless they `.unwrap()` there too). The problem is that they didn't surface a failure case, which means they couldn't handle rollouts of invalid configurations correctly. The use of `.unwrap()` isn't superficial at all -- it hid an invariant that should have been handled above this code. The failure to correctly account for and handle those true invariants is exactly what caused this failure mode.