Observe close(2) errors for std::fs::{copy, write}

[tbu-]

Adds a (private to std) File::close method and uses it in std::fs::write and std::fs::copy.

This is a lighter version of rust-lang/libs-team#705 that can be added even if that ACP isn't accepted, as this is purely a quality of implementation thing that could be reverted at any time. External libraries and applications can use external crates like https://crates.io/crates/close-err to achieve the same with the files they own.

CC rust-lang/libs-team#705.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[tbu-]

Even though this can't fail, I copied it for symmetry with the other implementations.

[the8472]

std::fs::{copy, write} are convenience methods, that do nothing about durability. They'll also happily truncate existing files and then leaving you stranded in the middle of the operation if something goes wrong. They don't do the rename dance, they don't use tempfiles, they don't fsync. Basically, if one is using them then one is signalling not caring much.

So I don't see why these practically least-effort methods are the ones that would warrant extra care.

Such a PR might be more appropriate to atomic write crates... except that they already do fsync, which subsumes this.

• https://github.com/untitaker/rust-atomicwrites/blob/c1b223ad040939501df1a98aeaf19f86f49a7302/src/lib.rs#L200-L203
• https://docs.rs/atomic-write-file/0.3.0/atomic_write_file/index.html#how-it-works).](https://docs.rs/atomic-write-file/0.3.0/atomic_write_file/index.html#how-it-works

[tbu-]

We're just returning the error from close(2) that we're calling anyway. I'm just changing the code from ignoring the error to returning it. Why is that such a problem? The OS is telling us something, and you'd prefer to throw the error away over returning it to the user?

[the8472]

Well, I both do not like the precedent, and I also think that if anything really needs to do this then these are the methods where it's the least necessary, which makes the first point worse "look, even these simple, least-defensive std methods do it!"

[Mark-Simulacrum]

I'm personally not a huge fan of incrementally doing this in semi-random places in the standard library that operate on files internally and happen to already handle some errors. At minimum, it makes it harder to read code by adding extra operations.

I'm going to mark this waiting on T-libs. I think it makes sense for T-libs and T-libs-api to discuss what we want as policy here for internal implementations and external implementations before we land changes either way.

[tbu-]

I'm personally not a huge fan of incrementally doing this in semi-random places in the standard library that operate on files internally and happen to already handle some errors. At minimum, it makes it harder to read code by adding extra operations.

I tried to not make it "semi-random places in the standard library", but "all places in the standard library that drop a file after writing to it", i.e. all the places where the calling code can't close the file itself.

[tbu-]

I'm going to mark this waiting on T-libs. I think it makes sense for T-libs and T-libs-api to discuss what we want as policy here for internal implementations and external implementations before we land changes either way.

Why does this need T-libs/T-libs-api decisions on policy?

This looks like a simple quality of implementation thing. It does not regress any use cases. It improves some (see rust-lang/libs-team#705). It doesn't do anything special a normal rust crate couldn't do. It can be reverted if it causes problems. It's not a huge diff, so it's not hard to maintain.

[ChrisDenton]

This was discussed in the libs meeting. The discussion in the ACP is still on going so we'd prefer to see the discussion there continue before deciding on this.

[the8472]

With the ACP closed I propose to also close this.

There's technically nothing wrong with returning errors from those places, but we should not create the impression that these convenience methods do anything special resulting in higher reliability than almost all other File APIs in std and that those other APIs should be avoided.

I believe that the error-swallowing behavior in File/OwnedFd's drop impl already is something of a de-facto policy in std that we don't treat close() as a meaningful/reliable/portable error return channel (other than EBADF) and that we should not start blurring that line.

@rfcbot fcp close libs

[rust-rfcbot]

Team member @the8472 has proposed to close this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @cuviper
• @joshtriplett
• @the8472
• @thomcc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[tbu-]

There's technically nothing wrong with returning errors from those places,

Correct.

but we should not create the impression that these convenience methods do anything special resulting in higher reliability than almost all other File APIs in std and that those other APIs should be avoided.

Is your imagined scenario that people will avoid these other APIs? I see little reason to believe that. It's just convenience for people who want to observe these errors. It comes at no performance cost, too.

I think the ACP closure is relatively irrelevant here, as I already pointed out in the very first comment, it's just a "return errors where we already know about them and are in a position to return them instead of explicitly throwing them away". It's not even an API promise, it's just an implementation detail.

If it turns out that this is confusing for people, we can always revert it.

[cuviper]

I don't love the decision against the File::close ACP, but in the spirit of "disagree and commit," I think these APIs should remain consistent and not expose close errors either. Essentially, I'm echoing this sentiment:

we should not create the impression that these convenience methods do anything special resulting in higher reliability than almost all other File APIs in std and that those other APIs should be avoided.

@tbu-

Is your imagined scenario that people will avoid these other APIs? I see little reason to believe that. It's just convenience for people who want to observe these errors.

I do think it will discourage the other APIs, yes. But a crate like close-err could also add convenience methods similar to std::fs::{copy,write} but reporting close errors, as this is their domain.

[tbu-]

But a crate like close-err could also add convenience methods similar to std::fs::{copy,write} but reporting close errors, as this is their domain.

close-err can easily provide a close method on File, it's just calling a simple OS function. close-err providing std::fs::{copy, write} as good as std is rather unlikely, as std contains specialized copying routines for various operating systems, and likely to be updated in the future. The minimal patch to support this in std is far smaller.

I do think it will discourage the other APIs, yes.

Fair then I guess. Although the following point still applies:

It's not even an API promise, it's just an implementation detail. If it turns out that this is confusing for people, we can always revert it.

[the8472]

It's not even an API promise, it's just an implementation detail. If it turns out that this is confusing for people, we can always revert it.

Is contradicted by the earlier comment

It's just convenience for people who want to observe these errors.

If you "want" to do this you'd have to know about that implementation detail and then rely on an implementation detail rather than the API contract.

[tbu-]

If you "want" to do this you'd have to know about that implementation detail and then rely on an implementation detail rather than the API contract.

Yes, just like what you do when you rely on other implementation details, like knowing which syscalls get called by std to implement your seccomp syscall filtering — you know you yourself are to blame if you upgrade your toolchain without checking whether the implementation details are still the same.

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔