Have service methods return a Future<T>.

[thedodd]

To accomplish this, there will have to be a decent number of backwards incompatible changes, but moving towards long-term stability and interoperability with the async I/O ecosystem, this may be a HUGE win. Some of the biggest benefits:

• error type coercion could be defined and then used in handlers for better error handling and propagation.
• for gRPC patterns of error handling where the error is embedded in the response type (way more flexible), this would still work. Wouldn't be able to use ? though.
• the async/await ecosystem seems to be depending upon #[async] methods having a return type of Result<T>. Aligning with this will help in achieving C10K+ level throughput with these services.

I know it is not as simple as this. The grpc lib here has a lot of code built up around not returning Result types. But I wanted to at least get the discussion started. I would rather put in the work on this project as opposed to start a completely new project.

[stepancheg]

I'm not sure I understand.

Result is now Future or Stream.

AFAIU, these two declarations should be equivalent:

trait MyService {
    fn foo(&self) -> Future<MyReturn, MyError>;
    #[async]
    fn foo(&self) -> Result<MyReturn, MyError>;
}

(Unfortunately, I couldn't verify it, because compiler reports error when I use #[async] in trait).

So trait signatures in grpc are async now. And ideally you should be able to choose async/await or direct future/stream implementation, and it doesn't matter which style declaration is.

(And even if it wouldn't possible to implement function with Future signature with #[async], you can always use async_block!).

C10K is possible now, because functions are async, as I said.

[crackcomm]

What is the benefit of returning Result as opposed to a Future? I can't see any, more than that - I see it as harmful for the interfaces - ServiceAsync? I think we just recently got rid of that.

[crackcomm]

error type coercion could be defined and then used in handlers for better error handling and propagation.

I am not sure I understand but futures are defined as Future<Item=T Error=E> whereas results are defined as Result<T, E> and both contain the same error type information.

for gRPC patterns of error handling where the error is embedded in the response type (way more flexible), this would still work. Wouldn't be able to use ? though.

gRPC error codes could be returned either using a Result or a Future type.

the async/await ecosystem seems to be depending upon #[async] methods having a return type of Result. Aligning with this will help in achieving C10K+ level throughput with these services.

I am not completely sure I understand what do you mean but maybe you are more concerned about scheduling of these futures on the server?

[thedodd]

Hey all, thanks for getting back with me. All solid feedback, and after reviewing your comments a few times, I am left with a few questions & a few things I need to test. A few things are still up in the air:

AFAIU, these two declarations should be equivalent:

@stepancheg in principal I agree, however:

(Unfortunately, I couldn't verify it, because compiler reports error when I use #[async] in trait).

Yea, I will need to do some additional testing as well (needs to be on nightly, of course), just to ensure there are no edge cases we are overlooking.

Just to expound on a few points (which I should have clarified in my original comment):

• the efforts around async/await in rust are falling into pretty specific patterns. One of those patterns is described here: https://github.com/alexcrichton/futures-await#technical-details

  #[async] - this attribute can be applied to methods and functions to signify that it's an asynchronous function. The function's signature must return a Result of some form (although it can return a typedef of results). Additionally, the function's arguments must all be owned values, or in other words must contain no references. This restriction may be lifted in the future!

The essence of this issue is focused on alignment with the direction that async/await is going in rust (I know it is not stable yet, but mostly trying to generate discussion and todo items for this), and the futures-await effort seems to be a source of truth on that front.

So, @crackcomm

What is the benefit of returning Result as opposed to a Future?

The return type definitely should be Future. I've updated the title to reflect that.

---

The items that are outstanding then:

• test this on nightly by using methods returning Results & marked with #[async] and ensure they still work with the generated code (they should coerce to the needed Future types ... we'll see).
• as far as scheduling of the futures, would be nice to experiment with the new tokio revamp which is taking place, so that we could use cpu pool or tokio/mio reactor for higher throughput (depending on workload, of course).

[andrenth]

Hi

I feel like I’m missing something here, so apologies if this is a stupid question.

I understood this issue was a request to switch from {Single,Streaming}Response to a Result<...> type in rpc methods in service traits. Is that correct?

The answers seem to indicate this is already possible, but looking at the library code, I don’t see how.

Cheers.

[thedodd]

@andrenth the idea is that if you choose to use nightly & enable proc macro, conservative impl trait & generators as described here, then you can simply decorate one of your handlers with #[async] and it will transform your Result into a Future.

** Disclaimer, theoretically this should work with the gRPC system here, but per my comments above, this remains to be tested. Please report your findings if you get around to doing this.

(UPDATE): tested. Does not work. As response types do not actually impl Future.

[andrenth]

@thedodd Yeah, I understood that idea, but currently handlers return SingleResponse or StreamingResponse, not Result. That's why I'm confused.

[thedodd]

Yea, both types of futures. The question that needs to be answered is if the compiler will coerce our Result type (which is already being coerced into a Future when the function is decorated with #[async]), into the appropriate SingleResponse or StreamingResponse future.

Need to test and find out where the walls are. Hopefully I’ll be able to do so as well soon.

[thedodd]

Hopefully I am answering your question effectively.

[andrenth]

I think I see what you mean now. They're not futures exactly, but newtypes for futures. What you're trying to find out is if the #[async] decorator will see through that.

[thedodd]

Yea, https://docs.rs/grpc/0.2.1/grpc/struct.SingleResponse.html looks like they are thin wrappers around GrpcFuture. Could be problematic.

[thedodd]

So, it has been quite some time since this discussion was started.

Per the discussion we had going above, the return type of the services (E.G., those which return a SingleResponse, same with the other response types), are not actually futures. They are just structs which wrap a future, but are not actually futures.

So, as of now, we can not actually decorate service request handlers with #[async] or #[async(boxed)]. E.G.:

/// *NOTE:* the return type here is neither a `Result` nor a `Future`.
/// It is a struct wrapping a `Future`, but itself does not impl `Future`.
fn create_project(&self, opts: RequestOptions, req: CreateProjectRequest) -> SingleResponse<CreateProjectResponse> { ... }

Per the above example, decorating the fn with #[async] would break due to the fact that the return type no longer conforms to the generated grpc code. Moreover, decorating and updating the return type to be a result (or any other type), breaks for the same reason.

So, to state this succinctly, if we were to update the various response types to implement Future themselves, then interop with async/await is g2g.

All in all, I should update the title to be Have service methods return a `Future<T>`. If this change were to be made, then we could use async/await out of the box with this framework.

[autrilla]

Has there been any progress on this? I'd be willing to put time into it and send a patch if I had some guidance on what's acceptable. Is modifying the service trait functions to return a Future<T> acceptable?

[thedodd]

@autrilla I’ve been using a workaround. Basically I’ve been having the service impl methods call private methods which are async, and then wrap the future in the SingleResponse type or the appropriate stream type from the service impl method.

This has been working pretty well, and has allowed for significantly more robust error handling.

Once the response types are restructured to use futures, then we won’t need private async methods anymore.

As far as framework progress in this front, haven’t seen any movement yet.

[autrilla]

@thedodd thanks a lot for the response. I'm not sure I understand how you wrap the future in the SingleResponse. Do you just manually create the SingleResponse with ::new, adding the metadata in yourself?

[thedodd]

@autrilla yea, there are a few options you have there, but that is the idea. Manually construct the needed response object. Add the future and any metadata that you need.

There are a few options that you have for constructors. I find myself using SingleResponse::no_metadata quite a lot. With this approach, you just pass in the future, and you’re g2g.

[thedodd]

Given where async/await is at, and futures 0.3 (plus the core::future trait), might be a good time to put together a nightly branch which uses tokio (as opposed to tokio-core [old]), futures 0.3 & async/await.

Thoughts?

[stepancheg]

FYI, I'm currently trying to rewrite grpc-rust API to do stream+sink interface instead of stream+stream.

(It's on my laptop now, unfinished)

So, basically, the signature of bidi method for both client and server is:

fn bidi(request: Stream<Req>) -> Stream<Resp>

and will be

fn bidi(req: Stream<Req>, resp: Sink<Resp>); // for serverto implement
fn bidi() -> (Stream<Req>, Sink<Resp>); // for client

This way it will be more similar to grpcio crate implementation and I think easier to use.

After this work is done with can think of switching to futures 0.3.

[thedodd]

@stepancheg nice. Glad to hear it. So, it may actually be best to cut over to tokio (away from tokio-core) first before making the change to Futures03, as tokio-core is deprecated and its repo is archived.

After that, cutting over to Futures03 may be a bit easier because tokio already has some amount of support for working with Futures03 and even with async/await. Moving to tokio would make things much easier I would say.

Also, is there anything I can help out with @stepancheg? At this point, I definitely have a vested interest in this crate, and would be happy to help out.

[stepancheg]

@thedodd if you are willing to help, there are basically four areas for improvements:

• grpc-rust (please don't touch it for now, as I'm doing refactoring)
• rust-http2 (my crate used by grpc-rust)
• rust-protobuf
• rust-tls-api (abstraction over TLS implementations)

rust-protobuf is in more or less good shape, it has a list of open issues, pick any.

rust-http2 and rust-tls-api code is messy, not well tested, undocumented, and requires improvements. So you can just open any of these projects and do whatever you think makes the code better. And I should create a list of issues for those projects.

rust-http2 is very important because it does all heavy lifting for rust-http2, grpc-rust itself is a thin layer over rust-http2.

And help is very appreciated.

[thedodd]

@stepancheg what are your thoughts on https://github.com/carllerche/h2??? The tower-grpc folks are also using it for their up-and-coming gRPC framework via the https://github.com/tower-rs/tower-h2 crate.

We might be able to leverage it here. That would get us off of tokio-core and onto tokio. Seems like that is what would actually need to happen either way. Either rust-http2 gets updated to move to tokio or we just move this crate onto h2. Thoughts?

[stepancheg]

@thedodd the project started because rust-http2 code quality is very bad according to the author (he is correct).

Once I tried to write a benchmark to compare its performance to rust-http2, and immediately found a bug in h2 by simply executing in a loop an example provided by the project, which was very disappointing to me, because problem reproduced 100% times, and test case was trivial. Now that bug is fixed, I need to try it again, but I'm afraid more bugs exist in it.

Anyway, I'm planning to do another iteration of performance comparison.

BTW, what are the reasons to move to newer tokio and futures 0.3? I probably missed something.

[thedodd]

what are the reasons to move to newer tokio and futures 0.3?

A few reasons:

• old tokio-core crate is deprecated and archived (nothing new happening there).
• futures03 is an implementation of the core::future::Future trait. Futures01 is not.
• async/await uses the core future trait.
• moving forward, if we want to keep this framework interoperable with the rest of the ecosystem in a sane manner, we will have to move to tokio.

EG, the issue that I am most concerned about is that, even today, using any of the modern futures based crates out there requires tokio not -core. So spawning new futures from within a grpc handler in this crate today essentially requires you to use a futures::sync::oneshot pattern to get the future onto a tokio pool.

If tokio was being used by this system, a user normally wouldn't have to worry about spawning new futures, as the default tokio executor would be used. Big win for ergonomics.

[stepancheg]

@thedodd is futures 0.3 compatible with stable rust?

[thedodd]

Not yet. It is currently held behind the #![feature(futures_api)] feature.

IMHO, cutting over to tokio as the first order of business would probably be a wise move. It runs on stable, works with Futures01 (tokio plans to support both), and would achieve immediate interoperability with the rest of the rust ecosystem.

Thanks for all of the discussion, @stepancheg. I definitely don't mean to heap all of this onto your shoulders or anything, to be sure.

[stepancheg]

@thedodd in that case switching to tokio would be the right thing, I guess

[thedodd]

Well, I will do a code deep-dive tomorrow and see if I can put together a high-level analysis and plan of attack so that we can all coordinate and such. I'll go ahead an open a new ticket tomorrow with the results of that deep-dive. I'll close this issue tomorrow as well and then link this issue to the new one.