Remix.run Logo
Remix clone Hacker News
new | show | ask | jobsGithub
▲ ojosilva 3 hours ago

This is the multi-million dollar .unwrap() story. In a critical path of infrastructure serving a significant chunk of the internet, calling .unwrap() on a Result means you're saying "this can never fail, and if it does, crash the thread immediately."The Rust compiler forced them to acknowledge this could fail (that's what Result is for), but they explicitly chose to panic instead of handle it gracefully. This is textbook "parse, don't validate" anti-pattern.

I know, this is "Monday morning quarterbacking", but that's what you get for an outage this big that had me tied up for half a day.

▲wrs 3 hours ago | parent | next [-]

It seems people have a blind spot for unwrap, perhaps because it's so often used in example code. In production code an unwrap or expect should be reviewed exactly like a panic.

It's not necessarily invalid to use unwrap in production code if you would just call panic anyway. But just like every unsafe block needs a SAFETY comment, every unwrap in production code needs an INFALLIBILITY comment. clippy::unwrap_used can enforce this.

▲dehrmann an hour ago | parent | next [-]

It's the same blind spot people have to Java's checked exceptions. People commonly resort to Pokemon exception handling and either blindly ignoring or rethrowing as a runtime exception. When Rust got popular, I was a bit confused by people talking about how great Result it's essentially a checked exception without a stack trace.

▲gwbas1c 26 minutes ago | parent | next [-]

It's a lot lighter: a stack trace takes a lot of overhead to generate; a result has no overhead for a failure. The overhead (panic) only comes once the failure can't be handled. (Most books on Java/C# don't explain that throwing exceptions has high performance overhead.)

Exceptions force a panic on all errors, which is why they're supposed to be used in "exceptional" situations. To avoid exceptions when an error is expected, (eof, broken socket, file not found,) you either have to use an unnatural return type or accept the performance penalty of the panic that happens when you "throw."

In Rust, the stack trace happens at panic (unwrap), which is when the error isn't handled. IE, it's not when the file isn't found, it's when the error isn't handled.

▲Terr_ 38 minutes ago | parent | prev [-]

"Checked Exceptions Are Actually Good" gang, rise up! :p

I think adoption would have played out very different if there had only been some more syntactic-sugar. For example, an easy syntax for saying: "In this method, any (checked) DeepException e that bubbles up should trigger a new (checked) MyException, with e as the linked cause."

▲dist1ll 2 hours ago | parent | prev | next [-]

> every unwrap in production code needs an INFALLIBILITY comment. clippy::unwrap_used can enforce this.

How about indexing into a slice/map/vec? Should every `foo[i]` have an infallibility comment? Because they're essentially `get(i).unwrap()`.

▲10000truths 2 hours ago | parent | next [-]

Yes? Funnily enough, I don't often use indexed access in Rust. Either I'm looping over elements of a data structure (in which case I use iterators), or I'm using an untrusted index value (in which case I explicitly handle the error case). In the rare case where I'm using an index value that I can guarantee is never invalid (e.g. graph traversal where the indices are never exposed outside the scope of the traversal), then I create a safe wrapper around the unsafe access and document the invariant.

▲dist1ll an hour ago | parent [-]

If that's the case then hats off. What you're describing is definitely not what I've seen in practice. In fact, I don't think I've ever seen a crate or production codebase that documents infallibility of every single slice access. Even security-critical cryptography crates that passed audits don't do that. Personally, I found it quite hard to avoid indexing for graph-heavy code, so I'm always on the lookout for interesting ways to enforce access safety. If you have some code to share that would be very interesting.

▲hansvm 38 minutes ago | parent [-]

> graph-heavy code

Could you share some more details, maybe one fully concrete scenario? There are lots of techniques, but there's no one-size-fits-all solution.

▲dist1ll 18 minutes ago | parent [-]

Sure, these days I'm mostly working on a few compilers. Let's say I want to make a fixed-size SSA IR. Each instruction has an opcode and two operands (which are essentially pointers to other instructions). The IR is populated in one phase, and then lowered in the next. During lowering I run a few peephole and code motion optimizations on the IR, and then do regalloc + asm codegen. During that pass the IR is mutated and indices are invalidated/updated. The important thing is that this phase is extremely performance-critical.

▲tux3 2 hours ago | parent | prev | next [-]

Usually you'd want to write almost all your slice or other container iterations with iterators, in a functional style.

For the 5% of cases that are too complex for standard iterators? I never bother justifying why my indexes are correct, but I don't see why not.

You very rarely need SAFETY comments in Rust because almost all the code you write is safe in the first place. The language also gives you the tool to avoid manual iteration (not just for safety, but because it lets the compiler eliminate bounds checks), so it would actually be quite viable to write these comments, since you only need them when you're doing something unusual.

▲wrs an hour ago | parent | next [-]

I didn't restate the context from the code we're discussing: it must not panic. If you don't care if the code panics, then go ahead and unwrap/expect/index, because that works with chosen error handling scheme. This is fine for lots of things like CLI tools or isolated subprocesses, and makes review a lot easier.

So: first, identify code that cannot be allowed to panic. Within that code, yes, in the rare case that you use [i], you need to at least try to justify why you think it'll be in bounds. But it would be better not to.

There are a couple of attempts at getting the compiler to prove that code can't panic (e.g., the no-panic crate).

▲kibwen an hour ago | parent [-]

Indexing is comparatively rare given the existence of iterators, IMO. If your goal is to avoid any potential for panicking, I think you'd have a harder time with arithmetic overflow.

▲dist1ll 2 hours ago | parent | prev [-]

For iteration, yes. But there's other cases, like any time you have to deal with lots of linked data structures. If you need high performance, chances are that you'll have to use an index+arena strategy. They're also common in mathematical codebases.

▲danielheath 2 hours ago | parent | prev [-]

I mean... yeah, in general. That's what iterators are for.

▲speed_spread an hour ago | parent | prev [-]

Pet peeve: unwrap() should be deprecated and renamed or_panic(). More consistent with the rest of stdlib methods and appropriately scarier.

▲jcalvinowens 39 minutes ago | parent | prev | next [-]

> This is the multi-million dollar .unwrap() story.

That's too semantic IMHO. The failure mode was "enforced invariant stopped being true". If they'd written explicit code to fail the request when that happened, the end result would have been exactly the same.

▲AdieuToLogic a minute ago | parent | next [-]

>> This is the multi-million dollar .unwrap() story.

> That's too semantic IMHO. The failure mode was "enforced invariant stopped being true". If they'd written explicit code to fail the request when that happened, the end result would have been exactly the same.

Problem is, the enclosing function (`fetch_features`) returns a `Result`, so the `unwrap` on line #82 only serves as a shortcut a developer took due to assuming `features.append_with_names` would never fail. Instead, the routine likely should have worked within `Result`.

▲navidmafi 8 minutes ago | parent | prev | next [-]

And dare I say, an exhibition of hindsight bias.

▲chrisweekly 13 minutes ago | parent | prev | next [-]

semantic? or pedantic?

▲ 4 minutes ago | parent [-]
[deleted]
▲echelon 18 minutes ago | parent | prev [-]

RUST TEAM - WHY DO WE HAVE `unwrap()`? Kill it with fire!

Of course people will use it in production! Of course they won't check!

This is Rust. This should NOT exist. We use this language to be free of this bullshit, not to find it lurking in the corner.

Jfc, I get that ICs should not use unwrap() in production Rust code. But it's not really the engineer's fault. It's squarely on the language.

Rust is so great - why are we footgunning ourselves with this "language feature"? It's an NPE on demand and isn't type checked!

Teams using Rust should lint this bullshit immediately. Set this to blow up in your production builds. You shouldn't allow unwrap or its evil kin to escape.

The Rust core team - This is on you! Y'all should make a new major version where unwrap only works in debug builds. Remove this "feature" from the language immediately.

This is a PR nightmare for Rust.

People will be talking about this for decades and poking fun at Rust for it. And it'll continue to fail just like today until it's removed.

Rust the language is a 50 carat diamond. The unwrap() cruft is a tin foil ring.

KILL THE UNWRAP METHODS!

p.s. the Zig guys are snickering at us right now. And deservedly so. This is just stupid.

▲dap 7 minutes ago | parent | next [-]

I'm not sure if this is serious or not, but to take it at face value: the value of this sort of thing in Rust is not that it prevents crashes altogether but rather that it prevents _implicit_ failures. It forces a programmer to make the explicit choice of whether to crash.

There's lots of useful code where `unwrap()` makes sense. On my team, we first try to avoid it (and there are many patterns where you can do this). But when you can't, we leave a comment explaining why it's safe.

▲echelon 5 minutes ago | parent [-]

The language semantics do not surface `unwrap` usage or make any guarantees. It should be limited to use in `unsafe` blocks.

▲abigailphoebe 9 minutes ago | parent | prev [-]

blaming the language is not the way to approach this. if an engineer writes bad code that’s the engineers fault, not the languages.

this was bad code that should have never hit production, it is not a rust language issue.

▲echelon 8 minutes ago | parent [-]

No. Don't say "you're holding it wrong". The language says "safe" on the tin. It advertises safety. This shouldn't be possible.

This is a null pointer. In Rust.

Unwrap needs to die. We should all fight to remove it.

▲smj-edison 2 hours ago | parent | prev | next [-]

Isn't the point of this article that pieces of infrastructure don't go down to root causes, but due to bad combinations of components that are correct individually? After reading "engineering a safer world", I find root cause analysis rather reductionistic, because it wasn't just an unwrap, it was that the payload was larger than normal, because of a query that didn't select by database, because a clickhouse made more databases visible. Hard to say "it was just due to an unwrap" imo. Especially in terms of how to fix an issue going forwards. I think the article lists a lot of good ideas, that aren't just "don't unwrap", like enabling more global kill switches for features, or eliminating the ability for core dumps or other error reports to overwhelm system resources.

▲vlovich123 2 hours ago | parent | prev | next [-]

To be fair, this failed in the non-rust path too because the bot management returned that all traffic was a bot. But yes, FL2 needs to catch panics from individual components but I’m not sure if failing open is necessarily that much better (it was in this case but the next incident could easily be the result of failing open).

But more generally you could catch the panic at the FL2 layer to make that decision intentional - missing logic at that layer IMHO.

▲hedora 2 hours ago | parent [-]

Catching panic probably isn’t a great idea if there’s any unsafe code in the system. (Do the unsafe blocks really maintain heap invariants if across panics?)

▲vlovich123 an hour ago | parent | next [-]

Unsafe blocks have nothing to do with it. Yes - they maintain all the same invariants as safe blocks or those unsafe blocks are unsound regardless of panics. But there’s millions of way to architect this (eg a supervisor process that notices which layer in FL2 is crashing and just completely disables that layer when it starts up the proxy again. There’s challenges here because then you have to figure out what constitutes a perma crashing (eg what if it’s just 20% of all sites? Do you disable?). And in the general case you have the fail open/fail close decision anyway which you should just annotate individual layers with.

But the bigger change is to make sure that config changes roll out gradually instead of all at once. That’s the source of 99% of all widespread outages

▲kibwen an hour ago | parent | prev [-]

I think the parent is implying that the panic should be "caught" via a supervisor process, Erlang-style, rather than implying the literal use of `catch_unwind` to resume within the same process.

▲AgentME 2 hours ago | parent | prev | next [-]

This is assuming that the process could have done anything sensible while it had the malformed feature file. It might be in this case that this was one configuration file of several and maybe the program could have been built to run with some defaults when it finds this specific configuration invalid, but in the general case, if a program expects a configuration file and can't do anything without it, panicking is a normal thing to do. There's no graceful handling (beyond a nice error message) a program like Nginx could do on a syntax error in its config.

The real issue is further up the chain where the malformed feature file got created and deployed without better checks.

▲aloha2436 an hour ago | parent | next [-]

> panicking is a normal thing to do

I do not think that if the bot detection model inside your big web proxy has a configuration error it should panic and kill the entire proxy and take 20% of the internet with it. This is a system that should fail gracefully and it didn't.

> The real issue

Are there single "real issues" with systems this large? There are issues being created constantly (say, unwraps where there shouldn't be, assumptions about the consumers of the database schema) that only become apparent when they line up.

▲kondro 25 minutes ago | parent | prev | next [-]

One feature failing like this should probably log the error and fail closed. It shouldn't take down everything else in your big proxy that sits in front of your entire business.

▲WD-42 an hour ago | parent | prev | next [-]

Yea, Rust is safe but it’s not magic. However Nginx doesn’t panic on malformed config. It exits with hopefully a helpful error code and message. The question is then could the cloudflare code have exited cleanly in a way that made recovery easier instead of just straight panicking.

▲JeremyNT an hour ago | parent | prev [-]

Exactly! Sometimes exploding is simply the least bad option, and is an entirely sensible approach.

▲ironman1478 an hour ago | parent | prev | next [-]

I'm not a fan of rust, but I don't think that is the only takeaway. All systems have assumptions about their input and if the assumption is violated, it has to be caught somewhere. It seems like it was caught too deep in the system.

Maybe the validation code should've handled the larger size, but also the db query produced something invalid. That shouldn't have ever happened in the first place.

▲ChrisMarshallNY 2 hours ago | parent | prev | next [-]

Swift has implicit unwrap (!), and explicit unwrap (?).

I don't like to use implicit unwrap. Even things that are guaranteed to be there, I treat as explicit (For example, (self.view?.isEnabled ?? false), in a view controller, instead of self.view.isEnabled).

I always redefine @IBOutlets from:

    @IBOutlet weak var someView!
to:

    @IBOutlet weak var someView?
I'm kind of a "belt & suspenders" type of guy.
▲butvacuum an hour ago | parent | prev | next [-]

It rang more as "A/B deployments are pointless if you can't tell if a downstream failure is related." To me.

▲antonvs 6 minutes ago | parent | prev | next [-]

> This is textbook "parse, don't validate" anti-pattern.

How so? “Parse, don’t validate” implies converting input into typed values that prevent representation of invalid state. But the parsing still needs to be done correctly. An unchecked unwrap really has nothing to do with this.

▲cvhc 2 hours ago | parent | prev | next [-]

Some languages and style guides simply forbid throwing exceptions without catching / proper recovery. Google C++ bans exceptions and the main mechanism for propogating errors is `absl::Status` which the caller has to check. Not familiar with Rust but it seems unwrap is such a thing that would be banned.

▲pdimitar 2 hours ago | parent | next [-]

There are even lints for this but people get impatient and just override them or fight for them to no longer be the default.

As usual: people problem, not a tech problem. In the last years a lot of strides have been made. But people will be people.

▲tonyhart7 2 hours ago | parent [-]

and people make mistake

at some point machine would be better in coding because well machine code is machine instruction task

same like chess, engine is better than human grandmaster because its solvable math field

coding is no different

▲kibwen 43 minutes ago | parent | prev | next [-]

> Not familiar with Rust but it seems unwrap is such a thing that would be banned.

Panics aren't exceptions, any "panic" in Rust can be thought of as an abort of the process (Rust binaries have the explicit option to implement panics as aborts). Companies like Dropbox do exactly this in their similar Rust-based systems, so it wouldn't surprise me if Cloudflare does the same.

"Banning exceptions" wouldn't have done anything here, what you're looking for is "banning partial functions" (in the Haskell sense).

▲gpm 2 hours ago | parent | prev [-]

Unwrap is used in places where in C++ you would just have undefined behavior. It wouldn't make any more sense to blanket ban it than it would ban ever dereferencing a pointer just in case its null - even if you just checked that it wasn't null.

▲ an hour ago | parent | next [-]
[deleted]
▲cherryteastain 2 hours ago | parent | prev [-]

Rust's Result is the same thing as C++'s std::expected. How is calling std::expected::value undefined behaviour?

▲gpm 2 hours ago | parent | next [-]

Rust's foo: Option<&T> is rust's rough equivalent to C++'s const T* foo. The C++ *foo is equivalent to the rust unsafe{ *foo.unwrap_unchecked() }, or in safe code *foo.unwrap() (which changes the undefined behavior to a panic).

Rust's unwrap isn't the same as std::expected::value. The former panics - i.e. either aborts the program or unwinds depending on context and is generally not meant to be handled. The latter just throws an exception that is generally expected to be handled. Panics and exceptions use similar machinery (at least they can depending on compiler options) but they are not equivalent - for example nested panics in destructors always abort the program.

In code that isn't meant to crash `unwind` should be treated as a sign saying that "I'm promising that this will never happen", but just like in C++ where you promise that pointers you deference are valid and signed integers you add don't overflow making promises like that is a necessary part of productive programming.

▲9029 an hour ago | parent | prev [-]

Tangential but funnily enough calling std::expected::error is ub if there is no error :D

▲nrhrjrjrjtntbt an hour ago | parent | prev | next [-]

I wonder what happens if they handle it gracefully? sounds like performance degradation (better than reliability degradation!).

Also wonder with a sharded system why are they not slow rolling out changes and monitoring?

▲ajross 2 hours ago | parent | prev | next [-]

I'm not completely sure I agree. I mean, I do agree about the .unwrap() culture being a bug trap. But I don't think this example qualifies.

The root cause here was that a file was mildly corrupt (with duplicate entries, I guess). And there was a validation check elsewhere that said "THIS FILE IS TOO BIG".

But if that's a validation failure, well, failing is correct? What wasn't correct was that the failure reached production. What should have happened is that the validation should have been a unified thing and whatever generated the file should have flagged it before it entered production.

And that's not an issue with function return value API management. The software that should have bailed was somewhere else entirely, and even there an unwrap explosion (in a smoke test or pre-release pass or whatever) would have been fine.

▲crote an hour ago | parent [-]

It sounds to me like there was validation, but the system wasn't designed for the validation to ever fail - at which point crashing is the only remaining option. You've essentially turned it into an assertion error rather than a parsing/validation error.

Ideally every validation should have a well-defined failure path. In the case of a config file rotation, validation failure of the new config could mean keeping the old config and logging a high-priority error message. In the case of malformed user-provided data, it might mean dropping the request and maybe logging it for security analysis reasons. In the case of "pi suddenly equals 4" checks the most logical approach might be to intentionally crash, as there's obviously something seriously wrong and application state has corrupted in such a way that any attempt to continue is only going to make things worse.

But in all cases there's a reason behind the post-validation-failure behavior. At a certain point leaving it up to "whatever happens on .unwrap() failure" isn't good enough anymore.

▲shadowgovt 2 hours ago | parent | prev | next [-]

In addition, it looks like this system wasn't on any kind of 1%/10%/50%/100% rollout gating. Such a rollout would trivially have shown the poison input killing tasks.

▲penteract an hour ago | parent | next [-]

To me it reads like there was a gradual rollout of the faulty software responsible for generating the config files, but those files are generated on approximately one machine, then propogated across the whole network every 5 minutes.

> Bad data was only generated if the query ran on a part of the cluster which had been updated. As a result, every five minutes there was a chance of either a good or a bad set of configuration files being generated and rapidly propagated across the network.

▲helloericsf an hour ago | parent | prev [-]

Not a DBA, how do you do DB permission rollout gating?

▲guluarte 43 minutes ago | parent | prev | next [-]

it's usually because of fail fast and fail hard, in theory critical bugs will be caught in dev/test

▲arccy 3 hours ago | parent | prev | next [-]

if you make it easy to be lazy and panic vs properly handling the error, you've designed a poor language

▲nine_k 2 hours ago | parent | next [-]

At Facebook they name certain "escape hatch" functions in a way that inescapably make them look like a GIANT EYESORE. Stuff like DANGEROUSLY_CAST_THIS_TO_THAT, or INVOKE_SUPER_EXPENSIVE_ACTION_SEE_YOU_ON_CODE_REVIEW. This really drives home the point that such things must not be used except in rare extraordinary cases.

If unwrap() were named UNWRAP_OR_PANIC(), it would be used much less glibly. Even more, I wish there existed a super strict mode when all places that can panic are treated as compile-time errors, except those specifically wrapped in some may_panic_intentionally!() or similar.

▲adzm 2 hours ago | parent | next [-]

> make them look like a GIANT EYESORE

React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED comes to mind. I did have to reach to this before, but it certainly works for keeping this out of example code and other things like reading other implementations without the danger being very apparent.

At some point it was renamed to __CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE which is much less fun.

▲ 16 minutes ago | parent [-]
[deleted]
▲Nathanba 2 hours ago | parent | prev [-]

right and if the language designers named it UNWRAP_OR_PANIC() then people would rightfully be asking why on earth we can't just use a try-catch around code and have an easier life

▲nine_k 2 hours ago | parent | next [-]

But a panic can be caught and handled safely (e.g. via std:: panic tools). I'd say that this is the correct use case for exceptions (ask Martin Fowler, of all people).

There is already a try/catch around that code, which produces the Result type, which you can presumptuously .unwrap() without checking if it contains an error.

Instead, one should use the question mark operator, that immediately returns the error from the current function if a Result is an error. This is exactly similar to rethrowing an exception, but only requires typing one character, the "?".

▲yoyohello13 2 hours ago | parent | prev | next [-]

Probably not, since errors as values are way better than exceptions.

▲nomel 2 hours ago | parent [-]

How so? An exception is a value that's given the closest, conceptually appropriate, point that was decided to handle the value, allowing you to keep your "happy path" as clean code, and your "exceptional circumstances" path at the level of abstraction that makes sense.

It's way less book-keeping with exceptions, since you, intentionally, don't have to write code for that exceptional behavior, except where it makes sense to. The return by value method, necessarily, implements the same behavior, where handling is bubbled up to the conceptually appropriate place, through returns, but with much more typing involved. Care is required for either, since not properly bubbling up an exception can happen in either case (no re-raise for exceptions, no return after handling for return).

▲yoyohello13 2 hours ago | parent | next [-]

There are many many pages of text discussing this topic, but having programmed in both styles, exceptions make it too easy for programmer to simply ignore them. Errors as values force you to explicitly handle it there, or toss it up the stack. Maybe some other languages have better exception handling but in Python it’s god awful. In big projects you can basically never know when or how something can fail.

▲nomel an hour ago | parent [-]

I would claim the opposite. If you don't catch an exception, you'll get a halt.

With return values, you can trivially ignore an exception.

    let _ = fs::remove_file("file_doesn't_exist");

    or

    value, error = some_function()
    // carry on without doing anything with error
In the wild, I've seen far more ignoring return errors, because of the mechanical burden of having type handling at every function call.

This is backed by decades of writing libraries. I've tried to implement libraries without exceptions, and was my admittedly cargo-cult preference long ago, but ignoring errors was so prevalent among the users of all the libraries that I now always include a "raise" type boolean that defaults to True for any exception that returns an error value, to force exceptions, and their handling, as default behavior.

> In big projects you can basically never know when or how something can fail.

How is this fundamentally different than return value? Looking at a high level function, you can't know how it will fail, you just know it did fail, from the error being bubbled up through the returns. The only difference is the mechanism for bubbling up the error.

Maybe some water is required for this flame war. ;)

▲yoyohello13 13 minutes ago | parent [-]

I can agree to disagree :)

▲pyrolistical 2 hours ago | parent | prev [-]

Exception is hidden control flow, where as error values are not.

That is the main reason why zig doesn’t have exceptions.

▲nomel 2 hours ago | parent [-]

I'd categorize them more as "event handlers" than "hidden". You can't know where the execution will go at a lower level, but that's the entire point: you don't care. You put the handlers at the points where you care.

▲sfink 2 hours ago | parent | prev [-]

...and you can? try-catch is usually less ergonomic than the various ways you can inspect a Result.

    try {
      data = some_sketchy_function();
    } catch (e) {
      handle the error;
    }
vs

    result = some_sketchy_function();
    if let Err(e) = result {
      handle the error;
    }
Or better yet, compare the problematic cases where the error isn't handled:

    data = some_sketchy_function();
vs

    data = some_sketchy_function().UNWRAP_OR_PANIC();
In the former (the try-catch version that doesn't try or catch), the lack of handling is silent. It might be fine! You might just depend on your caller using `try`. In the latter, the compiler forces you to use UNWRAP_OR_PANIC (or, in reality, just unwrap) or `data` won't be the expected type and you will quickly get a compile failure.

What I suspect you mean, because it's a better argument, is:

    try {
        sketchy_function1();
        sketchy_function2();
        sketchy_function3();
        sketchy_function4();
    } catch (e) {
        ...
    }
which is fair, although how often is it really the right thing to let all the errors from 4 independent sources flow together and then get picked apart after the fact by inspecting `e`? It's an easier life, but it's also one where subtle problems constantly creep in without the compiler having any visibility into them at all.
▲SchwKatze 3 hours ago | parent | prev | next [-]

Unwrap isn't a synonym for laziness, it's just like an assertion, when you do unwrap() you're saying the Result should NEVER fail, and if does, it should abort the whole process. What was wrong was the developer assumption, not the use of unwrap.

▲SchemaLoad 2 hours ago | parent | next [-]

It also makes it very obvious in the code, something very dangerous is happening here. As a code reviewer you should see an unwrap() and have alarm bells going off. While in other languages, critical errors are a lot more hidden.

▲kloop 2 hours ago | parent [-]

I hate that it's a method. That can get lost in a method chain easily enough during a code review.

A function or a keyword would interrupt that and make it less tempting

▲pdimitar an hour ago | parent [-]

Well, you can request Clippy to tell you about them. I do that in my hobby projects.

▲dietr1ch 2 hours ago | parent | prev | next [-]

> What was wrong was the developer assumption, not the use of unwrap.

How many times can you truly prove that an `unwrap()` is correct and that you also need that performance edge?

Ignoring the performance aspect that often comes from a hat-trick, to prove such a thing you need to be wary of the inner workings of a call giving you a `Return`. That knowledge is only valid at the time of writing your `unwrap()`, but won't necessarily hold later.

Also, aren't you implicitly forcing whoever changes the function to check for every smartass dev that decided to `unwrap` at their callsite? That's bonkers.

▲JuniperMesos 2 hours ago | parent | next [-]

I doubt that this unwrap was added for performance reasons; I suspect it was rather added because the developer temporarily didn't want to deal with what they thought was an unlikely error case while they were working on something else; and no other system recognized that the unwrap was left in and flagged it before it was deployed on production servers.

If I were Cloudflare I would immediately audit the codebase for all uses of unwrap (or similar rust panic idioms like expect), ensure that they are either removed or clearly documented as to why it's worth crashing the program there, and then add a linter to their CI system that will fire if anyone tries to check in a new commit with unwrap in it.

▲duped an hour ago | parent | prev [-]

Panics are for unexpected error conditions, like your caller passed you garbage. Results are for expected errors, like your caller passed you something but it's your job to tell if it's garbage.

So the point of unwrap() is not to prove anything. Like an assertion it indicates a precondition of the function that the implementer cannot uphold. That's not to say unwrap() can't be used incorrectly. Just that it's a valid thing to do in your code.

Note that none of this is about performance.

▲Rohansi 2 hours ago | parent | prev [-]

> when you do unwrap() you're saying the Result should NEVER fail

Returning a Result by definition means the method can fail.

▲Buttons840 44 minutes ago | parent | next [-]

Division can fail, but I can write a program where I'm sure division will not fail.

▲SchwKatze 2 hours ago | parent | prev | next [-]

Yeah, I think I expressed wrongly here. A more correct version would be: "when you do unwrap() you're saying that an error on this particular path shouldn't be recoverable and we should fail-safe."

▲Dylan16807 2 hours ago | parent | prev [-]

> Returning a Result by definition means the method can fail.

No more than returning an int by definition means the method can return -2.

▲yoyohello13 2 hours ago | parent [-]

What? Results have a limited number of possible error states that are well defined.

▲Dylan16807 2 hours ago | parent [-]

Some call points to a function that returns a Result will never return an Error.

Some call points to a function that returns an int will never return -2.

Sometimes you know things the type system does not know.

▲Rohansi 2 hours ago | parent [-]

The difference is functions which return Result have explicitly chosen to return a Result because they can fail. Sure, it might not fail in the current implementation and/or configuration, but that could change later and you might not know until it causes problems. The type system is there to help you - why ignore it?

▲Dylan16807 2 hours ago | parent [-]

Because it would be a huge hassle to go into that library and write an alternate version that doesn't return a Result. So you're stuck with the type system being wrong in some way. You can add error-handling code upfront but it will be dead code at that point in time, which is also not good.

▲JuniperMesos 2 hours ago | parent | prev | next [-]

It's a little subtler than this. You want it to be easy to not handle an error while developing, so you can focus on getting the core logic correct before error-handling; but you want it to be hard to deploy or release the software without fully handling these checks. Some kind of debug vs release mode with different lints seems like a reasonable approach.

▲yoyohello13 3 hours ago | parent | prev | next [-]

So… basically every language ever?

Except maybe Haskell.

▲dkersten 3 hours ago | parent [-]

And Gleam

▲leshenka 2 hours ago | parent | prev | next [-]

All languages with few exceptions have these kinds of escape hatches like unwrap

▲otterley 3 hours ago | parent | prev [-]

https://en.wikipedia.org/wiki/Crash-only_software

▲nine_k 3 hours ago | parent [-]

Works when you have the Erlang system that does graceful handing for you: reporting, restarting.

▲ 3 hours ago | parent | prev [-]
[deleted]