Add `OwnedFd::try_close` method?

[newpavlov]

The rustix::io::close explain why it does not return theoretically possible errors, but in our code we already return IO errors from function which drops OwnedFd, so we can "bubble" potential errors basically for free. Doing so would remove potential panic path (unreachable in practice, but still present) and potential questions during code review.

I suggest to add inherent OwnedFd::try_close(self) -> Result<()> method for cases like this.

[sunfishcode]

OwnedFd has been upstreamed into std, so the place to request new features for it is in std.

The panic in rustix::io::close is only a debug_assert_eq, so it's not included in release builds, and I'd be willing to add configuration flags to disable it, if that'd be useful. For comparison, the std drop function's assert isn't even enabled by default in debug builds.

As an aside, I myself would hesitate to add a try_close function to OwnedFd because I don't know how to explain to users when they should use it. "Use it when it's convenient to do so" is not very satisfying advice. If try_close exists, it would arguably imply that we believe close can fail, which would arguably imply that all Rust code ever should be using it. I don't think a Rust ecosystem-wide push to eliminate all File::drop calls throughout all Rust code ever would be worthwhile, with what I know of the situation today, and I don't know where else to draw the line.

[newpavlov]

I would be fine with introducing rustix::io::try_close as well or with modifying rustix::io::close to return Result. Right now rustix simply does not expose return result of the close syscall even though EBADF and ENOSPC/EDQUOT can be encountered in practice (I am not sure about EIO and EINTR).

It's fine to have an opinionated API for wrapper types like OwnedFd, but I think rustix should properly expose return types for "syscall" functions. Otherwise, I have not choice but to fall back to libc.

[sunfishcode]

I'm not clear on how we might explain to users when they should use this rustix::io::try_close, versus using rustix::io::close or using OwnedFd to close things. Could you say more about your use case?

[newpavlov]

I would suggest for rustix::io::close to eventually return Result (i.e. in v0.38 we could temporarily introduce try_close and remove it in v0.39) with a comment explaining that almost always it's safe to ignore return result (i.e. users can write let _ = close(fd);). It would be the most straightforward solution for a "syscall" function. The OwnedFd type in turn can stay as-is.

Could you say more about your use case?

It came up during migration from libc-based code. Our application may work over network file systems (e.g. NFS or FUSE-based FS), so silently ignoring potential errors is not appropriate.

[sunfishcode]

almost always it's safe to ignore return result

What does "almost" mean here? How would a programmer determine if their situation is one is one of the ones where it actually is safe, or if they're one of the unlucky few for whom it isn't safe?

Our application may work over network file systems

The nature of network filesystems is that every application that works with files, may work over network filesystems. So I'm still unclear on how a programmer might determine if their situation is one of the ones that requires handling errors from close.

[newpavlov]

How would a programmer determine if their situation is one is one of the ones where it actually is safe, or if they're one of the unlucky few for whom it isn't safe?

By using better judgment, reading appropriate platform documentation, and analyzing problem at hand? Why do you think rustix have to make this decision? If you don't want to provide any guidance, then don't do it. Just refer to the man page and maybe state that most users should not use this function directly and instead should prefer wrappers like OwnedFd.

Returning an error (if it's easy to do) is strictly better than silencing it, however unlikely the error is in practice. I am sorry to say that, but your position looks very arrogant to me. We are talking about very low-level unsafe API, literally a very thin wrapper around OS syscall, and you treat your users as little children who just started programming.

[sunfishcode]

After more consideration, I've now submitted #1085 to add a try_close function. Enable the "try_clone" cargo feature to use it.

Also, I am someone who has read the platform documentation and analyzed the situation. I genuinely am interested in learning more answers to the questions I asked here, if anyone has any.

[newpavlov]

#1085 is merged, so this issue can be closed. Thank you!