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

impl From<TryReserveError> for io::Error #121403

Merged
merged 2 commits into from
Mar 9, 2024
Merged

Conversation

kornelski
Copy link
Contributor

There's an obvious mapping between these two errors, and it makes I/O code less noisy.

I've chosen to use simple ErrorKind::OutOfMemory io::Error, without keeping TryReserveError for the source(), because:

  • It matches current uses in libstd,
  • ErrorData::Custom allocates, which is a risky proposition for handling OOM errors specifically.
  • Currently TryReserveError has no public fields/methods, so it's usefulness is limited. How allocators should report errors, especially custom and verbose ones is still an open question.

Just in case I've added note in the doccomment that this may change.

The compiler forced me to declare stability of this impl. I think this implementation is simple enough that it doesn't need full-blown stabilization period, and I've marked it for the next release, but of course I can adjust the attribute if needed.

@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2024

r? @cuviper

rustbot has assigned @cuviper.
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 rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 21, 2024
@cuviper
Copy link
Member

cuviper commented Feb 21, 2024

Since such impls are insta-stable...
@rustbot label -T-libs +T-libs-api
r? libs-api

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 21, 2024
@rustbot rustbot assigned dtolnay and unassigned cuviper Feb 21, 2024
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Feb 21, 2024

@rust-lang/libs-api:
@rfcbot fcp merge

The standard library changes in this PR illustrate how this impl is useful, in particular when using try_reserve inside the implementation of std::io::Read::read_to_end or std::io::Read::read_to_string.

@rfcbot
Copy link

rfcbot commented Feb 21, 2024

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

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 21, 2024
@the8472
Copy link
Member

the8472 commented Feb 21, 2024

TryReserveError has two TryReserveErrorKind, one is for capacity overflow and one is for an allocation failure.
I'd say the former is more like EOVERFLOW than ENOMEM.

Currently TryReserveError has no public fields/methods, so it's usefulness is limited.

It does have kind. Sure, that's unstable, but eventually it'll probably be stabilized.

@kornelski
Copy link
Contributor Author

There's no EOVERFLOW equivalent in ErrorKind, not even in the expanded #106375.

As far as I can tell, EOVERFLOW is mainly used to report accessing >4GB files using 32-bit APIs, when there is valid data to return, it just doesn't fit the API. CapacityOverflow is not quite the same case. I don't expect POSIX implementations to have a special case for malloc(isize_max), and they'd likely report ENOMEM in all cases.

I believe that the current shape of TryReserveErrorKind is only a byproduct of RawVec internals, and the OOM handling and allocator APIs being unfinished, and may not survive as-is when they're done.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 28, 2024
@rfcbot
Copy link

rfcbot commented Feb 28, 2024

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

@the8472
Copy link
Member

the8472 commented Feb 28, 2024

As far as I can tell, EOVERFLOW is mainly used to report accessing >4GB files using 32-bit APIs, when there is valid data to return, it just doesn't fit the API

It's also used to indicate that offset calculations exceed size_t. E.g. sendfile() returns EOVERFLOW when you try to use an offset + count that's beyond what's addressable by file offsets.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 9, 2024
@rfcbot
Copy link

rfcbot commented Mar 9, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thank you!

I think ErrorKind::OutOfMemory is all right as the mapping of TryReserveErrorKind::CapacityOverflow, but if there is agreement that a different io error kind is better for it, I am confident we can follow up on that afterward without breaking anybody's code.

@dtolnay
Copy link
Member

dtolnay commented Mar 9, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 9, 2024

📌 Commit aa581f0 has been approved by dtolnay

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 9, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 9, 2024
@Nadrieril
Copy link
Member

@bors rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 9, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#99153 (Add Read Impl for &Stdin)
 - rust-lang#114655 (Make `impl<Fd: AsFd>` impl take `?Sized`)
 - rust-lang#120504 (Vec::try_with_capacity)
 - rust-lang#121280 (Implement MaybeUninit::fill{,_with,_from})
 - rust-lang#121403 (impl From<TryReserveError> for io::Error)
 - rust-lang#121526 (on the fly type casting for `build.rustc` and `build.cargo`)
 - rust-lang#121584 (bump itertools to 0.12)
 - rust-lang#121711 (Implement junction_point)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9ccf798 into rust-lang:master Mar 9, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 9, 2024
Rollup merge of rust-lang#121403 - kornelski:io-oom, r=dtolnay

impl From<TryReserveError> for io::Error

There's an obvious mapping between these two errors, and it makes I/O code less noisy.

I've chosen to use simple `ErrorKind::OutOfMemory` `io::Error`, without keeping `TryReserveError` for the `source()`, because:

* It matches current uses in libstd,
* `ErrorData::Custom` allocates, which is a risky proposition for handling OOM errors specifically.
* Currently `TryReserveError` has no public fields/methods, so it's usefulness is limited. How allocators should report errors, especially custom and verbose ones is still an open question.

Just in case I've added note in the doccomment that this may change.

The compiler forced me to declare stability of this impl. I think this implementation is simple enough that it doesn't need full-blown stabilization period, and I've marked it for the next release, but of course I can adjust the attribute if needed.
@kornelski kornelski deleted the io-oom branch March 9, 2024 23:21
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 14, 2024
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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants