Add futures-io #780

cramertj · 2018-02-20T22:56:15Z

Currently, a few combinators have been disabled because they rely on BufRead capabilities. We should probably add an AsyncBufRead in order to bring back this functionality, but I have not yet done so.

I have also not yet ported the tests from the original repo. I plan to add these in a future commit.

seanmonstar · 2018-02-20T23:15:09Z

futures-io/src/lib.rs

+        /// methods, and that the return value of the method accurately reflects
+        /// the number of bytes that have been written to the head of the buffer.
+        #[inline]
+        pub unsafe fn nop() -> Initializer {


What is nop? Why not use a descriptive name, like the alternate constructor, zeroing?

This is done to match the current (unstable) std API. When that API is released, this Iniitalizer will be changed to a type alias to that one: https://doc.rust-lang.org/std/io/struct.Initializer.html

The name nop seems unfortunate. Regardless of the previous naming choice of an unstable std API, it seems better to pick a better name in a new library.

seanmonstar · 2018-02-20T23:15:50Z

futures-io/src/lib.rs

+        /// return a non-zeroing `Initializer` from another `AsyncRead` type
+        /// without an `unsafe` block.
+        #[inline]
+        unsafe fn initializer(&self) -> Initializer {


I thought the point of the Initializer having an unsafe constructor was so that this method didn't need to be unsafe?

Initializer::zeroing() is safe, Initializer::nop() is unsafe. It is safe to implement this method using Initializer::zeroing(), but unsafe to implement it using Initializer::nop().

This function is made unsafe so that users cannot implement it safely by delegating to the internally-unsafe implementation of the initializer function on another type.

All of this is copied from the std::io API. If you have comments, questions, or ideas about how to improve this design, please leave them in the tracking issue. If that API changes, we will update this one as necessary.

Quoting from the 0.2 RFC:

This resolves the issue of the unsafe on prepare_uninitialized_buffer being unsafe to implement, not unsafe to use (the more common, and arguably the correct meaning of unsafe fn).

The implementation doesn't seem to match, since even if Initializer::zeroing() is returned from fn initializer(), it doesn't matter, because it's unsafe to call initializer().

I'm leaving comments here because this API is being proposed in futures, a library that I rely on, and a place where stability markers cannot protect the it. The tracking issue in std hasn't seen much traction otherwise.

This PR does indeed address the issue I raised in the text you quoted. However, I think the wording I used may have been confusing.

Currently, unsafe fn prepare_uninitialized_buffer is unsafe to implement, but not unsafe to call. unsafe on functions is intended to mean "unsafe to call", not "unsafe to implement." If an RFC like rust-lang/rfcs#2316 were to be approved, the prepare_uninitialized_buffer API would be unsound.

This PR fixes this problem by moving the unsafety to the call to Initializer::nop(). Even if rust-lang/rfcs#2316 were approved, this API would be sound. The initializer function itself is still unsafe, but the reason is different: it isn't that initializer is unsafe to implement, but that calling initializer is unsafe because it could allow you to produce values of type Initializer::nop() even when your AsyncRead implementation read from the supplied buffer.

Currently, unsafe fn prepare_uninitialized_buffer is unsafe to implement, but not unsafe to call.

I assume you mean that is the intent, since it definitely is unsafe to call the function with the current compilers.

I find the reasoning that you could get an Initializer::nop() back a little bit arbitrary: you don't find this sort of unsafe "poisoning" elsewhere. For instance, you can get a string back with str::from_utf8_unchecked(), but that shouldn't mean that any function that happens to wrap that is inherently unsafe.

Consider this implementation:

struct MyUnsafePerformantRead; impl AsyncRead for MyUnsafePerformantRead { fn initializer(&self) -> Initializer { unsafe { Initializer::nop() } } fn poll_read.... // Doesn't read from the buffer } struct MySafeButStillUninitRead; impl AsyncRead for MySafeButStillUninitRead { fn initializer&self) -> Initializer { MyUnsafePerformantRead.initializer() } fn poll_read.... // Does read from the buffer (uninitialized memory), causes UB }

Yes, I imagined that is the concern, but I don't see anything that adding an unsafe to the function will help with. In fact, adding the unsafe keywords doesn't protect it at all. A wrong implementation is still wrong.

unsafe prevents the unsound implementation of MySafeButStillUninitRead (undefined behavior without unsafe code). If MyUnsafeButPerformantRead::initializer() is unsafe, then, MySafeButStillUninitRead::initializer would have to use unsafe in order to trigger the UB.

I'm even more unconvinced now, since you can easily grab an Initializer from a completely unrelated reader, and carry it around, and safely use it via unrelated_initializer.initialize(my_buf) everywhere else, and trigger the UB.

As a not-arbitrary example, consider Chain implementation takes Box<AsyncRead>s, and pushes them into a vec. The naive implementation calls self.reads[0].initializer(), and saves it, and then uses it in later reads, instead of remembering to ask for a new one.

I think the std API gives more an illusion of protection against UB, while still quite easily allowing it to happen, even by mistake.

since you can easily grab an Initializer from a completely unrelated reader, and carry it around

Not without using unsafe. If you're making unsafe calls and then randomly passing around the result, in explicit violation of the invariants stated in the docs, then you're right, it is easy to cause UB.

seanmonstar · 2018-02-20T23:19:09Z

futures-util/src/io/codec.rs

@@ -0,0 +1,373 @@
+//! Utilities for encoding and decoding frames.


All of the codec stuff seems out of scope.

seanmonstar · 2018-02-20T23:20:23Z

futures-util/src/io/codecs.rs

@@ -0,0 +1,124 @@
+use bytes::{Bytes, BufMut, BytesMut};


This codecs stuff also seems out of scope.

seanmonstar · 2018-02-20T23:20:56Z

futures-util/src/io/framed.rs

@@ -0,0 +1,257 @@
+use std::io::{self, Read, Write};


This framed/codec stuff seems out of scope.

seanmonstar · 2018-02-20T23:23:20Z

futures-util/src/io/mod.rs

+            return Ok(Async::Ready(0));
+        }
+
+        let n = try_ready!(self.poll_write(buf.bytes(), cx));


Seems the default implementation here could make use of self.poll_vectored_write.

Done! Thanks for the reminder :)

seanmonstar · 2018-02-20T23:26:58Z

futures-io/src/lib.rs

+    /// A type used to conditionally initialize buffers passed to `AsyncRead`
+    /// methods.
+    #[derive(Debug)]
+    pub struct Initializer(bool);


Looking at the std Read::initializer tracking issue, it seems there is doubt that this is the correct API. In std, the unstable attribute protects them.

It seems odd to be using that API here if it's thought to be inferior, especially since we can't rely on unstable to protect against breaking changes.

It's true that some people have doubts about the API. I personally think that it's the best-designed interface for this that I've seen, and it's the only one I've seen that is sound in the presence of rust-lang/rfcs#2316.

carllerche · 2018-02-20T23:46:42Z

For the record, Tokio will continue to provide it's own AsyncRead / AsyncWrite traits as well as all the utilities that were copied from the Tokio source.

I/O is fundamental to Tokio as such it needs to maintain its own traits (they will probably diverge some).

aturon · 2018-02-20T23:59:09Z

@carllerche I continue to see that as a rather unfortunate decision, since a basic goal here is to provide interoperation between different backing I/O systems, and basic implementation-agnostic conveniences -- exactly akin to std::io. I'm not aware of anything on a technical level that would prevent Tokio from using these traits. But I suspect it's fruitless to try to argue this point further.

In any case, the good news is that Rust provides ample means for "external" interoperation (through newtype wrappers and the like), which means the Tokio team's stance here will merely lead to increased friction, rather than total inability to interoperate.

carllerche · 2018-02-21T00:11:05Z

@aturon Tokio already provides traits for interop that are decoupled from the reactor. I'm not sure what the value is in renaming the lib from tokio-io to futures-io and creating an additional standard instead of making the changes in the existing tokio-io crate, especially since it looks like you are copying the contents.

But yes, as you mentioned, one will always be able to write adapters.

aturon

This basically LGTM, modulo small convention issue I mentioned.

As to the initializer stuff: I believe that we should match what std does, as planned, and address any issues upstream, pushing toward stabilization. Once that's done, we can adopt any changes here in e.g. 0.3.

aturon · 2018-02-21T00:49:52Z

futures-io/src/lib.rs

+        /// If reading would block, this function returns `Ok(Async::Pending)`
+        /// and arranges for `cx.waker()` to receive a notification when the
+        /// object becomes readable or is closed.
+        fn poll_read(&mut self, buf: &mut [u8], cx: &mut task::Context)


As per IRC discussion, let's make cx come first, consistently.

cramertj requested a review from aturon February 20, 2018 22:56

cramertj force-pushed the futures-io branch from fcb2fb9 to 78f045d Compare February 20, 2018 23:14

seanmonstar reviewed Feb 20, 2018

View reviewed changes

cramertj force-pushed the futures-io branch from 78f045d to b74774f Compare February 20, 2018 23:58

cramertj force-pushed the futures-io branch from b74774f to 98b1176 Compare February 21, 2018 01:02

Add futures-io

c3d3e08

cramertj force-pushed the futures-io branch from 98b1176 to c3d3e08 Compare February 21, 2018 01:05

aturon approved these changes Feb 21, 2018

View reviewed changes

Make cx always the first argument

00f8f37

cramertj merged commit c1297ec into rust-lang:0.2 Feb 21, 2018

cramertj deleted the futures-io branch February 21, 2018 05:09

cramertj mentioned this pull request Mar 20, 2018

Change argument order of poll_read and poll_write #883

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add futures-io #780

Add futures-io #780

cramertj commented Feb 20, 2018 •

edited

Loading

seanmonstar Feb 20, 2018

cramertj Feb 20, 2018

seanmonstar Feb 21, 2018

seanmonstar Feb 20, 2018

cramertj Feb 20, 2018

seanmonstar Feb 21, 2018

cramertj Feb 21, 2018 •

edited

Loading

seanmonstar Feb 21, 2018

cramertj Feb 21, 2018

seanmonstar Feb 21, 2018

cramertj Feb 21, 2018 •

edited

Loading

seanmonstar Feb 21, 2018

cramertj Feb 21, 2018

seanmonstar Feb 20, 2018

seanmonstar Feb 20, 2018

seanmonstar Feb 20, 2018

seanmonstar Feb 20, 2018

cramertj Feb 20, 2018

seanmonstar Feb 20, 2018

cramertj Feb 20, 2018

carllerche commented Feb 20, 2018

aturon commented Feb 20, 2018

carllerche commented Feb 21, 2018

aturon left a comment

aturon Feb 21, 2018

cramertj Feb 21, 2018

		@@ -0,0 +1,373 @@
		//! Utilities for encoding and decoding frames.

Add futures-io #780

Add futures-io #780

Conversation

cramertj commented Feb 20, 2018 • edited Loading

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

cramertj Feb 21, 2018 • edited Loading

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

cramertj Feb 21, 2018 • edited Loading

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

carllerche commented Feb 20, 2018

aturon commented Feb 20, 2018

carllerche commented Feb 21, 2018

aturon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj commented Feb 20, 2018 •

edited

Loading

cramertj Feb 21, 2018 •

edited

Loading

cramertj Feb 21, 2018 •

edited

Loading