Asynchronous Seek and Seek implementation for File #785

agrif · 2018-11-30T17:47:10Z

(Original issue: #776)

Motivation

AsyncRead and AsyncWrite provide a way to do asynchronous I/O without caring about the specific underlying implementation. No such interface exists for seeking, though.

Solution

I implemented AsyncSeek, modeled after AsyncRead and AsyncWrite. Like those, it comes with a default implementation that relies on the underlying Seek implementation to use WouldBlock errors.

I also added tokio_io::io::seek, a future-flavored seek call built on top of AsyncSeek. The future it returns is almost an exact copy of SeekFuture from tokio_fs, but I kept both structs around since one has a type argument and one does not.

Finally, I implemented AsyncSeek on File, and changed the existing test to use the new io::seek. The methods seek and poll_seek on File (as well as SeekFuture) should probably be deprecated, but I'm not sure how that works.

tobz · 2018-12-01T13:48:32Z

That was a fast turnaround from issue to PR! 👍

I'll try and take a look at this today. WRT your File::seek/File::poll_seek comment, I think we can deprecate/hide them from the docs like this:

#[doc(hidden)]
#[deprecated(note="something here about tokio::io::seek and AsyncSeek")]

We definitely can't delete them yet, for obvious backwards-compatibility reasons, but we should definitely push people towards the better path if possible.

agrif · 2018-12-01T13:51:58Z

I was mostly confused about a version field in the deprecated attribute, since I see one included on every other deprecated interface but don't really have any idea how that is decided. Code is pretty straightforward, but project management stuff like versions is complicated :D

tobz · 2018-12-01T19:29:51Z

Yeah, since is optional.

I'd say let's add the above, and set since to 0.2 which is what we'll be at after a minor version bump. I'm pretty sure @carllerche will be cool with that, given that we're deprecating bits.

agrif · 2018-12-01T20:24:08Z

I have deprecated the File methods and SeekFuture, and sprinkled #[allow(deprecated) as required. If we gotta change the version, let me know (that's easy).

tobz · 2018-12-01T21:48:40Z

Actually, now that I've gone and looked at it.... I say we should make the version be 0.1.5.

0.1.5 will be when we release this, and 0.2 will be when we delete the deprecated methods.

Sorry about the back and forth!

tobz

Left a few minimal nits, but had some thoughts overall....

So, AsyncRead inherits Read, and AsyncWrite inherits Write, but this is something that will change in the future with a breaking release. It was a mistake to inherit them, and ideally, we want async-only types to only implement asynchronous public methods.

Given that impl Seek for File only works when run via an executor, it feels like we should drop that impl, and only implement AsyncSeek for File.

The tricky part is that we still want the behavior of ::would_block because the threadpool may actually run out of blocking threads to spawn on to, so we have to account for that.

tokio-io/src/async_seek.rs

agrif · 2018-12-02T04:26:13Z

I'm happy to give it a go, but I'm a little bit confused about what the future breaking release would look like. Right now it looks like you need to implement Read to be able to rely on a ton of infrastructure from std, like Take, Chain, BufReader, BufRead, etc. Would you be re-implementing those in tokio? Or perhaps changing std to allow re-using those without Read?

I don't think there is any similar infrastructure for Seek, but I'll at least poke around for a bit before making these changes.

tobz · 2018-12-02T15:15:13Z

I'm happy to give it a go, but I'm a little bit confused about what the future breaking release would look like.

Sorry, mostly that in the pre-1.0 world, we've been treating minor releases as our way to distinguish breaking updates. Since we're in 0.1 now, we would do this new method/deprecate old stuff as a patch version, then eventually drop the deprecated methods in a minor, breaking version release. The deprecation cleanup isn't anything you have to be concerned with, but just wanted to clarify.

Right now it looks like you need to implement Read to be able to rely on a ton of infrastructure from std, like Take, Chain, BufReader, BufRead, etc. Would you be re-implementing those in tokio? Or perhaps changing std to allow re-using those without Read?

In general, letting people ever call Read or Write methods on their tokio::net::tcp::TcpStream, or whatever, is clearly wrong. It's a footgun. With that in mind, we don't want to do the same thing again for AsyncSeek.

There's a good chunk of infrastructure that std has and Tokio only has partial answers for, but that is changing as time goes on.

agrif · 2018-12-08T19:40:00Z

Ok, I think I've made the required changes. A summary:

fa3250f Removed spurious newlines.
e1051bb Changed 0.2 to 0.1.5.
cfc61a0 I noticed I did not expose AsyncSeek alongside the other traits in the main tokio::io module, so I fixed that.
3468387 Stopped inheriting from Seek, and rewrote implementations to work. For File I used blocking_io(...) which I believe will provide the same behavior that would_block(...) did in the old code.

I removed the implementations for BufReader and BufWriter since I don't think those are useful without the Read/Write traits. However, I did leave in an implementation for Cursor, since that seems to be what the futures preview is doing for AsyncRead etc.

(An aside: should I try to form a similar PR for the futures crate?)

tobz

I think this is good to merge.

Thanks again, and sorry for the long lead time on getting it merged. 👍

ipetkov · 2019-01-11T07:00:39Z

@tobz restarted a failed travis job, CI looks green now, but I'll let you merge

MarkJr94 · 2019-01-31T00:57:18Z

@tobz is this still good to merge? might need it soon

kevinkassimo · 2019-02-16T20:36:34Z

Is this PR ready? Similarly in need of this feature.

This patch contains a special hack that circumvents the current tokio seek problem. tokio `seek` is implemented to take ownership of the original File and emit a new one in its future, which conflicts with the design of ResourceTable. To avoid the problem, the current hack makes the FsFile resource an Option which we could `take` the value ownership out of it. We then convert the tokio File into a Rust std File, perform the seek, and then put it back into the resource. This might be able to drop this hack after tokio-rs/tokio#785 lands.

carllerche

Question inline

tokio-fs/src/file/mod.rs

carllerche

Posting the Request changes review until the question re: poll_seek name conflict is resolved.

This patch contains a special hack that circumvents the current tokio seek problem. tokio `seek` is implemented to take ownership of the original File and emit a new one in its future, which conflicts with the design of ResourceTable. To avoid the problem, the current hack makes the FsFile resource an Option which we could `take` the value ownership out of it. We then convert the tokio File into a Rust std File, perform the seek, and then put it back into the resource. This might be able to drop this hack after tokio-rs/tokio#785 lands.

agrif · 2019-02-24T03:49:20Z

@carllerche I've added a test for File::poll_seek. It looks like it is finding the File implementation even when AsyncSeek is in scope, though I don't see any way to verify this except to enable deprecation warnings on the test and make sure rust emits one. I don't know if you can make a test fail if a deprecation warning is not emitted...

Any suggestions, or is this good?

carllerche · 2019-02-24T04:23:26Z

@agrif Ok... I think we can make this work out.

The deprecated annotation should be removed. Unfortunately, because the compiler defaults to the inherent method (on File) there is no easy way for the user to use the trait fn. That is OK though since the trait fn and the inherent fn are the same. The inherent fn can still be marked as #[doc(hidden)].

Besides that, there is a bunch of other smaller tweaks I would do, but I can try to tackle it myself on monday or tuesday if you don't want to (it's a bit hard to explain).

A lot of the tokio-io stuff is in the pattern Tokio used when it was first written. Idioms have changed. You can see tokio-buf for the new idioms.

Basically:

tokio-io should re-export SeekFrom.
tokio-io should have a util module, in that module there is an AsyncSeekExt trait.
The AsyncSeekExt trait has the seek fn (it is not free).

agrif · 2019-02-25T06:04:25Z

Ok, I've removed the deprecation annotation on File::poll_seek. As for the other changes, I think I have a rough idea of what you want but am fuzzy on the details. If you are offering to make those, that's probably for the best.

EliSnow · 2019-04-09T20:52:22Z

Just wanted to ping this to check the status. @carllerche, based on the last comment it looks like @agrif is expecting input from you.

EliSnow · 2019-04-09T21:07:22Z

@agrif, Can this PR also implement std::io::Seek for tokio::fs::File? I looked over tokio-fs/src/file/mod.rs but I didn't notice it there. Sorry if I missed it.

My use case is I want to be able to use the seek method on a csv Reader build on top of a tokio::fs::File which requires std::io::Seek.

agrif · 2019-04-09T23:23:04Z

@EliSnow the original version of this pull request had AsyncSeek inherit Seek, and so tokio::fs::File also implemented Seek. This mirrored how, for example, AsyncRead and Read work. However, it seems like the devs have since cooled on this pattern, see tobz's comment above.

It would be easy enough to add a Seek implementation, but at the moment at least it sounds like that's not something the devs want.

carllerche · 2019-04-22T22:16:25Z

Adding AsyncSeek is blocked on #1045. I believe there may be complications w.r.t implementing the proposed trait using the fallback blocking pool. We need to first ensure it can work before introducing the trait.

ghost · 2019-05-11T20:16:22Z

So the concern @carllerche had with poll_seek() in #1045 was the following.

Suppose someone polls a seek operation, maybe several times in succession, and it always returns NotReady. If the seek operation is not driven to completion, is it okay if it is eventually silently completed and the file cursor gets moved?

We considered three different behaviors/guarantees of asynchronous seek:

The first time a seek operation is polled, NotReady is returned, but it begins executing in the background. Successive polls will only return the status of the already started asynchronous seek operation. That means the seek operation will eventually be completed even if not driven to completion by successive polling.
We don't specify whether the asynchronous seek operation will complete or not. If you poll it but don't drive to completion, it may or may not happen in the end.
If NotReady is returned, the seek operation is not completed. Only when Ready is returned the operation is "atomically" completed. So the file cursor is only moved when Ready is returned. In case one polls a seek operation but doesn't drive it to completion, the file will be left intact.

In the end, we chose the 3rd option because that behavior feels most natural and consistent with the way poll_read() and poll_write() move the cursor.

This PR looks good to me and doesn't seem to affect our choice of guarantees around seeking.

carllerche · 2019-05-15T19:38:39Z

Unfortunately it looks like there are merge conflicts, but the PR can be merged once those are resolved. Thanks.

These have been replaced by io::AsyncSeek and io::Seek<File>.

agrif · 2019-05-16T20:56:13Z

I've rebased on to the latest master. Along the way, I updated the deprecation annotations to point to tokio-fs 0.1.7, since that will be the next minor point release for that package. If that's not what you folks need, let me know and I can update it.

agrif · 2019-07-23T21:31:57Z

Just poking you folks for an update. It's been 235 days since this PR was opened, and I no longer even remember what I was trying to do when I ran into this limitation. Is there something I can do to help this along?

This patch contains a special hack that circumvents the current tokio seek problem. tokio `seek` is implemented to take ownership of the original File and emit a new one in its future, which conflicts with the design of ResourceTable. To avoid the problem, the current hack makes the FsFile resource an Option which we could `take` the value ownership out of it. We then convert the tokio File into a Rust std File, perform the seek, and then put it back into the resource. This might be able to drop this hack after tokio-rs/tokio#785 lands.

carllerche · 2019-10-21T17:07:19Z

Follow up here: #1641

agrif mentioned this pull request Nov 30, 2018

tokio-io, tokio-fs: No impl for Seek / No trait AsyncSeek #776

Closed

tobz reviewed Dec 1, 2018

View reviewed changes

tokio-io/src/async_seek.rs Outdated Show resolved Hide resolved

tokio-io/src/async_seek.rs Outdated Show resolved Hide resolved

tobz approved these changes Jan 5, 2019

View reviewed changes

carllerche reviewed Feb 16, 2019

View reviewed changes

tokio-fs/src/file/mod.rs Show resolved Hide resolved

kevinkassimo mentioned this pull request Feb 16, 2019

Add seek and implement Seeker on File denoland/deno#1797

Merged

carllerche requested changes Feb 18, 2019

View reviewed changes

LucioFranco mentioned this pull request Apr 4, 2019

AsyncSeek trait rust-lang/futures-rs#1512

Closed

ghost approved these changes May 11, 2019

View reviewed changes

agrif added 9 commits May 16, 2019 16:48

io: add AsyncSeek trait and Seek future

db0ae96

fs: implement tokio_io::AsyncSeek for File

2dfcdd2

fs: deprecate File::poll_seek, File::seek, and SeekFuture

adf9d39

These have been replaced by io::AsyncSeek and io::Seek<File>.

io: removed extra newlines

617e827

fs: deprecate seek methods in 0.1.7, not 0.2

c26e1f9

tokio: export AsyncSeek and seek future from io

bf4f32c

io, fs: changed AsyncSeek to not inherit Seek

37029ac

fs: added test for File::poll_seek while AsyncSeek is in scope

a06d343

tokio-fs: remove deprecation on File::poll_seek

7f2adf3

agrif force-pushed the async-seek branch from c9b82b6 to 7f2adf3 Compare May 16, 2019 20:54

carllerche mentioned this pull request Oct 12, 2019

io: add AsyncSeek trait #1641

Closed

carllerche closed this Oct 21, 2019

nrc mentioned this pull request Mar 23, 2022

AsyncSeek nrc/portable-interoperable#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asynchronous Seek and Seek implementation for File #785

Asynchronous Seek and Seek implementation for File #785

agrif commented Nov 30, 2018 •

edited

Loading

tobz commented Dec 1, 2018

agrif commented Dec 1, 2018

tobz commented Dec 1, 2018 •

edited

Loading

agrif commented Dec 1, 2018

tobz commented Dec 1, 2018

tobz left a comment

agrif commented Dec 2, 2018

tobz commented Dec 2, 2018

agrif commented Dec 8, 2018

tobz left a comment

ipetkov commented Jan 11, 2019

MarkJr94 commented Jan 31, 2019

kevinkassimo commented Feb 16, 2019

carllerche left a comment

carllerche left a comment

agrif commented Feb 24, 2019

carllerche commented Feb 24, 2019

agrif commented Feb 25, 2019

EliSnow commented Apr 9, 2019

EliSnow commented Apr 9, 2019

agrif commented Apr 9, 2019

carllerche commented Apr 22, 2019

ghost commented May 11, 2019

carllerche commented May 15, 2019

agrif commented May 16, 2019

agrif commented Jul 23, 2019

carllerche commented Oct 21, 2019

Asynchronous Seek and Seek implementation for File #785

Asynchronous Seek and Seek implementation for File #785

Conversation

agrif commented Nov 30, 2018 • edited Loading

Motivation

Solution

tobz commented Dec 1, 2018

agrif commented Dec 1, 2018

tobz commented Dec 1, 2018 • edited Loading

agrif commented Dec 1, 2018

tobz commented Dec 1, 2018

tobz left a comment

Choose a reason for hiding this comment

agrif commented Dec 2, 2018

tobz commented Dec 2, 2018

agrif commented Dec 8, 2018

tobz left a comment

Choose a reason for hiding this comment

ipetkov commented Jan 11, 2019

MarkJr94 commented Jan 31, 2019

kevinkassimo commented Feb 16, 2019

carllerche left a comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

agrif commented Feb 24, 2019

carllerche commented Feb 24, 2019

agrif commented Feb 25, 2019

EliSnow commented Apr 9, 2019

EliSnow commented Apr 9, 2019

agrif commented Apr 9, 2019

carllerche commented Apr 22, 2019

ghost commented May 11, 2019

carllerche commented May 15, 2019

agrif commented May 16, 2019

agrif commented Jul 23, 2019

carllerche commented Oct 21, 2019

agrif commented Nov 30, 2018 •

edited

Loading

tobz commented Dec 1, 2018 •

edited

Loading