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

Create copy_buf_abortable, which enables to stop copying in the middle. #2507

Merged
merged 9 commits into from Apr 19, 2022

Conversation

kazuki0824
Copy link
Contributor

@kazuki0824 kazuki0824 commented Oct 11, 2021

See #2497 .

Summary

When transferring data from one place to another using the copy_buf function, you may want to abort the operation in the middle of the transfer, for example, by SIGINT. Since the transfer is divided into small increments by internal buffers and probably must not be done at a time, the interruption logic can be implemented by checking for the aborting request flag at every poll_fill_buf().

This patch has

  • a new func and struct that is like CopyBuf and has a new behaviour, similar to what futures::abortable::Abortable does.
  • changes of visibility of some Abortable-related private members, such as AbortInner and its children.

@kazuki0824
Copy link
Contributor Author

kazuki0824 commented Oct 11, 2021

Motivation

If we read file with futures_io and make it abortable, we have to

  1. open std::fs::File struct by futures::io::BufReader,
  2. into_stream
  3. make it abortable by Abortable::new(file).
    In addition, if we want to perform copying, then we have to
  4. into_async_read()
  5. futures::io::copy().
    (Here, we have to use AsyncRead instead of stream::Abortable. If you deal with character devices or sockets, there is no guarantee that the size of the file is limited to a constant value. Since forward() in futures::stream reads all, and then write all, the approach cannot be applied.)
    It sounds very redundant to me. So it should be better to add a new abortable variant of copy() and copy_buf().

Recap of recent discussion in #2497

@taiki-e pointed out that

* Is it possible to do this same thing by implementing AsyncRead/AsyncWrite for Abortable? If so, that would be more generic and preferable.

I also think that for more convenience, something like AsyncRead / AsyncWrite 'd abortable is necessary. I agree to you at this point. (also mentioned in #2156)
But IMHO simply implementing Async io on an existing stream::Abortable may cause inconvenience in the case such as copying.

Pros and cons

I think there exist 2 different options,

  1. Add a new struct AbortableCopyBuf<T: AsyncRead> , method copy_buf_abortable(inner) (My current PR, as per @taiki-e 's suggestion)
  2. Implement AsyncRead, AsyncWrite on stream::Abortable<T: AsRef<[u8]>>(@taiki-e suggested)

For first one, we perform copy with futures_io and make it abortable, we have to

  1. open std::fs::File struct by futures::io::BufReader,
  2. futures::io::copy_buf_abortable().

But the second one,

  1. open std::fs::File struct by futures::io::BufReader,
  2. into_stream
  3. make it abortable by Abortable::new(file).
  4. directly pass the abortable stream to futures::io::copy()

So I think the second one has a thicker struct that perhaps lowers performance, and the first one is less complicated for programmers when it comes to copying one to another.
Consequently, I think it is a bit hard to realize copy() by simply adding AsyncRead/AsyncWrite to Abortable.

Thus, I believe that implement abortable copy into a separate mod, like this current PR, is better off.

@kazuki0824
Copy link
Contributor Author

kazuki0824 commented Oct 14, 2021

@taiki-e After that, I found an opinion that we should implement the ability to stop in the middle for all IOs, not just the copy_buf(). This topic is very relevant to this pull request, but the original Issue is already outdated. Therefore, I created a new issue in #2509, and then suggested another solution. Please check it out.
To me, the method I suggested in #2509 is more generic(and based on your suggestion!), but I feel it will be very very complex, costful to implement the future work if we aim to implement abortable feature on any readers/writers, not only a copying op.

@kazuki0824 kazuki0824 changed the title Create copy_buf_abortable, which allows stopping copying in the middle. Create copy_buf_abortable, which enables to stop copying in the middle. Oct 17, 2021
futures-util/src/io/copy_buf_abortable.rs Outdated Show resolved Hide resolved
futures-util/src/io/copy_buf_abortable.rs Outdated Show resolved Hide resolved
@taiki-e taiki-e added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Jan 4, 2022
@kazuki0824
Copy link
Contributor Author

@taiki-e Sorry for the late response. I saw through your review and added another change in this PR.

@taiki-e taiki-e removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Apr 16, 2022
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me aside from a nit.

futures-util/src/io/copy_buf_abortable.rs Show resolved Hide resolved
@taiki-e taiki-e added A-io Area: futures::io 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Apr 16, 2022
@taiki-e taiki-e merged commit c1d71c8 into rust-lang:master Apr 19, 2022
@taiki-e taiki-e mentioned this pull request Aug 14, 2022
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Aug 14, 2022
@taiki-e taiki-e mentioned this pull request Aug 14, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Aug 23, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [futures](https://github.com/rust-lang/futures-rs) | dependencies | patch | `0.3.21` -> `0.3.23` |

---

### Release Notes

<details>
<summary>rust-lang/futures-rs</summary>

### [`v0.3.23`](https://github.com/rust-lang/futures-rs/blob/HEAD/CHANGELOG.md#&#8203;0323---2022-08-14)

[Compare Source](rust-lang/futures-rs@0.3.22...0.3.23)

-   Work around MSRV increase due to a cargo bug.

### [`v0.3.22`](https://github.com/rust-lang/futures-rs/blob/HEAD/CHANGELOG.md#&#8203;0322---2022-08-14)

[Compare Source](rust-lang/futures-rs@0.3.21...0.3.22)

-   Fix `Sync` impl of `BiLockGuard` ([#&#8203;2570](rust-lang/futures-rs#2570))
-   Fix partial iteration in `FuturesUnordered` ([#&#8203;2574](rust-lang/futures-rs#2574))
-   Fix false detection of inner panics in `Shared` ([#&#8203;2576](rust-lang/futures-rs#2576))
-   Add `Mutex::lock_owned` and `Mutex::try_lock_owned` ([#&#8203;2571](rust-lang/futures-rs#2571))
-   Add `io::copy_buf_abortable` ([#&#8203;2507](rust-lang/futures-rs#2507))
-   Remove `Unpin` bound from `TryStreamExt::into_async_read` ([#&#8203;2599](rust-lang/futures-rs#2599))
-   Make `run_until_stalled` handle self-waking futures ([#&#8203;2593](rust-lang/futures-rs#2593))
-   Use `FuturesOrdered` in `try_join_all` ([#&#8203;2556](rust-lang/futures-rs#2556))
-   Fix orderings in `LocalPool` waker ([#&#8203;2608](rust-lang/futures-rs#2608))
-   Fix `stream::Chunk` adapters size hints ([#&#8203;2611](rust-lang/futures-rs#2611))
-   Add `push_front` and `push_back` to `FuturesOrdered` ([#&#8203;2591](rust-lang/futures-rs#2591))
-   Deprecate `FuturesOrdered::push` in favor of `FuturesOrdered::push_back` ([#&#8203;2591](rust-lang/futures-rs#2591))
-   Performance improvements ([#&#8203;2583](rust-lang/futures-rs#2583), [#&#8203;2626](rust-lang/futures-rs#2626))
-   Documentation improvements ([#&#8203;2579](rust-lang/futures-rs#2579), [#&#8203;2604](rust-lang/futures-rs#2604), [#&#8203;2613](rust-lang/futures-rs#2613))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xNTYuMSIsInVwZGF0ZWRJblZlciI6IjMyLjE1Ni4xIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1507
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants