Change AioCb to primarily use Bytes instead of Rc<[u8]> #820

asomers · 2017-12-21T06:02:26Z

Rc<[u8]> isn't a very good buffer type to use for aio. For one thing, it lacks interior mutability. For another, a single Rc<[u8]> can't be carved up into smaller buffers of the same type. Bytes and BytesMut fix both problems. This PR removes the ability to construct an AioCb from Rc<[u8]> and adds the ability to construct one from Bytes, BytesMut, or raw pointers (for consumers who need even more flexibility). At this stage, the PR has the following warts:

A hack is necessary to force small Bytes buffers to allocate on the heap. I plan to fix this with an enhancement to the bytes crate.
The AioCb::buffer method is necessary due to a deficiency in the tokio-core crate. Once I fix that, then only AioCb::into_bufferwill need to be public.

Susurrus · 2017-12-21T16:11:06Z

src/sys/aio.rs

+    ///
+    /// It is an error to call this method while the `AioCb` is still in
+    /// progress.
+    pub fn buffer(&mut self) -> Buffer<'a> {


Does it make sense to make this return a Result type instead of having failing if the in progress invariant is broken?

I think not. If we do that, then AioCb::into_buffer will need to be able to handle it, such as by returning None. However, that method can already return Buffer::none, which means something different. It would be confusing to return two different None types.

In my experience, your program's flow should make it obvious whether any given AioCb is still in progress or not. Treating an in-progress AioCb as though it has already finished is almost always going to be a programming bug. So I think asserting is appropriate.

Susurrus · 2018-01-04T00:31:54Z

Any movement on this?

asomers · 2018-01-04T00:33:12Z

Yeah, I'm 99% done modifying the downstream crates. But I can't be confident that my new API will work until I'm 100% done.

Susurrus · 2018-01-07T00:14:36Z

Got an ETA on this? We're at 6 months since our last release, and I think it's smart that we get this out the door instead of waiting. How would you feel about releasing 0.10 now?

asomers · 2018-01-11T04:18:57Z

I'm all satisfied with this change now. Review at your leisure.

Susurrus

I don't understand the AIO API very well, so this is mostly minor nits I'm pointing out.

One thing that would have helped me in reviewing this would be to see more doccomment examples for this subsystem. Since this is a refactoring, it can be helpful for users to see specific examples in the documentation (even if some are already included in tests). Could you think of some small examples that would make sense to add to the doccomments for any of the code you've touched? There's no module-level docs for this API, otherwise I'd suggest to throw them in there, but maybe this is a good time to add some?

Susurrus · 2018-01-13T18:48:41Z

src/sys/aio.rs

-    boxed(Rc<Box<[u8]>>),
+    /// Immutable shared ownership `Bytes` object
+    // Must use out-of-line allocation so the address of the data will be
+    // stable.  Bytes and BytesMut sometimes dynamically allocate a buffer, and


Backticks around Bytes and BytesMut

Susurrus · 2018-01-13T18:49:59Z

src/sys/aio.rs

 #[derive(Clone, Debug)]
-enum Keeper<'a> {
+pub enum Buffer<'a> {
    none,


Can you add a doccomment for this variant?

Additionally, why is this lowercase none? I'd expect it to be None.

Not for any good reason. I'll change it back. However, I'm wondering if I should make the Buffer type private. If I do that, then I'll have to replace AioCb::into_buffer with AioCb::into_bytes -> Option<Bytes> and AioCb::into_bytes_mut -> Option<BytesMut>. Do you think that would be a better API?

Nevermind. I forgot that I can't do that due to a limitation of tokio-core. In order to satisfy tokio-core, I need way to get the Buffer without consuming the AioCb.

Susurrus · 2018-01-13T18:56:41Z

CHANGELOG.md

@@ -107,6 +105,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  ([#648](https://github.com/nix-rust/nix/pull/648))

 ### Removed
+- `AioCb::from_boxed_slice` has been removed.  It was never actually safe.


What would be the replacement for this kind of functionality? If there's a short answer, it should be said here, otherwise it should be stated to "read the docs for more details" or "see the PR for more details". We should make it as easy as possible for downstream users of our library to update their code, this means pointing them in the right direction when we break it.

Susurrus · 2018-01-13T18:57:10Z

Cargo.toml

@@ -18,6 +18,11 @@ bitflags = "1.0"
 cfg-if = "0.1.0"
 void = "1.0.2"

+# Don't include the optional serde feature


Can you move this comment to right about the default-features line so it's more clear that's what it's pertaining to?

Susurrus · 2018-01-13T18:57:39Z

CHANGELOG.md

@@ -7,7 +7,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).

 ### Added
 - Added `AioCb::from_ptr` and `AioCb::from_mut_ptr`
-  ([#TODO](https://github.com/nix-rust/nix/pull/TODO))
+  ([#820](https://github.com/nix-rust/nix/pull/820))


Please fix the initial commit to have this number from the start.

Susurrus · 2018-01-13T18:58:06Z

CHANGELOG.md

@@ -58,8 +58,6 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ### Changed
 - Use native `pipe2` on all BSD targets.  Users should notice no difference.
  ([#777](https://github.com/nix-rust/nix/pull/777))
- `AioCb::from_boxed_slice` no longer allows `aio_read`


Is there a reason why we have this step fixing up from_boxed_slice only to remove it in the following commit?

Not really. I should squash them together.

asomers · 2018-01-14T03:57:12Z

@Susurrus I think I responded to all of your concerns except the one about squashing. I left everything unsquashed for your viewing pleasure.

Susurrus

Wow, you've done a great job with the docs, great job! I'm hoping we can get to this level of documentation for all of nix this year (#nix2018).

Besides the minor nits I brought up on specific lines, I did have two comments that apply to a few places in the code:

For the Examples, could you provide a brief summary of what's happening? Just a sentence is fine, but it can be helpful for new users to have context before they start diving into the code as the user may not even be wanting to read that example.
For the Safety sections, generally this is supposed to explain what invariants must by held by the caller/programmer that makes the function unsafe. This section is supposed to explain how to use the function safely, not just warn the user that the function is unsafe (the function definition and compiler errors about them needing to specify the call as unsafe should take care of that).

With those changes made, I'm r+ on this. Love that it improves our safety and docs so much!

Susurrus · 2018-01-14T17:03:23Z

src/sys/aio.rs

+//! [kevent](../signal/enum.SigevNotify.html#variant.SigevKevent).
+//!
+//! Multiple operations may be submitted in a batch with
+//! [`lio_listio`](fn.lio_listio.html), thought the standard does not guarantee


thought -> though

Susurrus · 2018-01-14T17:05:34Z

CHANGELOG.md

@@ -6,6 +6,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ## [Unreleased]

 ### Added
+- Added `AioCb::from_ptr` and `AioCb::from_mut_ptr`
+  ([#820](https://github.com/nix-rust/nix/pull/TODO))


This first link should be removed.

Susurrus · 2018-01-14T17:07:58Z

src/sys/aio.rs

+    /// result and handle operations that were not canceled or that have already
+    /// completed.
+    ///
+    /// See also


I would rephrase to indicate this is an external link like "See the man pages for aio_cancel for more details." This applies for all the uses of "See also LINK".

Susurrus · 2018-01-15T00:46:33Z

Absolutely beautiful! I think this is the new standard for our docs in nix.

Go ahead and squash with a good commit message and then LGTM, assuming the tests pass (i386 FreeBSD is failing still).

It's not actually safe to read into an `Rc<[u8]>`. It only worked because of a coincidental `unsafe` block. Replace that type with `BytesMut` from the bytes crate. For consistency's sake, use `Bytes` for writing too, and completely remove methods relating to `Rc<[u8]>`. Note that the `AioCb` will actually own the `BytesMut` object. The caller must call `into_buffer` to get it back once the I/O is complete. Fixes nix-rust#788

asomers · 2018-01-17T05:23:59Z

Travis is back up now. I'm going to go ahead and merge this even though the last OSX builds never ran, because the last changes I made were just formatting stuff that shouldn't have broken OSX.

bors r+ susurrus

820: Change AioCb to primarily use Bytes instead of Rc<[u8]> r=asomers a=asomers `Rc<[u8]>` isn't a very good buffer type to use for aio. For one thing, it lacks interior mutability. For another, a single `Rc<[u8]>` can't be carved up into smaller buffers of the same type. `Bytes` and `BytesMut` fix both problems. This PR removes the ability to construct an `AioCb` from `Rc<[u8]>` and adds the ability to construct one from `Bytes`, `BytesMut`, or raw pointers (for consumers who need even more flexibility). At this stage, the PR has the following warts: 1. A hack is necessary to force small `Bytes` buffers to allocate on the heap. I plan to fix this with an enhancement to the bytes crate. 2. The `AioCb::buffer` method is necessary due to a deficiency in the tokio-core crate. Once I fix that, then only `AioCb::into_buffer`will need to be public.

Susurrus · 2018-01-17T05:25:42Z

🤞

bors · 2018-01-17T09:54:01Z

Timed out

Susurrus · 2018-01-17T13:58:55Z

bors r+

820: Change AioCb to primarily use Bytes instead of Rc<[u8]> r=Susurrus a=asomers `Rc<[u8]>` isn't a very good buffer type to use for aio. For one thing, it lacks interior mutability. For another, a single `Rc<[u8]>` can't be carved up into smaller buffers of the same type. `Bytes` and `BytesMut` fix both problems. This PR removes the ability to construct an `AioCb` from `Rc<[u8]>` and adds the ability to construct one from `Bytes`, `BytesMut`, or raw pointers (for consumers who need even more flexibility). At this stage, the PR has the following warts: 1. A hack is necessary to force small `Bytes` buffers to allocate on the heap. I plan to fix this with an enhancement to the bytes crate. 2. The `AioCb::buffer` method is necessary due to a deficiency in the tokio-core crate. Once I fix that, then only `AioCb::into_buffer`will need to be public.

bors · 2018-01-17T18:28:56Z

Timed out

asomers · 2018-01-18T00:25:50Z

Try try again

bors r+ susurrus

820: Change AioCb to primarily use Bytes instead of Rc<[u8]> r=asomers a=asomers `Rc<[u8]>` isn't a very good buffer type to use for aio. For one thing, it lacks interior mutability. For another, a single `Rc<[u8]>` can't be carved up into smaller buffers of the same type. `Bytes` and `BytesMut` fix both problems. This PR removes the ability to construct an `AioCb` from `Rc<[u8]>` and adds the ability to construct one from `Bytes`, `BytesMut`, or raw pointers (for consumers who need even more flexibility). At this stage, the PR has the following warts: 1. A hack is necessary to force small `Bytes` buffers to allocate on the heap. I plan to fix this with an enhancement to the bytes crate. 2. The `AioCb::buffer` method is necessary due to a deficiency in the tokio-core crate. Once I fix that, then only `AioCb::into_buffer`will need to be public.

bors · 2018-01-18T04:54:12Z

Timed out

asomers · 2018-01-18T04:56:29Z

If at third you don't succeed, try try again.

bors r+ susurrus

820: Change AioCb to primarily use Bytes instead of Rc<[u8]> r=asomers a=asomers `Rc<[u8]>` isn't a very good buffer type to use for aio. For one thing, it lacks interior mutability. For another, a single `Rc<[u8]>` can't be carved up into smaller buffers of the same type. `Bytes` and `BytesMut` fix both problems. This PR removes the ability to construct an `AioCb` from `Rc<[u8]>` and adds the ability to construct one from `Bytes`, `BytesMut`, or raw pointers (for consumers who need even more flexibility). At this stage, the PR has the following warts: 1. A hack is necessary to force small `Bytes` buffers to allocate on the heap. I plan to fix this with an enhancement to the bytes crate. 2. The `AioCb::buffer` method is necessary due to a deficiency in the tokio-core crate. Once I fix that, then only `AioCb::into_buffer`will need to be public.

bors · 2018-01-18T09:24:12Z

Timed out

Susurrus · 2018-01-18T14:35:23Z

Travis is still having a tough time with Mac builds. Let's wait a couple of days until the backlog dies down a bit and we can try again.

asomers · 2018-01-19T01:54:37Z

I see that libc was able to run OSX builds on Travis. Maybe we can too.

bors r+ susurrus

820: Change AioCb to primarily use Bytes instead of Rc<[u8]> r=asomers a=asomers `Rc<[u8]>` isn't a very good buffer type to use for aio. For one thing, it lacks interior mutability. For another, a single `Rc<[u8]>` can't be carved up into smaller buffers of the same type. `Bytes` and `BytesMut` fix both problems. This PR removes the ability to construct an `AioCb` from `Rc<[u8]>` and adds the ability to construct one from `Bytes`, `BytesMut`, or raw pointers (for consumers who need even more flexibility). At this stage, the PR has the following warts: 1. A hack is necessary to force small `Bytes` buffers to allocate on the heap. I plan to fix this with an enhancement to the bytes crate. 2. The `AioCb::buffer` method is necessary due to a deficiency in the tokio-core crate. Once I fix that, then only `AioCb::into_buffer`will need to be public.

Susurrus · 2018-01-19T02:32:09Z

libc has a paid Travis subscription, so they can almost always run jobs. Note that the Open Source Mac build queue is almost 3000; this isn't going to finish.

bors · 2018-01-19T06:24:38Z

Timed out

Susurrus · 2018-01-21T16:46:09Z

Mac OS build queue down to 1100, so maybe this could actually happen now!

bors r+

820: Change AioCb to primarily use Bytes instead of Rc<[u8]> r=Susurrus a=asomers `Rc<[u8]>` isn't a very good buffer type to use for aio. For one thing, it lacks interior mutability. For another, a single `Rc<[u8]>` can't be carved up into smaller buffers of the same type. `Bytes` and `BytesMut` fix both problems. This PR removes the ability to construct an `AioCb` from `Rc<[u8]>` and adds the ability to construct one from `Bytes`, `BytesMut`, or raw pointers (for consumers who need even more flexibility). At this stage, the PR has the following warts: 1. A hack is necessary to force small `Bytes` buffers to allocate on the heap. I plan to fix this with an enhancement to the bytes crate. 2. The `AioCb::buffer` method is necessary due to a deficiency in the tokio-core crate. Once I fix that, then only `AioCb::into_buffer`will need to be public.

bors · 2018-01-21T20:22:11Z

Build succeeded

Susurrus reviewed Dec 21, 2017

View reviewed changes

asomers force-pushed the aio_bytes branch 2 times, most recently from 0b18352 to b2ab037 Compare January 8, 2018 01:15

asomers changed the title ~~WIP: Change AioCb to primarily use Bytes instead of Rc<[u8]>~~ Change AioCb to primarily use Bytes instead of Rc<[u8]> Jan 8, 2018

Susurrus suggested changes Jan 13, 2018

View reviewed changes

asomers force-pushed the aio_bytes branch from b2ab037 to bb46a99 Compare January 14, 2018 03:55

Susurrus suggested changes Jan 14, 2018

View reviewed changes

Susurrus approved these changes Jan 15, 2018

View reviewed changes

asomers added 4 commits January 15, 2018 21:39

Add AioCb::from_ptr and AioCb::from_mut_ptr

93b85d8

aio: Clippy cleanup

ec935e6

aio: more documentation

c3ff37b

asomers force-pushed the aio_bytes branch from 4680698 to c3ff37b Compare January 16, 2018 04:52

asomers mentioned this pull request Jan 16, 2018

More cleanups and enable some more compiler lints #837

Merged

bors bot merged commit c3ff37b into nix-rust:master Jan 21, 2018

Susurrus mentioned this pull request Jan 23, 2018

AioCb::from_boxed_slice violates mutability guarantees #788

Closed

Change AioCb to primarily use Bytes instead of Rc<[u8]> #820

Change AioCb to primarily use Bytes instead of Rc<[u8]> #820

Conversation

asomers commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jan 4, 2018

asomers commented Jan 4, 2018

Susurrus commented Jan 7, 2018

asomers commented Jan 11, 2018

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented Jan 14, 2018

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jan 15, 2018

asomers commented Jan 17, 2018

Susurrus commented Jan 17, 2018

bors bot commented Jan 17, 2018

Timed out

Susurrus commented Jan 17, 2018

bors bot commented Jan 17, 2018

Timed out

asomers commented Jan 18, 2018

bors bot commented Jan 18, 2018

Timed out

asomers commented Jan 18, 2018

bors bot commented Jan 18, 2018

Timed out

Susurrus commented Jan 18, 2018

asomers commented Jan 19, 2018

Susurrus commented Jan 19, 2018

bors bot commented Jan 19, 2018

Timed out

Susurrus commented Jan 21, 2018

bors bot commented Jan 21, 2018

Build succeeded