RFC: Do not panic when failing to release resources in Drop impls.

[adamreichold]

C.f. rust-lang/lang-team#97 for a general discussion on why this is undesirable.

[RazrFalcon]

Can it even fail? Based on munmap/UnmapViewOfFile man, it can fail only when we try to unmap random memory, which is impossible with the current code.

On the other hand, someone can unmap it manually, since ptr is publicly visible. But I guess this is not our problem. And it doesn't violate the logic.

Honestly, I think we can replace assert with eprintln here. Or is it also able to panic? (UPD: yes, print can panic too... we can use custom writer here, maybe)

And probably we can add a comment in each Drop impl explaining why we don't check the result.

[adamreichold]

Can it even fail? Based on munmap/UnmapViewOfFile man, it can fail only when we try to unmap random memory, which is impossible with the current code.

I do suspect that this falls into the "logic error" rather than "runtime error" category. But that would also be an argument for demoting this to a debug_assert! or ignoring it outright instead of forcing every user to codegen the unwinding.

On the other hand, someone can unmap it manually, since ptr is publicly visible. But I guess this is not our problem. And it doesn't violate the logic.

Since this would be calling an unsafe function, user code would be responsible to avoid duplicate unmapping, i.e. I agree that we do not need to handle this.

Honestly, I think we can replace assert with eprintln here. Or is it also able to panic? (UPD: yes, print can panic too... we can use custom writer here, maybe)

I don't think we should write to stderr in a library. This might not even be connected to something that a human reads. I would suggest just following the lead of the standard library and swallow these errors.

And probably we can add a comment in each Drop impl explaining why we don't check the result.

Will do with a link to the rust issue.

[RazrFalcon]

Agree.

[adamreichold]

Thanks for the quick response! I will not try to land anything else today.

[RazrFalcon]

Thanks again! I will publish a new release soon.

[adamreichold]

@RazrFalcon @Amanieu I replaced the assert! by ignoring errors based on looking at std types like FileDesc, OwnedHandle, Mutex and BufWriter which all ignore errors in their Drop impls but reading

Assertions for things that should never happen, such as failures in close, munmap, pthread_mutex_destroy, etc. If any of these calls fail then something has gone seriously wrong (double-free?) and we are better off aborting immediately.

in rust-lang/lang-team#97 makes me wonder whether we should call std::process::abort here instead?

I am uncomfortable with unwinding due to the effects on calling code, with ignoring errors as I fear making a vulnerability exploitable by not aborting but aborting also seems somewhat "impolite" when done from a library. Do you think we should explicitly abort here?

Also even if we keep ignoring this in release builds, it seems we might want to turn this into a debug_assert to better catch implementation errors (e.g. incorrecly aligning ptr) when working on this crate? Or since people might want to run with debug assertions enable, to abort/panicking for #[cfg(test)]?

[RazrFalcon]

Error cannot happen anyway. So there is not reason to abort.

And since we're providing an access to a raw pointer, we cannot guarantee anything. So ignoring is the best solution.