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

Add `seek` and implement `Seeker` on `File` #1797

Merged
merged 5 commits into from Feb 18, 2019

Conversation

4 participants
@kevinkassimo
Copy link
Contributor

kevinkassimo commented Feb 16, 2019

Based on #1794 .
Closes #1304 .

#1794 fails due to that seek takes the ownership of the original tokio File.

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.
The hack is simplified. See comments in the code.

We might be able to drop this hack after tokio-rs/tokio#785 lands.

bartlomieju and others added some commits Feb 16, 2019

Patch Rust seek and implement Seeker for File
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.
js/files.ts Outdated
export async function seek(
rid: number,
offset: number,
whence: number

This comment has been minimized.

@lucascaro

lucascaro Feb 17, 2019

pardon the dumb question here... why is whence a number and not SeekMode?

This comment has been minimized.

@kevinkassimo

kevinkassimo Feb 17, 2019

Author Contributor

Fixed, it was some leftover from the old Seeker interface.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Feb 17, 2019

@kevinkassimo awesome you picked it up, it was laying around for some time, thanks!

@@ -153,3 +153,53 @@ testPerm({ read: true, write: true }, async function openModeWriteRead() {

This comment has been minimized.

@bartlomieju

bartlomieju Feb 17, 2019

Contributor

@kevinkassimo can you fix this test case and remove TODO?

This comment has been minimized.

@kevinkassimo

kevinkassimo Feb 17, 2019

Author Contributor

Done.

@kevinkassimo

This comment has been minimized.

Copy link
Contributor Author

kevinkassimo commented Feb 18, 2019

@bartlomieju I've simplified the hack a bit and avoid blocking other resource table operations.

@ry should we land this seek implementation now or instead just wait for tokio::io::seek() and AsyncSeek? (no idea when it would land in tokio...)

@ry

ry approved these changes Feb 18, 2019

Copy link
Collaborator

ry left a comment

LGTM - perfectly executed, nice tests.

@ry ry merged commit 077af20 into denoland:master Feb 18, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.