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.

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

Switch to std::future::Future #485

Closed
ebkalderon opened this issue Aug 29, 2019 · 20 comments · Fixed by #603
Closed

Switch to std::future::Future #485

ebkalderon opened this issue Aug 29, 2019 · 20 comments · Fixed by #603

Comments

@ebkalderon
Copy link
Contributor

Transitioning from futures 0.1 to std::future::Future would greatly improve the user experience in the near future, especially with async/await stabilizing in Rust 1.39 and std::future::Future compatible versions of tokio, hyper, and tower on the horizon. It seems that jsonrpc-core is currently the only library in my personal JSON-RPC projects that still relies exclusively on futures 0.1, and it is blocking me from switching over to the new trait completely. I understand if this issue will sit on the backburner for the time being until at least tokio 0.2.0 is released, though.

@tomusdrw
Copy link
Contributor

Yeah, I think it would be great to switch at least jsonrpc-core (and derive maybe) initially, the transports would be fine with the Compat layer.

@tomaka you have some experience with porting to std::future, any hints? :)

@tomaka
Copy link

tomaka commented Aug 31, 2019

Most of the transition should be straight-forward, except that you might have to add where F: Unpin a bit everywhere. The tokio runtime can be replaced with what is in futures-preview::executor.

One part where it might be difficult is that libraries compatible with new futures have ditched lots of combinators in favour of async-await-friendly methods that borrow objects. For example, calling incoming() of async-std::TcpListener (which returns a stream of incoming connections) no longer takes ownership of the TcpListener but borrows it. This is very user-friendly with async/await, but is a pain in the ass if you don't have access to async/await yet. In libp2p, we decided to wait for async/await to be stable for this reason.

Also, unless that changed, if you use tokio for networking, you will have to use tokio for executing futures as well. I'd encourage using futures-timer and async-std instead of tokio.

@ebkalderon
Copy link
Contributor Author

According to Rust Forge, version 1.39 will land on November 7th, which will include stable async/await. In that case, should we start transitioning to std::future::Future now? Or should we wait for the API redesign that @tomaka is working on to land first?

@tomusdrw
Copy link
Contributor

tomusdrw commented Sep 8, 2019

I think it's worth start the transition, I didn't have time to look into it yet. I'd be really happy to accept a PR though if you are interested in working on this @ebkalderon :)

@ebkalderon
Copy link
Contributor Author

@tomusdrw Sure, I suppose I can try!

@ebkalderon
Copy link
Contributor Author

ebkalderon commented Sep 8, 2019

I've successfully converted jsonrpc-core to std::future::Future, but I am blocked on jsonrpc-server-utils since it requires tokio, and tokio-sync >=0.2.0-alpha.2 apparently requires a nightly compiler and async/await.

EDIT: See the switch-to-std-future branch for details.

One particularly unfortunate change in futures 0.3 is the loss of the IntoFuture trait, which means that closures passed to IoHandler::handle_request(), IoDelegate::add_method(), and all related methods look a bit less nice than they do currently. This isn't a deal-breaker, though, as the crate is still perfectly usable without it.

@tomusdrw
Copy link
Contributor

tomusdrw commented Sep 8, 2019

Can we just convert jsonrpc-core for now and use the Compat layer for the rest?

@ebkalderon
Copy link
Contributor Author

@tomusdrw Almost, but not quite. It seems we will also need to make some changes to jsonrpc-derive, since the code produced by MethodRegistration::generate() makes use of the now-nonexistent IntoFuture trait. We will likely need to explicitly check whether each RPC method returns Result or a future type, and wrap the former in a futures::future::ready() where necessary. I'm not exactly sure how to do this with syn, but I assume there is a way.

@ebkalderon
Copy link
Contributor Author

Apparently, there was mention of IntoFuture in the original async/await Rust RFC, as I had learned from this recent blog post, but it seems it was permanently dropped. I'm not exactly sure how to move forward with the conversion process since the lack of IntoFuture will likely substantively break the functionality of jsonrpc-derive.

@tomusdrw
Copy link
Contributor

We can force the methods to return a Future instead of IntoFuture. So sync calls won't be a first-class citizen any more.

Please also follow https://github.com/paritytech/jsonrpsee/ as that's a new library we are working on that is fully Futures-based.

@frondeus
Copy link

Now async/await is stable - do you need any help with migration @ebkalderon?

@ebkalderon
Copy link
Contributor Author

ebkalderon commented Dec 16, 2019

@frondeus Stability of async/await wasn't exactly a blocker for starting migration, but rather the removal of the IntoFuture trait. I don't believe this library can be migrated to async/await without changing the API semantics since it relies very heavily on IntoFuture in order to have the API accept both Result and Future types in numerous areas, with the distinction being that the former is used for synchronous requests and the latter for asynchronous requests. I'm personally waiting for jsonrpsee to stabilize and replace this library completely, since the IoHandler architecture was apparently overdue for a redesign anyway. See the comment from @tomusdrw above.

@ebkalderon
Copy link
Contributor Author

Good news, looks like IntoFuture is coming back! It might be possible to convert jsonrpc-core as-is to async/await if this lands. 🎉

@Thegaram
Copy link

Thegaram commented Feb 1, 2020

Hey @ebkalderon, now that IntoFuture is back, do you think it would be more straightforward to migrate to std::future?

I'm trying to call an async function from within my RPC handler but I find it troublesome to convert the result to futures::Future. Would be awesome if we could just write async handlers directly.

@frondeus
Copy link

frondeus commented Feb 1, 2020

It is not back yet - it was reverted cause of some strange performance drops.
rust-lang/rust#67982

@Thegaram
Copy link

Thegaram commented Feb 2, 2020

Oh I see, thanks for the info @frondeus!

Then I assume the only way to use std::future::Future with this crate for now is to convert it to a futures 0.1 Future? Any recommendations for how to do this properly? Relevant resources I found [1, 2] seem to be outdated. I tried the following but suspect there must be an easier way...

impl<T, X: std::future::Future<Output=Result<T, Error>>> futures::future::Future for X {
    type Item = T;
    type Error = Error;
    // ...
}

@Thegaram
Copy link

Thegaram commented Feb 4, 2020

If anyone wants to use async functions in RPC handlers, this is how I resolved it.

First, you'll need the compat feature:

futures = { version = "0.3.2", features = ["compat"] }

Then you can have an RPC handler like this (see this and this):

use jsonrpc_core::BoxFuture;
use futures::future::{FutureExt, TryFutureExt};

fn handler(&self, ...) -> BoxFuture<...> {
    // ...

    // this block should produce an `RpcResult`
    let fut = async move {
        // ...
        Ok(res)
    };

    Box::new(fut.boxed().compat())
}

Additionally, you need to make sure that all your types are Send. This helped me a lot in understanding why my fut was not Send. You can also annotate fut to learn more:

use futures::future::Future;
let fut2: Box<dyn Future<Output=Result<..., ....>> + Send> = Box::new(fut);

You also need to make sure that there's no reference to self within your async block, so try to clone anything you might need before that. Otherwise you'll run into lifetime issues.

let field = self.field.clone();

let fut = async move {
    // ...
    let x = field.bar().await;
    // ...
};

(Sorry if this is off-topic; it seems useful to me before this crate is updated to use std::future.)

@ebkalderon
Copy link
Contributor Author

@Thegaram Thanks for the tips! Using your instructions, I was able to get async fn methods to be usable as RPC handlers in tower-lsp and support async/await (see ebkalderon/tower-lsp#101). However, there are multiple levels of nested boxing and pinning involved, since I am in turn converting the IoHandler::handle_request() future back into an std::future::Future, so I doubt this approach is resource efficient.

I'm still holding out for jsonrpsee to gain a bidirectional stdio transport so I can switch tower-lsp over to it, but it doesn't appear to be a high priority at the moment.

@ebkalderon
Copy link
Contributor Author

@Thegaram I've found that you can eliminate the double nested future boxing by creating the following type alias to use in place of jsonrpc_core::BoxFuture:

pub type BoxFuture<T> = Compat<Pin<Box<dyn Future<Output = Result<T, Error>> + Send>>>;

Then you would change your RPC handler example to this instead:

use futures::future::{FutureExt, TryFutureExt};

type BoxFuture<T> = Compat<Pin<Box<dyn Future<Output = Result<T, Error>> + Send>>>;

fn handler(&self, ...) -> BoxFuture<...> {
    // ...

    // this block should produce an `RpcResult`
    let fut = async move {
        // ...
        Ok(res)
    };

    fut.boxed().compat()
}

@JanisGailis
Copy link

Hi!

Thanks for all the work involved in making this library!

I'm exploring what options for using JSON-RPC in Rust there are for a project I'm working on now and I was wondering what's the status of converting jsonrpc-core to std::future::Future, or alternatively, what's the status of jsonrpsee?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants