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.

Sign up for GitHub

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

Jump to bottom

fs: implement unix OpenOptionsExt for OpenOptions #2515

Merged
merged 11 commits into from
May 21, 2020
Merged

fs: implement unix OpenOptionsExt for OpenOptions #2515

merged 11 commits into from
May 21, 2020

Conversation

chovine
Copy link
Contributor

@chovine chovine commented May 11, 2020 •
edited
Loading

Motivation

Implement std::os::unix::fs::OpenOptionsExt for tokio::fs::OpenOptions in order to avoid to have to create tokio::fs::OpenOptions from std::fs::OpenOptions when using OpenOptionsExt.

Solution

  • Added an as_inner_mut method to tokio::fs::OpenOptions, visible only to the tokio::fs module.
  • Added the trivial implementation of OpenOptionsExt to open_options_ext.rs, using the underlying std::fs::OpenOptions of tokio::fs::OpenOptions.
  • Mirrored the definition of std::os::unix::fs::OpenOptionsExt in tokio::fs::os::unix

fs: implement unix OpenOptionsExt for OpenOptions
b07b2b9 
Trait std::os::unix::fs::OpenOptionsExt is now implemented for
fs::OpenOption.

In order to access the underlying std::fs::OpenOptions wrapped in
tokio's OpenOption, an `as_inner_mut` method was added to OpenOption,
only visible to the parent module.

Fixes: tokio-rs#2366
@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-fs Module: tokio/fs labels May 11, 2020
Darksonn
Darksonn reviewed May 11, 2020
View reviewed changes
tokio/src/fs/os/unix/mod.rs Show resolved Hide resolved
tokio/src/fs/open_options.rs Outdated Show resolved Hide resolved
@sfackler
Copy link
Contributor

I would recommend against directly implementing std's trait. Tokio's build will break whenever new methods are added to it.

@Darksonn
Copy link
Contributor

@sfackler But that would be a breaking change in std, no?

@sfackler
Copy link
Contributor

We generally assume that those traits have not been implemented externally. They should probably be sealed, but that was invented after the traits were added. custom_flags was added in 1.10, for example.

@Darksonn
Copy link
Contributor

I see. I guess we should add our own duplicate of the trait then.

@chovine
Copy link
Contributor Author

chovine commented May 11, 2020

I see. I guess we should add our own duplicate of the trait then.

I can redefine OpenOptionExt along with the trait implementation. Should this be a separate issue?

@Darksonn
Copy link
Contributor

You can just do it here.

@chovine
add OpenOptionsExt trait definition
07683b6 
Mirror the definition of `std::os::unix::fs::OpenOptionsExt`
@chovine chovine marked this pull request as ready for review May 11, 2020 13:15
Darksonn
Darksonn reviewed May 11, 2020
View reviewed changes
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Just some documentation nits, and I think this is ready to merge.

tokio/src/fs/os/unix/open_options_ext.rs Outdated Show resolved Hide resolved
tokio/src/fs/open_options.rs Outdated Show resolved Hide resolved
@chovine
Copy link
Contributor Author

chovine commented May 11, 2020

Both remarks included, thanks for your review.

Darksonn
Darksonn reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Ugh, I'm sorry I didn't catch this on the previous review.

tokio/src/fs/os/unix/open_options_ext.rs Outdated Show resolved Hide resolved
@shkurski shkurski mentioned this pull request May 12, 2020
Merged
@Darksonn
Copy link
Contributor

CI is messed up. Can you please push an empty commit to fix it?

@Darksonn
Copy link
Contributor

This is a bit ridiculous. You don't need to push more empty commits, I'll do something about it if it doesn't work this time around.

@chovine
Copy link
Contributor Author

chovine commented May 21, 2020

This is a bit ridiculous. You don't need to push more empty commits, I'll do something about it if it doesn't work this time around.

I assumed the timeouts were due to the CI runner being under heavy load. Feel free to fix if it does not work this time

@Darksonn
Copy link
Contributor

No, there's some issue with the windows tests in #2499, but we aren't many windows users, and the few of us that do use windows have not been able to reproduce it on their own machines.

@Darksonn Darksonn merged commit 4f4f480 into tokio-rs:master May 21, 2020
jensim pushed a commit to jensim/tokio that referenced this pull request Jun 7, 2020
@chovine @jensim
fs: implement OpenOptionsExt for OpenOptions (tokio-rs#2515)
1b1789c 
Trait OpenOptionsExt is now implemented for fs::OpenOption.

In order to access the underlying std::fs::OpenOptions wrapped in
tokio's OpenOption, an as_inner_mut method was added to OpenOption,
only visible to the parent module.

Fixes: tokio-rs#2366
@Darksonn Darksonn mentioned this pull request Jun 30, 2020
Open
hawkw added a commit that referenced this pull request Jul 20, 2020
@hawkw
chore: prepare to release 0.2.22
972c717 
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2572, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)

- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- io: fix panic in `AsyncReadExt::read_line` (#2541)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw mentioned this pull request Jul 20, 2020
Merged
hawkw added a commit that referenced this pull request Jul 22, 2020
@hawkw
chore: prepare to release 0.2.22 (#2672)
21f7260 
# 0.2.22 (July 2!, 2020)

### Fixes
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)
- io: fix panic in `AsyncReadExt::read_line` (#2541)

### Changes
- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

### Added
- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@Darksonn Darksonn mentioned this pull request Sep 23, 2020
Open
2 tasks
@taiki-e taiki-e mentioned this pull request Oct 4, 2020
Merged
@Darksonn Darksonn mentioned this pull request Oct 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Darksonn Darksonn Darksonn approved these changes

Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-fs Module: tokio/fs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chovine @sfackler @Darksonn