Re-opening deprecating Option::unwrap and Result::unwrap

language design
101

Normally, I'd agree (that's why, to this day, I'm wary of anything written by the author of goblin. I haven't trusted their judgment since the day I managed to panic goblin with one of my only mildly esoteric Open Watcom C/C++ "Hello, World!" test files.) but, it really does depend on how you architect your system.

Libraries should bubble up I/O errors so the binary can choose how they get handled, but you might want a binary to fail fast so some higher-level safety mechanism can either roll back the version or take it out of the pool.

102

If write! happens around IO - sure, in the first example I found in my codebase - I'd have to change several functions that can't fail to return Result, do shenanigans with iterators and then still use unwrap because IO uses println! (well, safer variant of it that exits the whole program if it detects errors).

103

The assumption that these never fail has been a problem for adding handling of out of memory errors during writes to Vec. These writes can fail like any other I/O (Linux with an overcomitting allocator is not the only platform in the world), and should be handled.

2 Likes
104

Currently Rust can't reliably handle out-of-memory on any platform, even with a memory quota (which is required for out-of-memory states to be meaningful: otherwise the out-of-memory situation hits every process on the system simultaneously, generally making it unusable unless it's an embedded system that only has one process), due to the risk that that the last byte of memory is consumed by a stack allocation rather than by a heap allocation.

There is some potential for adding builtins with semantics like "please check all the functions I'm about to call for stack overflows, and give me an error now rather than an uncatchable error later if one of them would overflow" but I haven't seen much discussion of that basic direction (and yet it's required for handilng out-of-memory to be meaningful).

105

You can statically compute worst case stack usage and budget for that. This is not uncommon in safety critical embedded. Or you can determine it empirically (common in non-safety critical embedded). Of course recursion throws all that out the window, but you really shouldn't be doing recursion (or at least have a strict upper bound) in such code.

Desktop or server is probably a bit more YOLO I expect (and don't even speak of academic code, that tends to be awful in so many ways for any sort of long term stability or maintenance).

Perhaps it is time for the non-embedded people to start taking resource limitations seriously too? Wait, who am I kidding...

1 Like
106

I could see utility in a more restricted unwrap_in_result lint which only fires when E1 and E2 in Result<T, E1>/Result<T, E2> are the same type, and/or compatible types (if compatibility can be easily determined at this stage)

107

I agree, but at the moment Rust doesn't have good support for doing that from within the language itself, so it has to be done outside the language and/or by just guessing that a program as a whole won't use a problematic amount of stack (and even then you would have to prefault the portion of the stack you need, which many OSes don't do because it just wastes memory in cases where programs don't use much stack in practice as they are theoretically allowed).

I would much prefer it if the language did in fact have a check_stack_usage_of!{â€Ļ} macro that statically calculated the stack usage of everything inside the block (and that wasn't inside an interior copy of the same macro), attempted to prefault it, and returned Err if there wasn't enough memory to do the prefaulting. (Part of the context here is that I've been trying to design a replacement allocator_api and trying to make guarantees about out-of-memory behaviour was just impossible due to the stack allocations not being accounted for.) This would be compatible with recursive code but you woul need to place the macro around the recursive call.

It's also worth noting that on non-embedded OSes, the systemwide out-of-memory state may never happens in a detectable way – Linux's "attempt to detect the leaking process and kill it" heuristic is well-known, and Windows at least used to (and possibly still does, but I haven't checked at all recently) gradually allocate the entire remaining space on the hard disk as swap space if it gets low on memory, so the out-of-memory state was never actually reached (because the computer would become unusably slow due to the hard drive effectively being used as an extension of RAM whilst not being able to maintain anything near the same speed). Setting a memory quota for a single process is a reliable way to avoid those sorts of system-wide effects and allow the process itself to gracefully handle reaching its memory limit (once the stack-allocation problem is solved).

108

Rust's default semantics were designed around the reality that Linux and other Unixy OSes default to memory overcommit as an optimization for fork().

On these platforms, unless the site specifically changes a tunable and accepts pathological spikes in reserved memory every time a process uses fork()/exec() instead of the younger posix_spawn(), the underlying OS APIs will report success and then your program runs the risk of being killed when you attempt to access the memory which was "allocated".

2 Likes
109
OT about OOM This is not true, and I'm talking from years of experience of running web services at large scale, which regularly bump into OOMs.
  • This isn't all-or-nothing, but a quality scale. The more OOM cases can be handled well, the more resilient the program is. Handling OOM gracefully in 99% of cases is way better than 0%. Programs have to be prepared to crash/abort anyway, but that is disruptive, often expensive, and retrying risks crash loops, so graceful isolated OOM handling helps.
  • It basically never happens that the process is out of the very last byte of memory. Likelihood of hitting the limit is generally proportional to the allocation size, so almost all OOMs happen when (re)allocating large buffers. Real-world allocators also have per-size buckets, so failure to allocate one size doesn't prevent allocating other sizes (typically you can allocate error messages easily, even after OOM when doubling a Vec)
  • In real deployments it never happens that the machine is desperately out of memory. Reasonable setups have limits per container and per process, which can be much lower than the total machine memory.
  • Rust's own allocator can be instrumented to voluntarily limit max allocated memory, which works very reliably even on systems that can't report OOM themselves. It isn't subject to virtual memory cleverness, and can be used to curb memory usage of a process before it becomes a machine-wide problem.
  • Returning Err typically starts by releasing memory. If you don't allocate in your error's From (and most leaf functions don't), you can often even recover from the OOM-to-the-last-byte situations. libstd could do better by preallocating Box for PanicInfo, but as long as it gets past that hurdle, unwinding is mostly releasing memory too.

I've seen many people assume it's impossible based on failures of raw malloc and goto error-style shoddy untested error paths, but the situation with compiler-inserted Drop and Rust's design patterns is very different. I know it works, because I'm using it and actually handle OOMs in production services. It's really annoying to be told time after time that what I'm currently doing is impossible to do.

8 Likes
110

Configuring OOM to panic is a lot less disruptive than forcing every allocation to return a Result without having much downsides besides it still requiring nightly, right? In the case of a web server, you probably have to abort the entire request and you would probably have a catch_unwind in place anyway to return a 500 on panics, which would then automatically handle OOM too.

111

There are complicated reentrancy concerns.

112
OT about OOM

I run all my Linux-based machines with overcommit disabled and haven't had any problems. With today's disk sizes, it's perfectly reasonable to set aside 2*(installed RAM) bytes as swap space that won't ever actually be used, just to ensure that normal usage of fork+exec will go through without overcommit, no matter how big the parent process is. I also use system-wide resource limits to prevent any single process from getting bigger than physical RAM.

1 Like
113

In general, panic on OOM would be an improvement over the current mostly-aborting behavior.

But in the case of write!, it still wouldn't make any sense. write! already returns a Result, so having it panic in some situations doesn't make the interface any simpler nor more convenient. It adds more work if you want to handle both error reporting methods in a generic context, and even in the Vec-only case it still requires handling/discarding the useless Result. It's just a misleading and inconsistent behavior that fell out of a neglected/nihilistic OOM handling. If write! to Vec had been returning ErrorKind::OutOfMemory since Rust 1.0, nobody would later approve an RFC to make it abort or panic in just that one case instead. It's just a legacy wart.

OOM OT

Would it be so bad? Rust programs already try to avoid (re)allocations as much as possible.

The most string-realloc-intensive fmt machinery already has fmt::Result on everything, specifically for stopping on I/O errors (which should have included the string OOM case), so it wouldn't get any worse. Various serializers, codecs, compressors, template engines want to work with any destination, which pushes them towards using io::Write and propagate io::Error anyway. That is a bit annoying and inefficient (try_reserve() with zero-sized error is more efficient), but it wouldn't get any worse either.

In programs where I want to handle as many OOMs as I can, it mainly boils down to adding Error::OOM to a few enums, and sprinkling of try_ + ? here and there. It would be easier if std had try_ methods for everything, and more bounded capacity functions.

Result exists to make error handling more flexible and locally explicit than try/catch, and that's a feature. When I want to handle all possible errors, not having this explicitness is a problem.

I don't believe that handling of OOM is pointless, so I don't see any argument in favor of unwinding/against Result more convincing for OOM than for example I/O errors. You could also say that real recoverable I/O errors are rare: when SSDs fail they fail catastrophically, and other I/O errors during data writes are mainly due to lack of disk space, but modern systems happen to die almost as badly without any disk space left as they die without RAM (on macOS it is basically the same thing)[1]. So io::Write::write() could panic too, and try { serialize(fd); } catch() { close(fd); unlink(path); } avoids the burden of propagating the io::Errors everywhere.

Which will still exist if oom=panic is removed, because that problem isn't caused by the convenience setting, but the hooks it uses.

The global linker hacks for panic=abort/panic=unwind can be annoying, and allocator APIs and their error handling is severely limited by linker hacks and LLVM magic powering #[global_allocator]. oom=panic adds to this pile, but I presume that whatever solves the other two, will make oom=panic more elegant too.

  1. I'm exaggerating of course — root disk failure isn't the only I/O error that can happen, just like overcomitted Linux isn't the only OOM case ↩ī¸Ž

1 Like
114

IMO the only thing wrong with unwrap is that it's not called assert, which would draw a nice parallel with assert! and make its panicky nature blindingly obvious. And then I wish we also had assert as an inherent method on bool since method chaining is often nicer then wrapping the entire call in an assert!, especially if the call has side-effects. (*Map::try_insert has helped a lot to reduce the need for the latter, though it is still needed for *Set::insert as no corresponding try_insert exists there.)

But, I seem to be in the minority with this opinion, so :person_shrugging: .

8 Likes
115

I could bikeshed about the exact name, but I agree that it would be good to have a separate method specifically for "this is supposed to always succeed" case.

The way .unwrap() is used in practice made it unclear whether each use is really asserting that, or is a "todo: put proper error handing here later", or "this may fail, but should fail loudly in a way that's hard to ignore".

3 Likes
116

FWIW my personal workaround here is to use .expect("TODO") for the TODO case (and // UNWRAP comments in the "never fail" case), but I agree a well-named shared public convention would be useful!

1 Like
117

long_chain_of_random_things.is_ok().assert() does have a certain ring to it, I'll be honest. I could see it happening.

2 Likes
118

Clippy lint doesn't block building, so unless the Ci runs Clippy as a step or the user create an alias to run clippy and then cargo. Files with unwrap() could still be pushed. Updating attributes to the Cargo.toml for for lints including option for clippy such as run_before_build = bool and deny_build_for = ["release", "debug"] would be my suggestion.

Doesn't fix existing projects, but shouldn't break them either and would we easy to opt into later.

119

Is it possible to have a mode for the clippy lint to not fire in cfg(test) code? Or do those all need turned into expect() as well?

2 Likes
120

These configuration options might be of interest to you:

# "Whether expect should be allowed in test functions or #[cfg(test)]"
# https://rust-lang.github.io/rust-clippy/master/index.html#expect_used
allow-expect-in-tests = true

# "Whether indexing_slicing should be allowed in test functions or #[cfg(test)]"
# https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing
allow-indexing-slicing-in-tests = true

# "Whether panic should be allowed in test functions or #[cfg(test)]"
# https://rust-lang.github.io/rust-clippy/master/index.html#panic
allow-panic-in-tests = true

# "Whether unwrap should be allowed in test functions or #[cfg(test)]"
# https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_used
allow-unwrap-in-tests = true

For information on configuring lint behavior, see: rust-lang/rust-clippy: README.md - "Configure the behavior of some lints"

3 Likes