RAII for Limits.max_alloc

[Shnatsel]

Right now the Limits struct provides methods to reserve and free memory from within its memory budget by calling .reserve() and .free(). This is a powerful API, but it is easy to forget to write .free() exactly once with the right amount in all error paths. It will also not be invoked on panic.

Possible solutions

This is essentially the same problem as memory allocation in C. It is solved with drop guards: instead of requiring the programmer to call free, the reserving function returns a type that frees automatically once it is dropped for any reason - end of scope, errors and panics are all covered.

A straightforward drop guard must maintain a reference to the original value; this conflicts with Rust's ownership system and the API of ImageDecoder.set_limit() accepting Limits rather than a borrow of it. We could make it accept &Limits and immediately clone . But that would require an API break. This crate provides a template for what drop guards could look like.

We might be able to get away with a simpler design: instead of mutating an instance of Limits with reserve/free calls that must match, we can deprecate those and make a single method, .reserved() or something, that returns a new #[must_use] instance of Limits with that much less max_alloc. That new instance can then be passed down the call stack. Then we never run into the need to call .free() because the modified copy is subject to RAII and the original is still around, untouched.

[Shnatsel]

So I've prototyped the .reserved() design and I can't say it's particularly great: https://github.com/Shnatsel/image/tree/raii-limits

There seems to be no way to put #[must_use] on the returned Limits. So it's easy to assume the method mutates the instance of the Limits that you have when it actually doesn't. And I don't want to introduce a confusing API.

It is possible to put #[must_use] on Limits and enforce the use of the return value that way!

[Shnatsel]

On Discord, moulins suggested enforcing stacking of Limits using a lifetime:

struct Limits<'a>(...);

impl<'a> Limit<'a> {
  #[must_use]
  pub fn reserved(&mut self, bytes: usize) -> ImageResult<Limits<'_>>;
}

[fintelia]

I think it would be easier to think about this in the context of a specific decoder's use of the Limits object. For some decoders, all the actual allocations happen in a dependency and we simply use the value in limits to set the relevant parameter for that crate. In other cases, the allocation might happen in this crate but be on the heap and outlive the specific function that does the allocation.

One other thing from looking at the Limits struct is that I'm not particular sure why its methods are public in the first place? The main place you'd want to call them are from the implementations of decoders. But because decoders are generally either implemented within this crate or imported as dependencies (and thus would cause circular dependencies if their tried to use types from this crate) that doesn't really help.

[Shnatsel]

I'll work on adding limits to animation decoders such as GIF using the existing API, then see if there's any benefit to changing it once we have more internal use of the Limits.reserve() API.