use futures::Poll (or clone of it) instead of nb::Error

[vitiral]

Prefix and Apology

I went down the wrong track in #2 and japaric/embedded-hal#13. My error was because I was not familiar enough with the inner workings of futures and I used the incorrect terminology.

I do NOT think we should return a Future, but rather that our asyncronous functions should return a type identical to futures::Poll (or possibly futures::Poll itself)

Issue at hand

I suggest that although this is not futures, we should try and keep the API as similar as possible to futures. Future::poll returns the type Poll which is just Result<Async<T>, E>. See: Async.

These types are mathematically identical: futures:

type Poll = Result<Async<T>, E>;

pub enum Async<T> {
    Ready(T),
    NotReady,
}

nb:

type MyResult = Result<T, nb::Error<E>>;
enum Error<E> {
    Other(E),
    WouldBlock,
}

We have simply moved the WouldBlock/NotReady around from Ok to Err.

However, Result<V, nb::Error<E>> is much less ergonomic for a function that is supposed to be asyncronous:

• When someone creates a function that is non blocking, they should be returning a type that specifies the function as non-blocking. That type is the Poll type (although it doesn't have to come from futures).
• Having the function return a Result that could have an Error::WouldBlock is much more difficult to follow -- you have to dig into what the error type is to understand that this is an async function!

Summary

An async function should return Poll type -- not have an "async error." In general, it should mimick the API of Future::poll. Fundamentally this is a usability/readability question, but it will also make it easier to reason about what futures::Poll::from::<nb::Poll>() does, as well as allow us to more easily leverage futures.

I posit that it would be better if we used the futures::Poll type directly, as that type really is zero cost by definition (futures::Poll<T, E> is mathenatically identical to Result<T, nb::Error<E>>) and would make converting async functions to futures much more readable and streamlined (they would automatically implement Future::poll!)

[vitiral]

Also, according to the trait documentation, Future::poll is the only required method to implement a Future! If we simply return future::Poll for our functions, we will have basically already implemented them as futures (we will just have to have a struct which just calls it for the poll method).

[Kixunil]

I actually don't feel like returning Result<Async<T>, E> is the best idea. When playing with futures and trying to implement my own version of them, I realised Async<Result<T, E>> would be better approach.

When writing combinators, most of them don't care whether it's Ok or Err. They just return when Async::NotReady is encountered. So instead of doing this:

let foo = match async_op() {
    Ok(Async::Ready(val)) => val,
    Ok(Async::NotReady) => return Ok(Async::NotReady),
    Err(e) => return Err(e),
};

One would write:

let foo = match async_op() {
    Async::Ready(val) => val,
    Async::NotReady => return Ok(Async::NotReady),
};

It's usually done in macros, but those macros generate the code. Also it avoids type conversion.

Secondly, this allows one to cleanly express operations that can't fail. (Although, I have nothing against Result<T, !>, I think it shouldn't be over-used.)

Thirdly, I guess that would play nice with ? operator.

Finally, I suppose returning Async::NotReady is the common case. Err is probably least common case.

Disclaimer: I haven't explored all uses of Futures, so maybe I'm missing something.

[vitiral]

I also like Async as a type much better, but I REALLY don't think it's worth having an API different than Future::poll here. Futures are the standard in simple async handling, we should maintain as similar an API as we can to make conversion simple and learning straightforward.

[Kixunil]

Maybe open an issue there? While Futures is becoming standard, it's not developed yet and may change.

[vitiral]

The reason Poll is the type it is is so that futures can use and_then, etc (i.e. They behave like iterators). So it's a pretty decent reason.

It's also a reason why putting WouldBlock/NotReady in the Error is not a good idea, Result already has pretty much this exact same api, but it only works if you stop on the first error.

[Kixunil]

I don't think implementing and_then etc on Future would be a problem. Quite the opposite.

[vitiral]

Sorry, I confused a few different topics there... In order for Future to have and_then, Poll has to have an Err type (since Poll is how Future knows whether there was an error). That is why just using Async doesn't work (Async has no concept of errors).

Having Poll just be a Result where Err is a fatal error means it has roughly the same API, which is a benefit as well.

Edit: I tried to explain a bit better and added formatting

[Kixunil]

That is solvable:

trait Future {
    type Item; // Just the item, no Result here.

    // ...

    fn and_then(self) -> AndThen where Self: Future<Item=Result> {
        // ...
    }
}

I was actually trying to do something similar with where Self::Item: SomeTrait, impl<T, E> SomeTrait for Result<T, E>, impl<T> SomeTriat for Option<T>. Unfortunately it collided in some way (Rust is currently unable to understand that Foo<Bar=Result<T, E>> and Foo<Bar=Option<T>> are different traits.)

[vitiral]

but Future::poll has type:

fn poll(&mut self) -> Poll<Self::Item, Self::Error>;

That is why Item doesn't have to be a Result... because Error is defined spearately!

AndThen is then implemented like this:

impl<A, B, F> Future for AndThen<A, B, F>
    where A: Future,
          B: IntoFuture<Error=A::Error>,
          F: FnOnce(A::Item) -> B,
{
    type Item = B::Item;
    type Error = B::Error;

    fn poll(&mut self) -> Poll<B::Item, B::Error> {
        self.state.poll(|result, f| {
            result.map(|e| {
                Err(f(e).into_future())
            })
        })
    }
}

So again, poll has to return a Result in order for AndThen to know how to do result.map in it's poll() method. I'm pretty sure there is no way around that.

[Kixunil]

That is why Item doesn't have to be a Result... because Error is defined spearately!

I know. I suggest removing Error associated type and instead use Result as item for cases when the Future can fail. Don't use Result if future is infallible. Option is another option. ;)

[vitiral]

but if you use a Result as the Item (and don't have an assigned Error) then and_then and related functions don't work.

You can just set Error = ! if future is infallible.

[japaric]

I have already mentioned this but when writing RTFM tasks / callback style code WouldBlock as an error is the right type. The API in this crate is meant to be usable in blocking mode, with futures, with the future #[async] / await! (generators) and with callback code and the current API fits the bill for all of them.

Neither Result<Async<T, E>> or Async<Result<T, E>> has an advantage over nb::Result<T, E> in terms of pattern matching, but both are misleading in callback style code. The former because serial.write(0).unwrap() would not check if the operation completed or not, and the latter because serial.write(0) does not even raise a warning about the operation not even having occurred.

When someone creates a function that is non blocking, they should be returning a type that specifies the function as non-blocking.

The standard library Read / Write traits when used in non-blocking mode return a Result and use the ErrorKind::WouldBlock variant to signal that the operation needs to block to complete. So in a sense we are already using the standard return type for this operation.

Having the function return a Result that could have an Error::WouldBlock is much more difficult to follow

This doesn't matter at all when writing blocking / futures-based code as you are either using block!, try_nb!, hal-blocking or hal-futures and neither of those returns nb::Result; they return a plain Result (or a future that will converge to a Result).

For callback style code this is not as ergonomic as it should be but the alternatives Result<Async<T, E>> and Async<Result<T, E>> are worse (see the second paragraph).

(nb would be nicer if we had "untagged enums" as in Result<T, MyError | WouldBlock>; that would reduce one layer from the pattern matching:

match serial.write(byte) {
    Ok(()) => ..
    Err(MyError::Noise) => ..
    Err(MyError::Overrun) => ..
    Err(WouldBlock) => ..
    // ...
}

but that doesn't exist today)

An async function should return Poll type -- not have an "async error."

The Poll type was created to be the return type of polling a future not to be the return type of an async function. Futures based async functions return impl Future (a type that implements the Future trait). Proper #[async] functions will return an impl Generator (a type that implements the Generator trait)

The only nonblocking functions I have seen in practice are the ones in the standard library Read / Write traits and those return a Result and use the WouldBlock variant to indicate "this operation needs to block to complete". nb is modeled after those functions.

but it will also make it easier to reason about what futures::Poll::from::() does, as well as allow us to more easily leverage futures.

So is the whole point to simplify writing futures based APIs on top of nb? You want to go from:

fn write<S>(byte: u8) -> impl Future<Item = (), Error = Error>
where
    S: hal::Serial,
{
    future::poll_fn(|| {
        Ok(Async::Ready(try_nb!(serial.write(byte))))
    })
}

to:

fn write<S>(byte: u8) -> impl Future<Item = (), Error = Error>
where
    S: hal::Serial,
{
    future::poll_fn(|| serial.write(byte).into())
}

? Well, the required From implementation to make that possible requires (a) that the nb crate depends on the futures crate, or (b) that the futures crate depends on the nb crate. (a) is against the goal of the nb crate and (b) seems unlikely so I don't think either is going to happen.

[Kixunil]

@vitiral why and_then couldn't be implemented for F: Future<Item=Result<T, E>>?

I know one can use !. I just found it cleaner to not differentiate between Ok and Err in then() implementation (and others as well).

@japaric

and the latter because serial.write(0) does not even raise a warning about the operation not even having occurred.

The solution is obviously very simple: #[must_use = "Operation might not completed"]

The standard library Read / Write traits when used in non-blocking mode return a Result and use the ErrorKind::WouldBlock variant to signal that the operation needs to block to complete. So in a sense we are already using the standard return type for this operation.

This breaks when read_exact() and write_all() are used. I find it quite unfortunate.

[vitiral]

@Kixunil if you have a chain like this:

my_future.and_then(|r| ...).and_then(|r| ...)

If the first and_then fails you must not call the others. Therefore there has to be a "quit" type as part of Poll. That type is simply Error. For nb that type is Error::Other.

If you still don't understand, I would recommend copy/pasting the code that I linked and try to implement what you are saying. However, the point is semi-irrelevant because of my reply to japaric.

@japaric

The former because serial.write(0).unwrap() would not check if the operation completed or not, and the latter because serial.write(0) does not even raise a warning about the operation not even having occurred.

Ah, this (along with io::ErrorKind::WouldBlock) is the missing piece for me. This api makes much more sense now! I will re-look at the crate and see if I can get a PR that better documents the API. I'll have the PR close this issue.

Thanks for your patience and excellent explanation.

[Kixunil]

@vitiral I'm sure that is implementable. I don't have much time for it now though. Basic idea is to provide and_then() only for fallible operations (Item = Result).

[japaric]

@Kixunil

The solution is obviously very simple: #[must_use = "Operation might not completed"]

Yeah but even with that Async<Result<T, E>> is not as ergonomic as nb::Result<T, E>. With the later I can write serial.write(byte).unwrap_or_else(|| /* something */) (in callback-style code), but with the former I have to sort of unwrap twice: once to remove the Async layer and another one to remove the Result layer.

This breaks when read_exact() and write_all() are used. I find it quite unfortunate.

Are you referring to the standard library version of those functions? nb::Result and the WouldBlock idea is only applicable to operations that don't have internal state. Remember that the core operations here are read and write which map to syscalls. read_exact and write_all are stdlib specific constructs on top of read / write that contain internal state; WouldBlock is not applicable in that case. Tokio deals with those two functions differently as well.

[Kixunil]

I have to sort of unwrap twice: once to remove the Async layer and another one to remove the Result layer.

This depends heavily on how one uses it.

Are you referring to the standard library version of those functions?

Yes, I mean that standard library is broken in some sense. I say it to avoid this in other crates.

BTW Tokio had it incorrect too - it assumed that everything was written with async in mind. (If someone used read_exact or write_all, the code was broken.) They changed it to require AsyncRead and AsyncWrite marker traits (right decision IMHO).