panics from unwrap causes memory leak on background threads with FFI

[devtempmac]

hello!
I've been using this library for some image processing, and some panics from unwraps in this library have causes leaks on my background threads. (and with rust threads defaulting to catch_unwind, they're practically invisible to me by default...)

would it be ok to add some 'safe' variants that return Result type instead of panicking?

eg)

pub fn draw_filled_rect<I>(image: &I, rect: Rect, color: I::Pixel) -> Image<I::Pixel>
where
    I: GenericImage,
{
    let mut out = Image::new(image.width(), image.height());
    out.copy_from(image, 0, 0).unwrap();
    draw_filled_rect_mut(&mut out, rect, color);
    out
}

would become:

pub fn draw_filled_rect_safe<I>(image: &I, rect: Rect, color: I::Pixel) -> ImageResult<Image<I::Pixel>>
where
    I: GenericImage,
{
    let mut out = ImageBuffer::new(image.width(), image.height());
    out.copy_from(image, 0, 0)?;
    draw_filled_rect_mut(&mut out, rect, color);
    Ok(out)
}

#[inline]
pub fn draw_filled_rect<I>(image: &I, rect: Rect, color: I::Pixel) -> Image<I::Pixel>
where
    I: GenericImage,
{
   return draw_filled_rect_safe(I, rect, color).unwrap();
}

[cospectrum]

Even if you return Result instead of unwrap, most functions in Rust still won't be panic-free.
panic-free is a very complex property, almost imposible.

[devtempmac]

@cospectrum yes 100% panic-free is impossible, but these are really low-hanging fruits.

And it gives users choice to use 'panicking' function, or somewhat safer 'could be handled right after' function.

(if safe name is misleading, would safer or with_result be ok?)

I'm processing like 200+ cams with rust, and that single function change solved a memory leak of 10gb in 1 day period (and the leak wasn't even near imageproc, but in ffmpeg...)

ps: if it's too much work, I can do some pull-requests related to this

[cospectrum]

Safe is not a good suffix. There is nothing unsafe about unwrap or panic in general. Panic is the reason why slice indexing is safe, for example.
First, let's clarify which function you're referring to. And let's finally decide who is really responsible for memory leak. Because right now I don't see how usage of Result will solve any problem other than personal preference. Every change needs an argument

[devtempmac]

https://stackoverflow.com/questions/56107324/why-does-rust-consider-it-safe-to-leak-memory
unsafe doesn't mean "does not leak memory" in rust

and this memory leak not 100% imageproc's fault, but due to some unsafe+FFI implemention problem of some library I don't control.

for 'no-panic' happy path, everything goes as planned per that library's authors, so there's no leak.

but for panic-path, it seems that library doesn't handle 'panic-unwind-path' 100% to cleanup resources.

https://effective-rust.com/panic.html

Finally, panic propagation also interacts poorly with FFI (foreign function interface) boundaries (Item 34); use catch_unwind to prevent panics in Rust code from propagating to non-Rust calling code across an FFI boundary.

the "right" solution would be implementing proper unwinding within that library, (though some other library might also have problem with ffi unwinding...)
But the fastest lowest hanging fruit is making this library return Result types instead of panicking

anyway, it's just a matter of adding a suffix for Result-returning functions,
and adding inline wrapper functions so pre-existing users don't have to edit their code

[cospectrum]

I understand. But even if we add try_ functions, if you don't control the dependency, the dependency can still call the function without try, or it can call unwrap, and nothing will change for you, you still have a problem. But if you call imageproc directly, sometimes its your responsibility to ensure invariants before the call.
Btw, can you name a function that panics in your dependency? As far as I know, there are still places in imageproc where it unexpectedly panics without asserts, and such places can certainly be improved. imageproc doesn't have 100% test coverage at the moment

[devtempmac]

But if you call imageproc directly, sometimes its your responsibility to ensure invariants before the call.

that'll require a lot of extra conditionals just to check "is it safe to call imageproc?"...
With Result type, I can just call imageproc fn and see if it errs or not and handle accordingly

anyway, in one project, that function above (draw_filled_rect_safe) is the only fix I needed for imageproc
but in another one -- I'm still working on getting rid of unwraps and finding panics 🥲

So...will try_draw_filled_rect do?
I'll work on some pull requests if you're ok with this

[cospectrum]

Wait, do not rush. Which line exactly? Maybe stacktrace?

[cospectrum]

Or better give me the image size with rect and I'll reproduce

[cospectrum]

There is no need for try_ version of that function. It simply should never panic