Migration to 0.3 & Async/Await #582
Conversation
Note that we had to add a "Send" restriction on the error types of interledger-packet to work around "the trait `std::marker::Send` is not implemented for `(dyn std::error::Error + 'static)`"
There was a problem hiding this comment.
I quite like the look of this so far. In my head the idea was first to have a massive PR transitioning from 0.1 to 0.3, and then have a second massive PR moving to async/await, just to split the effort into more manageable chunks even if it meant more effort overall. But from what you've got here I'm thinking that you've got the right idea by just switching immediately to async/await, since it seems like we can avoid a whole lot of frustration from things that got changed since we're using so much less of the futures crate by simply getting rid of all the combinators via await.
| @@ -10,8 +10,13 @@ repository = "https://github.com/interledger-rs/interledger-rs" | |||
| [dependencies] | |||
| bytes = { version = "0.4.12", default-features = false } | |||
| byteorder = { version = "1.3.2", default-features = false } | |||
| futures = { version = "0.1.29", default-features = false } | |||
| futures = { version = "0.3", default-features = false } | |||
There was a problem hiding this comment.
futures is on 0.3.1 by now, might as well update directly to that.
| account_ids: Vec<Uuid>, | ||
| ) -> Box<dyn Future<Item = Vec<Self::Account>, Error = ()> + Send>; | ||
| ) -> Result<Vec<Self::Account>, ()>; |
There was a problem hiding this comment.
I'd love to begin thinking about returning actual error types rather than just () everywhere, which would hopefully get rid of a ton of explicit error!() calls, and likewise remove a ton of map_err and the associated closures (which could go even further toward improving compile time). It doesn't need to be in this PR, but it's something to look forward to (and there's no reason we wouldn't want to do so, right?).
| where | ||
| A: Account, | ||
| B: IntoFuture<Item = Fulfill, Error = Reject>, | ||
| <B as futures::future::IntoFuture>::Future: std::marker::Send + 'static, | ||
| F: FnMut(IncomingRequest<A>) -> B, |
There was a problem hiding this comment.
The simplification here is delicious.
| B: IntoFuture<Item = Fulfill, Error = Reject>, | ||
| <B as futures::future::IntoFuture>::Future: std::marker::Send + 'static, | ||
| F: FnMut(IncomingRequest<A>) -> B, | ||
| F: FnMut(IncomingRequest<A>) -> IlpResult + Send, |
There was a problem hiding this comment.
Nit, but to make the association easier to visually parse I would write bounds like this as:
F: Send + FnMut(IncomingRequest<A>) -> IlpResult,
| ilp_address: Address, | ||
| ) -> Box<dyn Future<Item = (), Error = ()> + Send>; | ||
| ) -> Box<dyn Future<Output = Result<(), ()>> + Send>; |
There was a problem hiding this comment.
Why not use async_trait on this trait rather than returning an explicitly boxed future?
There was a problem hiding this comment.
Ended up doing it on a separate branch https://github.com/interledger-rs/interledger-rs/blob/gakonst/futures03-http/crates/interledger-service/src/lib.rs#L324
| #[cfg(feature = "trace")] | ||
| pub use trace::*; | ||
|
|
||
| pub type IlpResult = Result<Fulfill, Reject>; |
| "Unable to parse ILDCP response from fulfill packet: {:?}", | ||
| err | ||
| ); | ||
| })?; |
There was a problem hiding this comment.
As I mention elsewhere, I'd love to introduce proper error return types so that we would no longer have to deal with so much wasted space due to map_errs, like those in this function. Doesn't need to be part of this effort, but async/await makes our code so much cleaner that instances like these really really stand out.
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Why add these tests as part of this PR?
Write locks should not be in the same scope as await'ed futures. We fix this by scoping the write lock for as long as needed, and then yield only the result of the computation. When the scope is over, the lock gets dropped, so we can safely use the future.
WIP. Need to fix the Server (warp does not support tungstenite yet), and the Client's add_connection method is commented out still
async fn -> IlpResult desugars to fn -> Future<Output = IlpResult
Note: exchange rate service does not compile on 1.39, but compiles on nightly. Why?
As title. Still WIP. Opening for initial feedback. This will be split in smaller PRs and squashed/split into more verbose commits.
BoxedIlpFutureand replace it everywhere withIlpResult(which is really aResult<Fulfill, Reject>.'async_traitas a lifetime.I am considering getting this PR to a merge-able state, and then open a separate issue to port the whole crate to Bytes 0.5.
Will fix #85.