Support async requests

[jontze]

I recently found this crate and absolutely love it. Thanks for the amazing project and keep up the good work! :+1:

During usage I wonder why the provided functions aren't async as the data must be fetched from somewhere. After looking into the code base I found that we currently use blocking requests.

At first it seems like an easy implementation for me, since we just have to migrate the usage of the blocking reqwest client to a non blocking and transform some traits and functions to async. What maybe could be implemented with a optional feature flag. However, after trying to prototype this on my own fork (https://github.com/jontze/geocoding/tree/feat/async-fetching) I came to the conclusion that it is not that easy :laughing: . Since it looks like that there have to be done some refactor of at least the Point struct in the geo-types crate to support rust futures.

Furthermore, a feature flag is definitely not the cleanest solution to implement this since it will lead to some code duplication, I guess. On the other side transforming the whole crate to async will be a major breaking change for the user...

However, before continuing on that, are there any plans for the future to support async data fetching instead of blocking?

[urschrei]

Could you provide some more detail as to why (and how) you think the Point struct needs to be modified to support futures? I don't do much async programming, so it was my understanding that simple structs (esp. those that impl Copy, Clone, Send, and Sync) can be used without modification.

[lnicola]

I imagine because Float doesn't require Send and Point is generic in the coordinate type. But under most executors, futures don't work very well with non-Send types.

[lnicola]

To comment on the implementation, a blocking feature (enabled by default) with an always enabled async API might be more nices (following reqwest, for example).

[jontze]

I'm not 100% sure what have to be done at the Point struct as I'm atm kind of stuck figuring out what the error message exactly means that is caused by passing Point as parameter to an async function (e.g. here: https://github.com/jontze/geocoding/blob/9480d0ad44307b7644ebab63d44faf61970f4012/src/openstreetmap.rs#L312).

future cannot be sent between threads safely

future created by async block is not `Send`

note: required for the cast to the object type `dyn std::future::Future<Output = std::result::Result<std::string::String, GeocodingError>> + std::marker::Send`rustc
openstreetmap.rs(312, 29): has type `&geo_types::Point<T>` which is not `Send`, because `geo_types::Point<T>` is not `Sync`
openstreetmap.rs(312, 81): future created by async block is not `Send`

[jontze]

Yes it looks like it is basically what @lnicola wrote. The num_traits::float::Float trait doesn't implement Send + Sync. But it is kind of weird that I can pass the Pointstruct to a normal async function as parameter without any compiler errors. But if I do the same in an async trait the above described error appears.

However, as this only is a problem for async traits I for now oped-in into Non-threadsafe futures ... Can't wait until async traits are stable

After resolving this for now, I followed @lnicola suggestion and structured it like the reqwest crate.

src
|___blocking // Code with blocking api requests
       |___geoadmin.rs
       |___opencage.rs
       |___openstreetmap.rs
|___async_impl // Code with async api requests
       |___geoadmin.rs
       |___opencage.rs
       |___openstreetmap.rs
|___lib.rs
|___geoadmin.rs // shared structs and functions e.g. params, ...
|___opencage.rs // shared
|___openstreetmap.rs //shared

So we have two features blocking and async. The former is a default feature and always enabled and the latter is optional and can be enabled by passing asyncto the list of features.

The lib.rs will still re-export the blocking API as default, so for the user the import path doesn't change, if he want to use the the blocking api.

// lib.rs
#[cfg(feature = "blocking")]
pub use crate::blocking::openstreetmap::Openstreetmap;

The async tests are already passing as well. So we are on a good track. I moved a lot of code around and have some failing doc-tests atm. So I have to tackle this as next. Furthermore the async api need some examples and documentation as well.

One last drawback in my opinion is that we have some duplicated code now in the async_impl and blocking module, as the configuring of query parameters and processing of the response stays the same, only doing the requests to the provider api changed. So further refactoring could be done by outsourcing the duplicated code into own functions that then could be used in the async_impl and blocking module.

So the next steps are:

• [ ] Fix and adjust docs in blockingmodule
• [ ] Add docs and examples to async_impl module
• [ ] Refactor to reduce duplicated code in async_impl and blocking
• [ ] Investigate how to continue with Non-threadsafe futures as mentioned above (need help here)

Do you have any suggestions or thoughts on that and how to continue? :smile: If you want to look into the code, you can find the changes and the latest state in the feat/async-fetching branch of my my fork

[lnicola]

But it is kind of weird that I can pass the Pointstruct to a normal async function as parameter without any compiler errors.

Does that still work across an await? Can you access the parameter afterwards?

What if you patch it to add a Send bound to Point?

Do you have any suggestions or thoughts on that and how to continue?

File a PR? :smile:

[fakeshadow]

I come across this issue this was my take on this problem:

1. All json response should bound to DeserializeOwned trait. You can't achieve zero copy deserialize when the reqwest response live only in the scope of forward and reverse method.
2. Don't ues async_trait macro. Use lazy future trait object in Forward::forwad and Reserve::reserve. Most of the references needed from input type are only used to prepare a http request and they are forced to live longer than they should be.
3. Some minor lifetime fix.
4. async feature should default to Send because reqwest itself construct thread safe Future and nothing in this crate go against the way of it too.

This is my fork that builds with async reqwest: https://github.com/fakeshadow/geocoding I don't know this crate well and I have no intention to make a PR out of this. I just hope this can help people do want to make the effort of implement async feature into this crate.

[jontze]

Thanks a lot, @fakeshadow ! I will look into this and give it another try!

[ttytm]

@fakeshadow thanks for the effort. To have a little bit of an easier time working into this, is it possible to update the test to the changes?

[jrandolf]

We've forked this and published this as a new crate: https://github.com/jun-sheaf/geocoding-async

If you'd like, we can send a PR, but the PR will be as is.