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

Fix futures_util::io::copy & copy_buf to allow aborting operation in the middle #2497

Closed

Conversation

kazuki0824
Copy link
Contributor

@kazuki0824 kazuki0824 commented Sep 15, 2021

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. 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 bunch of modification that introduces a new behaviour, similar to what futures::abortable::Abortable does, into CopyBuf.
  • changes of return values coming from copy_buf(...) & copy(...), which can break backward compatibility a little.
  • changes of visibility of some Abortable-related private members, such as AbortInner and its children.

How to use

Unlike before, copy() & copy_buf() returns the combination of Future and AbortHandle. If you want to abort the operation, you should extract the tuple and trigger the inner AbortHandle.

Backward compatibility

If you want to fit the existing code to this change, all you have to do is simply adding ".0" after every copy_buf() or copy().

@kazuki0824 kazuki0824 changed the title Introduce an abortable mechanism into futures_util::io::copy & copy_buf Fix futures_util::io::copy & copy_buf to allow aborting operation in the middle Sep 20, 2021
@taiki-e
Copy link
Member

taiki-e commented Oct 8, 2021

Thanks for the PR!

  • copy is a very common function, and most of its use cases do not require this feature. If you want to add this feature, I would prefer to add it as a separate function.

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

@taiki-e taiki-e added the A-io Area: futures::io label Oct 8, 2021
@kazuki0824
Copy link
Contributor Author

Thanks for your reply! I understood your ideas.

Workaround

Implementing this as a separate module function sounds fine to me, so I re-implemented it as another module.

Suggestion

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

IMHO, I think it is a bit hard to realize copy() by simply adding AsyncRead/AsyncWrite to Abortable.

What is the problem?

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

  1. open File by futures::io::BufReader
  2. make it abortable by Abortable::new(file)

In addition, if we want to perform copying, then we have to
3. into_async_read()
4. 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 will be limited. Since forward() in futures::stream reads all, and then write all, that cannot be used.)

Conclusion

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 simply implementing Async io with existing stream::Abortable may cause inconvenience in the case such as copying, mentioned above.

@kazuki0824
Copy link
Contributor Author

kazuki0824 commented Oct 11, 2021

I fetched upstream into my branch accidentally and the history went dirty, so I force-pushed and the pr got closed. I'll create another one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: futures::io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants