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

Tracking issue for io_slice_advance #62726

Closed
1 of 3 tasks
Tracked by #7
Thomasdezeeuw opened this issue Jul 16, 2019 · 63 comments · Fixed by #62987 or #127661
Closed
1 of 3 tasks
Tracked by #7

Tracking issue for io_slice_advance #62726

Thomasdezeeuw opened this issue Jul 16, 2019 · 63 comments · Fixed by #62987 or #127661
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC 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. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Thomasdezeeuw
Copy link
Contributor

Thomasdezeeuw commented Jul 16, 2019

This is a tracking issue for IoSlice::{advance, advance_slices} and IoSliceMut::{advance, advance_slices}.

Feature gate: #![feature(io_slice_advance)].

Steps:

Current API additions:

impl<'a> IoSlice<'a> { // And `IoSliceMut`
    pub fn advance(&mut self, n: usize);
    pub fn advance_slices(bufs: &mut &mut [IoSlice<'a>], n: usize);
}

Old issue:

Writing rust-lang/futures-rs#1741 I needed to resort to unsafe code to change the underlying slice in IoSlice (and IoSliceMut). I'm missing a method that can change the underlying slice. @Nemo157 said that I should open an issue.

Current idea would be something like Buf::advance from the bytes crate.

impl IoSlice {
    // Advance the internal cursors of the slice by `n` bytes.
    fn advance(&mut self, n: usize) {
        // ..
    }
}
@Thomasdezeeuw
Copy link
Contributor Author

Looking for some feedback as to whether this would be accepted.

@jonas-schievink jonas-schievink added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 16, 2019
Centril added a commit to Centril/rust that referenced this issue Aug 5, 2019
…omasdezeeuw

Add {IoSlice, IoSliceMut}::advance

API inspired by the [`Buf::advance`](https://docs.rs/bytes/0.4.12/bytes/trait.Buf.html#tymethod.advance) method found in the [bytes](https://docs.rs/bytes) crate.

Closes rust-lang#62726.
bors added a commit that referenced this issue Aug 6, 2019
Add {IoSlice, IoSliceMut}::advance

API inspired by the [`Buf::advance`](https://docs.rs/bytes/0.4.12/bytes/trait.Buf.html#tymethod.advance) method found in the [bytes](https://docs.rs/bytes) crate.

Closes #62726.
@Thomasdezeeuw Thomasdezeeuw changed the title Add IoSlice advance or similiar method Tracking issue for io_slice_advance Aug 6, 2019
@Thomasdezeeuw
Copy link
Contributor Author

Can someone reopen this as the tracking issue for io_slice_advance? I've updated the the first comment, but I'm not sure it's 100% correct.

@jonas-schievink jonas-schievink added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC B-unstable Blocker: Implemented in the nightly compiler and unstable. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Aug 6, 2019
@seanmonstar
Copy link
Contributor

I was trying to impl Buf for IoSlice in the bytes crate, and noticed there's currently an issue with lifetimes:

impl Buf for IoSlice<'_> {
    fn advance(&mut self, cnt: usize) {
        *self = IoSlice::new(&self[cnt..]); // conflicting lifetime requirements
    }
}

To allow such a thing externally, we could add a method to IoSlice that gives the slice with the same lifetime as IoSlice (whereas Deref uses an anonymous lifetime).

impl<'a> IoSlice<'a> {
    pub fn inner_ref(&self) -> &'a [u8] {
        // ...
    }
}

@Thomasdezeeuw
Copy link
Contributor Author

@seanmonstar can get around the lifetime issues by replacing self with an empty slice and then replacing it again with the advanced slice? Something like can be found here: https://github.com/rust-lang/rust/pull/62987/files#diff-668f8f358d4a93474b396dcb3727399eR975.

@seanmonstar
Copy link
Contributor

@Thomasdezeeuw I don't think so. It's not possible to get the inner slice with the same lifetime due to Deref:

let tmp = std::mem::replace(self, IoSlice::new(&[]));
let tail = &tmp[cnt..]; // <- lifetime isn't same as `IoSlice<'a>`
*self = IoSlice::new(tail);

@Nemo157
Copy link
Member

Nemo157 commented Oct 16, 2019

@seanmonstar it's not an amazing API to build that on top of, but it is possible (playground):

let mut slices = [std::mem::replace(self, IoSlice::new(&[]))];
let slices = IoSlice::advance(&mut slices[..], cnt);
match slices {
    [slice] => {
        *self = std::mem::replace(slice, IoSlice::new(&[]));
    }
    [] => {
        *self = IoSlice::new(&[]);
    }
    _ => unreachable!(),
}

@seanmonstar
Copy link
Contributor

Oh right, that is using the unstable function. I was referring to be able to implement this now.

@Nemo157
Copy link
Member

Nemo157 commented Oct 16, 2019

Ah, I just assumed since that's what the tracking issue is about. It seems like having IoSlice<'a>::into_inner(self) -> &'a [u8] along with Clone and Copy would make sense since it's just a newtyped slice (hopefully that's possible with the actual os-specific implementations).

@mpdn
Copy link
Contributor

mpdn commented Nov 10, 2019

I've been trying to write a write_all_vectored function that continuously calls write_vectored, but it seems like this is not really possible to do without some way of advancing the IoSlices (or resorting to unsafe, of course). It does not seem to me like vectored IO is very practical without this feature or something equivalent.

@Ekleog
Copy link

Ekleog commented Jun 5, 2020

Are there any news on landing this? I'm trying to do vectored IO too, and it would be very nice if it were possible to just use this function — I think this is the only unstable feature I currently depend on, and it would be very nice to be able to compile on stable without having to add unsafe code.

into_inner as discussed above is another primitive that would make it possible to implement advance as a user, and it would most likely be great to have it, but seeing the way vectored IO is done I can't see a world where a function like advance isn't required.

@KodrAus KodrAus added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` I-nominated Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
pkhuong added a commit to pkhuong/woodpile that referenced this issue May 26, 2024
@PureWhiteWu
Copy link

Hi, is there any progress of this feature?

@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 Jul 2, 2024
@rfcbot
Copy link

rfcbot commented Jul 2, 2024

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 12, 2024
@rfcbot
Copy link

rfcbot commented Jul 12, 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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jul 12, 2024
@bors bors closed this as completed in 68ec9c1 Jul 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 13, 2024
Rollup merge of rust-lang#127661 - eduardosm:stabilize-io_slice_advance, r=cuviper

Stabilize io_slice_advance

Closes rust-lang#62726 (FCP completed)

Stabilized API:

```rust
impl<'a> IoSlice<'a> {
    pub fn advance(&mut self, n: usize);
    pub fn advance_slices(bufs: &mut &mut [IoSlice<'a>], n: usize);
}

impl<'a> IoSliceMut<'a> {
    pub fn advance(&mut self, n: usize);
    pub fn advance_slices(bufs: &mut &mut [IoSliceMut<'a>], n: usize);
}
```
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jul 22, 2024
@zh-jq-b
Copy link

zh-jq-b commented Aug 2, 2024

The following assert won't work after this if the reader call IoSliceMut::advance() in it's read_vectored().

fn what_to_read<R>(reader: R)
where
    R: Read,
{
    let mut b1 = [0u8; 16];
    let mut b2 = [0u8; 32];

    let mut iov = [IoSliceMut::new(&mut b1), IoSliceMut::new(&mut b2)];
    let nr = reader.read_vectored(&mut iov).unwrap();

    if nr > 0 {
        assert_eq!(b1[0], iov[0][0]);
    }
    if nr >= b1.len() {
        assert_eq!(iov[0].len(), b1.len());
    }
}

@Thomasdezeeuw
Copy link
Contributor Author

@zh-jq-b in your example you don't call IoSliceMut::advance, so I don't see how it's relevant to the method?

@zh-jq-b
Copy link

zh-jq-b commented Aug 2, 2024

@zh-jq-b in your example you don't call IoSliceMut::advance, so I don't see how it's relevant to the method?

Yes I didn't call IoSliceMut::advance but the reader.read_vectored() may call that indirectly?
What I want to say is that, there is no guranteen that IoSliceMut::advance won't get called inside reader.read_vectored() or any other existed functions that take a IoSliceMut param as a way to receive data.

@Thomasdezeeuw
Copy link
Contributor Author

I don't think we can guarantee that the method will never be incorrectly used. Same for process::exit for example... I don't know what you expect the standard library to change about this.

@zh-jq-b
Copy link

zh-jq-b commented Aug 2, 2024

I don't think we can guarantee that the method will never be incorrectly used. Same for process::exit for example... I don't know what you expect the standard library to change about this.

Before the stabilization of io_slice_advance, users can use both the original buffer and also the iov to get the read data.
After the stabilization, I'm confused with which one should be used?
For all old methods that take a IoSliceMut, should I still read the data by using the old way? Will it broken without noticeable API changes?

@dead-claudia
Copy link

@zh-jq-b Consider filing a bug report against anyone who sets them incorrectly.

It's not Rust's fault if someone implements read_vectored wrong. It's the fault of whoever wrote the bad implementation for that io::Read trait instance method.

@zh-jq-b
Copy link

zh-jq-b commented Aug 6, 2024

@zh-jq-b Consider filing a bug report against anyone who sets them incorrectly.

It's not Rust's fault if someone implements read_vectored wrong. It's the fault of whoever wrote the bad implementation for that io::Read trait instance method.

The read_vectored should be documented to specify what's the wrong implementation. So I opened #128669 for further confirmation.

facebook-github-bot pushed a commit to facebook/buck2 that referenced this issue Oct 7, 2024
Summary:
Upgrading Rust from `1.80.1` to `1.81.0` ([release notes](https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html)) with the following changes affecting `fbsource`:
* Feature stabilizations:
  * `io_slice_advance` ([#62726](rust-lang/rust#62726))
  * `panic_info_message` ([#66745](rust-lang/rust#66745))
  * `fs_try_exists` ([#83186](rust-lang/rust#83186))
  * `lint_reasons` ([#120924](rust-lang/rust#120924))
  * `duration_abs_diff` ([#117618](rust-lang/rust#117618))
  * `effects` ([#102090](rust-lang/rust#102090))
* New `clippy` lints:
  * `manual_inspect` ([#12287](rust-lang/rust-clippy#12287))
  * `manual_pattern_char_comparison` ([#12849](rust-lang/rust-clippy#12849))
* Other:
  * `NEVER_TYPE_FALLBACK_FLOWING_INTO_UNSAFE` became a deny-by-default lint ([#126881](rust-lang/rust#126881) & [#123748](rust-lang/rust#123748))
  * Changes to `stringify!` introducing whitespaces between some tokens ([#125174](rust-lang/rust#125174))
  * `format!` is now marked with a `must_use` hint ([#127355](rust-lang/rust#127355))

ignore-conflict-markers

Reviewed By: zertosh, dtolnay

Differential Revision: D63864870

fbshipit-source-id: c8d61f3e9483ec709c8116514cfb030c883ec262
facebook-github-bot pushed a commit to facebookexperimental/rust-shed that referenced this issue Oct 7, 2024
Summary:
Upgrading Rust from `1.80.1` to `1.81.0` ([release notes](https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html)) with the following changes affecting `fbsource`:
* Feature stabilizations:
  * `io_slice_advance` ([#62726](rust-lang/rust#62726))
  * `panic_info_message` ([#66745](rust-lang/rust#66745))
  * `fs_try_exists` ([#83186](rust-lang/rust#83186))
  * `lint_reasons` ([#120924](rust-lang/rust#120924))
  * `duration_abs_diff` ([#117618](rust-lang/rust#117618))
  * `effects` ([#102090](rust-lang/rust#102090))
* New `clippy` lints:
  * `manual_inspect` ([#12287](rust-lang/rust-clippy#12287))
  * `manual_pattern_char_comparison` ([#12849](rust-lang/rust-clippy#12849))
* Other:
  * `NEVER_TYPE_FALLBACK_FLOWING_INTO_UNSAFE` became a deny-by-default lint ([#126881](rust-lang/rust#126881) & [#123748](rust-lang/rust#123748))
  * Changes to `stringify!` introducing whitespaces between some tokens ([#125174](rust-lang/rust#125174))
  * `format!` is now marked with a `must_use` hint ([#127355](rust-lang/rust#127355))

ignore-conflict-markers

Reviewed By: zertosh, dtolnay

Differential Revision: D63864870

fbshipit-source-id: c8d61f3e9483ec709c8116514cfb030c883ec262
facebook-github-bot pushed a commit to facebookincubator/buck2-change-detector that referenced this issue Oct 7, 2024
Summary:
Upgrading Rust from `1.80.1` to `1.81.0` ([release notes](https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html)) with the following changes affecting `fbsource`:
* Feature stabilizations:
  * `io_slice_advance` ([#62726](rust-lang/rust#62726))
  * `panic_info_message` ([#66745](rust-lang/rust#66745))
  * `fs_try_exists` ([#83186](rust-lang/rust#83186))
  * `lint_reasons` ([#120924](rust-lang/rust#120924))
  * `duration_abs_diff` ([#117618](rust-lang/rust#117618))
  * `effects` ([#102090](rust-lang/rust#102090))
* New `clippy` lints:
  * `manual_inspect` ([#12287](rust-lang/rust-clippy#12287))
  * `manual_pattern_char_comparison` ([#12849](rust-lang/rust-clippy#12849))
* Other:
  * `NEVER_TYPE_FALLBACK_FLOWING_INTO_UNSAFE` became a deny-by-default lint ([#126881](rust-lang/rust#126881) & [#123748](rust-lang/rust#123748))
  * Changes to `stringify!` introducing whitespaces between some tokens ([#125174](rust-lang/rust#125174))
  * `format!` is now marked with a `must_use` hint ([#127355](rust-lang/rust#127355))

ignore-conflict-markers

Reviewed By: zertosh, dtolnay

Differential Revision: D63864870

fbshipit-source-id: c8d61f3e9483ec709c8116514cfb030c883ec262
facebook-github-bot pushed a commit to facebook/starlark-rust that referenced this issue Oct 7, 2024
Summary:
Upgrading Rust from `1.80.1` to `1.81.0` ([release notes](https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html)) with the following changes affecting `fbsource`:
* Feature stabilizations:
  * `io_slice_advance` ([#62726](rust-lang/rust#62726))
  * `panic_info_message` ([#66745](rust-lang/rust#66745))
  * `fs_try_exists` ([#83186](rust-lang/rust#83186))
  * `lint_reasons` ([#120924](rust-lang/rust#120924))
  * `duration_abs_diff` ([#117618](rust-lang/rust#117618))
  * `effects` ([#102090](rust-lang/rust#102090))
* New `clippy` lints:
  * `manual_inspect` ([#12287](rust-lang/rust-clippy#12287))
  * `manual_pattern_char_comparison` ([#12849](rust-lang/rust-clippy#12849))
* Other:
  * `NEVER_TYPE_FALLBACK_FLOWING_INTO_UNSAFE` became a deny-by-default lint ([#126881](rust-lang/rust#126881) & [#123748](rust-lang/rust#123748))
  * Changes to `stringify!` introducing whitespaces between some tokens ([#125174](rust-lang/rust#125174))
  * `format!` is now marked with a `must_use` hint ([#127355](rust-lang/rust#127355))

ignore-conflict-markers

Reviewed By: zertosh, dtolnay

Differential Revision: D63864870

fbshipit-source-id: c8d61f3e9483ec709c8116514cfb030c883ec262
facebook-github-bot pushed a commit to facebookincubator/reindeer that referenced this issue Oct 7, 2024
Summary:
Upgrading Rust from `1.80.1` to `1.81.0` ([release notes](https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html)) with the following changes affecting `fbsource`:
* Feature stabilizations:
  * `io_slice_advance` ([#62726](rust-lang/rust#62726))
  * `panic_info_message` ([#66745](rust-lang/rust#66745))
  * `fs_try_exists` ([#83186](rust-lang/rust#83186))
  * `lint_reasons` ([#120924](rust-lang/rust#120924))
  * `duration_abs_diff` ([#117618](rust-lang/rust#117618))
  * `effects` ([#102090](rust-lang/rust#102090))
* New `clippy` lints:
  * `manual_inspect` ([#12287](rust-lang/rust-clippy#12287))
  * `manual_pattern_char_comparison` ([#12849](rust-lang/rust-clippy#12849))
* Other:
  * `NEVER_TYPE_FALLBACK_FLOWING_INTO_UNSAFE` became a deny-by-default lint ([#126881](rust-lang/rust#126881) & [#123748](rust-lang/rust#123748))
  * Changes to `stringify!` introducing whitespaces between some tokens ([#125174](rust-lang/rust#125174))
  * `format!` is now marked with a `must_use` hint ([#127355](rust-lang/rust#127355))

ignore-conflict-markers

Reviewed By: zertosh, dtolnay

Differential Revision: D63864870

fbshipit-source-id: c8d61f3e9483ec709c8116514cfb030c883ec262
facebook-github-bot pushed a commit to facebookexperimental/allocative that referenced this issue Oct 7, 2024
Summary:
Upgrading Rust from `1.80.1` to `1.81.0` ([release notes](https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html)) with the following changes affecting `fbsource`:
* Feature stabilizations:
  * `io_slice_advance` ([#62726](rust-lang/rust#62726))
  * `panic_info_message` ([#66745](rust-lang/rust#66745))
  * `fs_try_exists` ([#83186](rust-lang/rust#83186))
  * `lint_reasons` ([#120924](rust-lang/rust#120924))
  * `duration_abs_diff` ([#117618](rust-lang/rust#117618))
  * `effects` ([#102090](rust-lang/rust#102090))
* New `clippy` lints:
  * `manual_inspect` ([#12287](rust-lang/rust-clippy#12287))
  * `manual_pattern_char_comparison` ([#12849](rust-lang/rust-clippy#12849))
* Other:
  * `NEVER_TYPE_FALLBACK_FLOWING_INTO_UNSAFE` became a deny-by-default lint ([#126881](rust-lang/rust#126881) & [#123748](rust-lang/rust#123748))
  * Changes to `stringify!` introducing whitespaces between some tokens ([#125174](rust-lang/rust#125174))
  * `format!` is now marked with a `must_use` hint ([#127355](rust-lang/rust#127355))

ignore-conflict-markers

Reviewed By: zertosh, dtolnay

Differential Revision: D63864870

fbshipit-source-id: c8d61f3e9483ec709c8116514cfb030c883ec262
facebook-github-bot pushed a commit to facebook/hhvm that referenced this issue Oct 7, 2024
Summary:
Upgrading Rust from `1.80.1` to `1.81.0` ([release notes](https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html)) with the following changes affecting `fbsource`:
* Feature stabilizations:
  * `io_slice_advance` ([#62726](rust-lang/rust#62726))
  * `panic_info_message` ([#66745](rust-lang/rust#66745))
  * `fs_try_exists` ([#83186](rust-lang/rust#83186))
  * `lint_reasons` ([#120924](rust-lang/rust#120924))
  * `duration_abs_diff` ([#117618](rust-lang/rust#117618))
  * `effects` ([#102090](rust-lang/rust#102090))
* New `clippy` lints:
  * `manual_inspect` ([#12287](rust-lang/rust-clippy#12287))
  * `manual_pattern_char_comparison` ([#12849](rust-lang/rust-clippy#12849))
* Other:
  * `NEVER_TYPE_FALLBACK_FLOWING_INTO_UNSAFE` became a deny-by-default lint ([#126881](rust-lang/rust#126881) & [#123748](rust-lang/rust#123748))
  * Changes to `stringify!` introducing whitespaces between some tokens ([#125174](rust-lang/rust#125174))
  * `format!` is now marked with a `must_use` hint ([#127355](rust-lang/rust#127355))

ignore-conflict-markers

Reviewed By: zertosh, dtolnay

Differential Revision: D63864870

fbshipit-source-id: c8d61f3e9483ec709c8116514cfb030c883ec262
facebook-github-bot pushed a commit to facebookincubator/gazebo that referenced this issue Oct 7, 2024
Summary:
Upgrading Rust from `1.80.1` to `1.81.0` ([release notes](https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html)) with the following changes affecting `fbsource`:
* Feature stabilizations:
  * `io_slice_advance` ([#62726](rust-lang/rust#62726))
  * `panic_info_message` ([#66745](rust-lang/rust#66745))
  * `fs_try_exists` ([#83186](rust-lang/rust#83186))
  * `lint_reasons` ([#120924](rust-lang/rust#120924))
  * `duration_abs_diff` ([#117618](rust-lang/rust#117618))
  * `effects` ([#102090](rust-lang/rust#102090))
* New `clippy` lints:
  * `manual_inspect` ([#12287](rust-lang/rust-clippy#12287))
  * `manual_pattern_char_comparison` ([#12849](rust-lang/rust-clippy#12849))
* Other:
  * `NEVER_TYPE_FALLBACK_FLOWING_INTO_UNSAFE` became a deny-by-default lint ([#126881](rust-lang/rust#126881) & [#123748](rust-lang/rust#123748))
  * Changes to `stringify!` introducing whitespaces between some tokens ([#125174](rust-lang/rust#125174))
  * `format!` is now marked with a `must_use` hint ([#127355](rust-lang/rust#127355))

ignore-conflict-markers

Reviewed By: zertosh, dtolnay

Differential Revision: D63864870

fbshipit-source-id: c8d61f3e9483ec709c8116514cfb030c883ec262
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC 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. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.