Proposal to implement async API (Futures)

[hugues31]

Hello,

After some research, I decided to give a try to Futures to come up with a simple async API. The main idea is to keep everything very straightforward for the user (the event loop is created by the API).

The idea originates from here : #9

The source code is :

#![feature(conservative_impl_trait)]

extern crate futures;
extern crate hyper;
extern crate tokio_core;
extern crate serde_json;

use futures::Future;
use futures::future;
use futures::stream::Stream;

use hyper::Client;
use serde_json::Value;


struct Api {
    core: tokio_core::reactor::Core,
}

impl Api {
    pub fn new() -> Api {
        Api {
            core: tokio_core::reactor::Core::new().unwrap(),
        }
    }

    pub fn time(&self) -> impl Future<Item = Value, Error = hyper::Error> {
        let url = "http://date.jsontest.com";
    
        let uri = url.parse::<hyper::Uri>().unwrap();

        let handle = self.core.handle();
        let client = Client::new(&handle);

        client.get(uri).and_then(|res| {
            res.body().concat2().and_then(move |body| {
                let v: Value = serde_json::from_slice(&body).unwrap();
                future::ok((v))
            })
        })
    }
}

fn main() {
    // create the API and the Tokio core
    let mut api = Api::new();

    // BENCHMARK THE SYNC VERSION
    let work_1 = api.time(); // create the handle, the client and send request inside a Future
    let res = api.core.run(work_1).unwrap();
    let time_1 = res["milliseconds_since_epoch"].as_f64().unwrap();

    let work_2 = api.time(); 
    let res = api.core.run(work_2).unwrap();
    let time_2 = res["milliseconds_since_epoch"].as_f64().unwrap();

    println!("\nSync version takes {} milliseconds to execute.", time_2 - time_1);

    // BENCHMARK THE ASYNC VERSION
    let work_async = api.time().join(api.time());
    let (res_1, res_2) = api.core.run(work_async).unwrap();
    let time_1 = res_1["milliseconds_since_epoch"].as_f64().unwrap();
    let time_2 = res_2["milliseconds_since_epoch"].as_f64().unwrap();

    println!("\nAsync version takes {} milliseconds to execute.\n", (time_2 - time_1).abs());
}

Cargo.toml :

[dependencies]
futures = "0.1"
hyper = "0.11"
tokio-core = "0.1"
serde_json = "1.0.5"

The code below returns:

Sync version takes 198 milliseconds to execute.

Async version takes 5 milliseconds to execute.

What do you think? Since it's a huge rewrite it would be nice to have some feedbacks :)

[ainestal]

This changes fundamentally how the whole library works and will break any currently working applications using it.

That being said I like the idea because enables the possibility of using APIs that push events alongside APIs that are polling all the time. That is the main argument for me to use this approach.

The performance is also a good point, but currently is not too bad, specially considering the polling is around 1-2s in the exchanges we are supporting. For me in this context some mili seconds are not a big deal.

I specially agree with the statement:

The main idea is to keep everything very straightforward for the user

So go for it! If you need help let me know.

[hugues31]

@ainestal Thanks for your input. Indeed, this is a breaking change (Coinnect 0.6 here we go). For the performance, this is exactly the same as before if the Future contains only 1 request but as the number of requests increase the advantage of async API surge. We can add an example where we compare the time taken to pull the price of the pair BTC-ETC for the 4 exchanges with the sync and async method as shown above. My guess is that async version is ≈300% faster

[crackcomm]

Hey, I am currently working on streams @ crypto-bank/orca.
I will finish streams and I will catch up with you guys here.

In the meantime what do you guys think about using #47 and implement traits for generated code?

[ainestal]

@crackcomm I think it can be very useful to have the code generation

[hugues31]

@crackcomm From what I have seen, orca is a Poloniex websocket API implementation, right? Apart from that, I did not study the OpenAPI issue. My plan for now and the foreseeable future is just to write some Rust code and bring Coinnect to a stable version.

[crackcomm]

@hugues31 I am currently working only on Poloniex because I am still studying stream design in rust.

@ainestal I would like to see that too but currently for APIs like Poloniex etc. that do not provide OpenAPI descriptors I'd rather write code myself like @hugues31.

I will also implement async clients in orca but responses will be hard typed.
I am using protocol buffers on every step for storage and remote capabilities.

This data is mostly streamed to Python where it can be trained on by TensorFlow.
Currently data is deserialized but rust-numpy implementation is a possible future.

[ainestal]

@crackcomm I didn't take a close look at the OpenAPI but the code generation feels very useful for the future. In the case of exchanges that don't implement it is probably easier to create the description of the API than to code it in rust.

@hugues31 I agree with the approach of coding it now, since we will get the value straight away. I like that over having to wait until we have the code generation and then we can describe all the API, which will take a long time before is completed and usable.

[ainestal]

Hey @hugues31 who is the adventure with the code going? Let me know if I can help. For now I'm waiting until you are done to then help migrating whatever is left (I guess at least Bitstamp)

[hugues31]

Hello @ainestal ! I'm ready to begin the implementation for one exchange just to see what it will looks like. I suggest to take Poloniex as an example since it may interest more people than the others.
I will create a new branch and you are very welcome to submit your PR. A lot of code need to be changed before the first successful compilation though.
EDIT: In fact, we need to change all exchanges at once... And I'm facing several issues like sharing the same client across the API as we do now (and not recreate it each time we make a request), returning error with error_chain, and so on... Not an easy task

[ainestal]

This is the kind of changes that is way easier to do by pairing, but that's kind of complicated in our case.
@hugues31 you can get rid of the rest of exchanges while working with Poloniex. We will include the rest afterwards when the library is compiling again.

If you see that is very difficult to make it work with all the constraints we had try removing some of them (errors, recreation of the client, etc), we can re introduce them later.

[hugues31]

You can check the new "async" branch.
I just write down the depencies and start converting the code but there is a lot to do. Feel free to PR on this branch. I don't have a lot of time ahead of me for now..

[ainestal]

I'll take a look when I can, currently I don't have a lot of spare time.

[ainestal]

@hugues31 I was yesterday taking a look at the code and thinking on how to do it. My main assumption is that "We want to deliver futures to the users of the library". Is this correct?

If this is true I will work on the code trying to change the least possible parts in order to make it work.

[hugues31]

@ainestal Yes it should be the point. I'm not sure if including the tokio_core inside the API is a good idea thought

[ainestal]

@hugues31 I'm trying to implement just the return of Futures from ticker, orderbook, add_order, and balances, staying at the generic_api layer, without going any deeper to the api of the exchanges.
I'm trying to keep it simple just returning a Future instead of a Result. For what I understand if we include the tokio_core in the api then we will just return a Result, not a Future. That defeats the point of using futures. If we do that just to get asynchronicity then we could use threads and not suffer the complexity of the Futures.

The part that is making me really struggle to implement this are the errors. I don't know how to pass the error chain to the future.
I've been taking a look at rust-lang-deprecated/error-chain#90 and https://github.com/sezaru/futures-error-chain but those all are workarounds.

So currently I don't know how to implement it. I'm tempted to try to get rid of the error_chain and see if I can make the library work without it, but feels like going a step backwards. What are your views on this?