Handle out of memory errors in io::Read::read_to_end

[kornelski]

I'd like to propose handling of out-of-memory errors in the default implementation of io::Read::read_to_end() and fs::read(). These methods create/grow a Vec with a size that is external to the program, and could be arbitrarily large.

Due to being I/O methods, they can already fail in a variety of ways, in theory even including ENOMEM from the OS too, so another failure case should not surprise anyone.

While this may not help much Linux with overcommit, it's useful for other platforms like WASM. Internals thread.

I haven't changed implementation of impl Read for &[u8] and VecDeque out of caution, because in these cases users could assume read can't fail.

This code uses try_reserve() + extend_from_slice() which is optimized since #117503.

[rustbot]

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

[joshtriplett]

This does affect the API surface, so:

@rfcbot merge

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

Concerns:

• documented guarantee (Handle out of memory errors in io::Read::read_to_end #116570 (comment))

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.

[BurntSushi]

Is this error condition something we're going to guarantee? If so, it should probably be in the docs.

The litmus test here is this: if we merge this, and then some time later for some reason to decide to change it back, could we do that without it being considered a breaking change in behavior? I suspect not. And if not, I think that means we might as well document the behavior.

@rfcbot concern documented guarantee

[the8472]

@BurntSushi that seems like circular reasoning to me. We can break that which we haven't guaranteed not to break.
If we only guarantee what's in the API/documentation then we can change implementation details however we like. If implementation details become part of the contract... then... uh, we never can change anything at all.
Anything between that is best-effort and not a guarantee.

[BurntSushi]

We can break that which we guaranteed not to break.

Not sure what you mean. Is this something we guarantee we won't break in the future?

If it is, then we ought to document it.

If it isn't, then this probably should be a libs fcp and not a libs-api fcp.

[BurntSushi]

To be clear, IMO, I believe this ought to be an API change and something we guarantee. And thus it should go in the docs and thus folks can rely on these routines returning an error in an out-of-memory condition.

[the8472]

That's complicated by

I haven't changed implementation of impl Read for &[u8] and VecDeque out of caution, because in these cases users could assume read can't fail.

which means we'd make this guarantee conditionally, not for all R: Read, which would make it far less useful for things that take generic readers.

Not sure what you mean.

My bad, I made a critical typo, missing a negation.

[kornelski]

The difficulty here is that there likely are 3rd party implementations of read_to_end that don't handle OOM. It can't be enforced by Rust or libstd. So it's a quality of implementation issue rather than an API contract.

Of course it can be documented what libstd does, perhaps with a recommendation for Read trait implementers to follow the example and handle OOM too.

[BurntSushi]

@the8472 I feel like you're responding to my comments here very narrowly. Can we please take a more expansive view here? My understanding is that this PR changes the behavior of some code to presumably be more useful in some contexts than it was previously by returning an error in OOM instead of the usual OOM behavior. Is that correct?

If so, then we should make a choice about whether we want to guarantee this more useful behavior going forward, however limited it is. If we want to, then we ought to document it as a guarantee in whatever way makes sense.

If we don't want to, then arguably this is "just" an implementation detail and ought not be decided by libs-api.

I bring this up because @joshtriplett said:

This does affect the API surface, so

But the fact that this PR changes zero wording in the docs made me pause and wonder whether we actually ought to treat this as an implementation detail or whether we truly do want to treat this as an API change.

[BurntSushi]

The difficulty here is that there likely are 3rd party implementations of read_to_end that don't handle OOM. It can't be enforced by Rust or libstd. So it's a quality of implementation issue rather than an API contract.

Of course it can be documented what libstd does, perhaps with a recommendation for Read trait implementers to follow the example and handle OOM too.

Right... This is why I posed what I should have more explicitly labeled as a thought experiment:

1. Let's say we merge this PR as-is.
2. It gets release in a stable release.
3. People start to write code in a way that depends on the out-of-memory error happening.
4. Something comes up (I don't know what, this is why it's a thought experiment) that causes us to revert this PR.
5. The revert gets into a stable release.
6. People who previously wrote code relying on this behavior are upset.

How do we respond? "Sorry, it was an implementation detail and we were within our rights to change it." Or, "Oh shoot you're right, we didn't mean to change the behavior here and we regard this as an unintentional breaking change."

If it's the latter, then this is indeed an API change and we should figure out how to document it. If it's the former, then I don't think this is a libs-api matter.

My opinion is that it probably ought to be the latter, but I certainly do not feel strongly.

[the8472]

My understanding is that this PR changes the behavior of some code to presumably be more useful in some contexts than it was previously by returning an error in OOM instead of the usual OOM behavior. Is that correct?

That is also my understanding, but I take it as a best-effort approach that'll mostly help code that already handles all kinds of errors in a useful way.

Things that unwrap() or exit(1) on any error will continue to do so.
Things that only handle specific errors and bail on other ones will continue to do so.

Without an API guarantee that we always return ErrorKind::OutOfMemory - which we won't, even with this change - it can still make some programs on some platforms more robust.

If so, then we should make a choice about whether we want to guarantee this more useful behavior going forward, however limited it is. If we want to, then we ought to document it as a guarantee in whatever way makes sense.

If we don't want to, then arguably this is "just" an implementation detail and ought not be decided by libs-api.

Agreed. Imo it's an implementation detail.

[kornelski]

@BurntSushi I think the hypothetical response would be "Sorry, it was an implementation detail, because the io::Read trait never guaranteed that."

[BurntSushi]

All righty, then I think this shouldn't be libs-api? Any I've disagree with that? @joshtriplett?

[kornelski]

@BurntSushi you haven't removed your concern from rfcbot, so it's still blocking merge.

[BurntSushi]

@kornelski I don't think my concern has been resolved? Specifically:

Is this something we guarantee we won't break in the future?

If it is, then we ought to document it.

If it isn't, then this probably should be a libs fcp and not a libs-api fcp.

[kornelski]

It isn't a guarantee. I've updated the docs to explicitly state it's not guaranteed.

I don't have ability to change the FCP, so if it needs to be handed over, someone with access has to do it.