-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
panics from unwrap causes memory leak on background threads with FFI #708
Comments
Even if you return |
@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 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 |
|
https://stackoverflow.com/questions/56107324/why-does-rust-consider-it-safe-to-leak-memory 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
the "right" solution would be implementing proper unwinding within that library, (though some other library might also have problem with ffi unwinding...) anyway, it's just a matter of adding a suffix for |
I understand. But even if we add |
that'll require a lot of extra conditionals just to check "is it safe to call imageproc?"... anyway, in one project, that function above ( So...will |
Wait, do not rush. Which line exactly? Maybe stacktrace? |
Or better give me the image size with rect and I'll reproduce |
There is no need for try_ version of that function. It simply should never panic |
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)
would become:
The text was updated successfully, but these errors were encountered: