Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use try_reserve and panic in Vec's io::Write #84612

Closed
wants to merge 1 commit into from

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Apr 27, 2021

io::Write is already fallible in principle, so it makes sense to be actually fallible for Vec too.

This enables use of fallible allocation for Vec without exposing the yet-undecided TryAllocError.

I've changed original PR from returning io::Error to panicking, to de-risk the change for cases where let _ = vec.write() may have been used.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 27, 2021
@jyn514 jyn514 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 27, 2021
@Stargateur
Copy link
Contributor

If we change latter this would not be considered as breaking change ?

BTW, that why almost everything should return result

@kornelski
Copy link
Contributor Author

ErrorKind is marked as #[non_exhaustive], so adding of new cases is not a breaking change.

@Stargateur
Copy link
Contributor

Stargateur commented Apr 30, 2021

I mean remove it later.

This patch say "on oom an error of type other will be trigger" changing to a dedicated error later would be breaking change no ?

@kornelski
Copy link
Contributor Author

io::Write doesn't give you any guarantees about the error you will get, and the docs already warn against interpreting Other as anything special.

@Stargateur
Copy link
Contributor

Can you please quote and link it ?

@Stargateur
Copy link
Contributor

Stargateur commented Apr 30, 2021

Ah find it:

Errors that are Other now may move to a different or a new ErrorKind variant in the future. It is not recommended to match an error against Other and to expect any additional characteristics, e.g., a specific Error::raw_os_error return value. https://doc.rust-lang.org/std/io/enum.ErrorKind.html

Well, I have no objection anymore. It's perfect.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 1, 2021
Add ErrorKind::OutOfMemory

Ability to express `ENOMEM` as an `io::Error`.

I've used `OutOfMemory` as opposed to `NotEnoughMem` or `AllocationFailed`, because "OOM" is used in Rust already.

See also rust-lang#84612
@bjorn3
Copy link
Member

bjorn3 commented May 1, 2021

There may be code that assumes that writing to a Vec will never give an error result and thus use for example let _ = . This change would silently cause such code to give incorrect results on OOM.

@Stargateur
Copy link
Contributor

There may be code that assumes that writing to a Vec will never give an error result and thus use for example let _ = . This change would silently cause such code to give incorrect results on OOM.

they choice to ignore error... we can't prevent bad code. If you choice to ignore error, you choice it. Anyway, OOM is too rare to affect code that ignore error.

@bjorn3
Copy link
Member

bjorn3 commented May 1, 2021

they choice to ignore error... we can't prevent bad code. If you choice to ignore error, you choice it.

Up till now it was a completely reasonable choice as there was literally no way that it could produce an error.

Anyway, OOM is too rare to affect code that ignore error.

It will make for hard to debug problems. Currently OOM will just crash the program, but after this change it will cause it to silently give a bogus result. The fact that OOM's are so rare is precisely why this is a problem. If OOM's were common, the code would still break with this PR, but at least it would relatively consistently break, which makes it easy to fix before the hitting production. Because OOM's are uncommon, the incorrect handling will almost guaranteed hit production.

@Stargateur
Copy link
Contributor

Up till now it was a completely reasonable choice as there was literally no way that it could produce an error.

That what we call implementation behavior, the function return a result, nothing said it's guarantee to not error. So, not it was not a completely reasonable choice.

@kornelski
Copy link
Contributor Author

kornelski commented May 1, 2021

If I understand correctly, you're describing Hyrum's Law here. Users may have created a trap for themselves by relying on an aspect of an API that has never been promised or documented.

Write to a Vec could always fail, but the implementation wasn't reporting it the way io::Write promised. So in a way this change is a bug fix, and situation you're describing is users relying on a bug. Such situations are hypothetically possible with any bug fix. I don't think this should be an overriding concern here.

  • Vec has extend_from_slice and many other methods that are more convenient and optimize better than io::Write, so I think it's very unlikely that anybody has specifically used that interface for Vec only. io::Write is used generically, and in generic code implementers need to deal with errors, because they can't rely on files, sockets, and everything else never failing.

  • Reporting io::Error is the correct use of the io::Write trait. If we decide that we can't make this change, then it effectively redefines this trait to "returns io::Error for all types except Vec, which aborts". This is an ugly API. I don't think a hypothetical concern is worth redefining io::Write to officially and permanently have such a wart.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 2, 2021
Add ErrorKind::OutOfMemory

Ability to express `ENOMEM` as an `io::Error`.

I've used `OutOfMemory` as opposed to `NotEnoughMem` or `AllocationFailed`, because "OOM" is used in Rust already.

See also rust-lang#84612
library/std/src/io/impls.rs Outdated Show resolved Hide resolved
@m-ou-se m-ou-se added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 4, 2021
@m-ou-se
Copy link
Member

m-ou-se commented May 4, 2021

@rfcbot merge

This makes it possible for vec.write() to return an Err when not enough memory is available. There might be code around that assumes writing to a vector never fails, and uses let _ = vec.write(..); or similar. See #84612 (comment) and #84612 (comment) for discussion.

@rust-lang/libs curious to hear your opinions.

@rfcbot
Copy link

rfcbot commented May 4, 2021

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

Concerns:

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.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label May 4, 2021
@Amanieu
Copy link
Member

Amanieu commented May 30, 2021

Support for oom=panic will also solve the immediate OOM issues I have, so if oom=panic may land soon, then io::Write workaround is not so urgent for me.

oom=panic is not implemented yet and work on it has no started yet. Would you like to work on the implementation?

@m-ou-se

This comment has been minimized.

@bors

This comment has been minimized.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2021
@m-ou-se m-ou-se changed the title Use try_reserve in Vec's io::Write Use try_reserve and panic in Vec's io::Write Jun 5, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 5, 2021

@bors r-

Support for oom=panic will also solve the immediate OOM issues I have, so if oom=panic may land soon, then io::Write workaround is not so urgent for me.

Alright, let's go for that then.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 5, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 10, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 27, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 12, 2021
@bstrie
Copy link
Contributor

bstrie commented Jul 28, 2021

Triage here, I'm unclear on the status of this PR, is it being postponed while other avenues are pursued?

@yaahc
Copy link
Member

yaahc commented Aug 5, 2021

Triage here, I'm unclear on the status of this PR, is it being postponed while other avenues are pursued?

yea, I think this is currently blocked on oom=panic being implemented. I've got that on the list of action items for the error handling project group to get to for when someone comes along looking to contribute but for now nobody is working on it AFAIK.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 22, 2021
@kornelski kornelski closed this Aug 30, 2021
@kornelski kornelski deleted the oomwrite branch August 30, 2021 21:14
@kornelski kornelski restored the oomwrite branch October 12, 2022 10:38
@kornelski kornelski reopened this Oct 12, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 12, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.