Make requests concurrently #13
Conversation
`to_tuple()` was using unwrap instead of handling the error, e.g. return an `Err(ApiError)` in this case. AFAICT `to_tuple()` is only used on date strings coming from the API so it should be fine, but it still makes sense to handle potentiall parsing errors. NOTE: The `?` operator implicitly converts the given value into the return error type via `From`/`Into`. That's what the `#[from]` attribute is doing: it's implementing the `From` trait to convert a `chrono::ParseError` into an `ApiError`.
Both branches push into the `ranges` `Vec`.
By using the `?` operator to bubble up `chrono::ParseError`. This is now possible because we convert those into `ApiError`.
This is a better name that conveys what it does and it also avoid potential confusion with the standard `parse()` method that uses the `FromStr` trait.
This is a common pattern in crates that return `Result` with a given error. For example `std::io::Result` is a type alias to `std::result::Result<T, std::io::Error>`.
The logic to get make the HTTP request, handle errors, deserialise the response was common. I've factored out this into its own generic function.
This will be more useful in the logic that unwrap the tokio tasks results and gradually peal off the various layers of `Result`.
When requesting data for different date ranges make these requests concurrently instead of making them sequentially. Broadly speaking this is what's happening: - spawn a Tokio task for each of these requests. - each task will return a `Result<Vec<IntensityForDate>>` - wait for them to return using `try_join_all()` - this returns a `Result<Vec<Result<Vec<IntensityForDate>>>>` - if it's an error (e.g. some of the task panicked) we return it using `?` - we then have a `Vec<Result<Vec<IntensityForDate>>>`. One element for each of the requests' results. - we "invert" that and turn it into `Result<Vec<Vec<IntensityForDate>>>` - either we return the first error or unwrap the list of tuples - we now have a `Vec<Vec<IntensityForDate>>`, one element per response and each element is a `Vec<IntensityForDate>` (the list of tuples with date/intensity) - we finally flatten this into a simpler `Vec<IntensityForDate>` This is a bit more convoluted than I'd like but I hope it makes sense. Some of the types in the variable declarations are there mainly for readability sake given the levels of nesting but some of them are there to help Rust figure out how to `collect()`. **NOTE**: I'm still not 100% happy with this and I do wonder if the unwrapping/error handling could be done more elegantly - possible. Also, potentially there may be a way of doing this in a single bigger step, I may experiment with it further to see if it turns out to be a bit more readable.
| @@ -30,6 +30,8 @@ pub enum ApiError { | |||
| Error(String), | |||
| } | |||
|
|
|||
| pub type Result<T> = std::result::Result<T, ApiError>; | |||
There was a problem hiding this comment.
One reason why I introduced this was because writing down the types when massaging the return values from the tokio tasks was getting a bit out of control and error prone.
it the number of requests needed is large, we might flood the API (unlikely though). Maybe later on we could limit the number of threads used.
separating them would be extra work for now but maybe in the future let's aim to have separate PRs to make it easier to review |
|
we are now getting "The date range you have specified is greater than 14 days. Please select a smaller date range." but are getting the expected values with the main branch |
simplified pipeling that - unwraps/bubble up tokio `JoinError` - converts list of tasks Results into a single Result of tuples - flatten the tuples (Vec of Vecs)
This reverts commit 111d09f. It turns out the logic inside the if/else blocks was **not** the same. This was a bug :)
Its-a me a bug :)! I had a look and tried go back in time slowly and it turns out that this commit introduced a bug: 111d09f - I've reverted it and it now seems to work. PS: Wow, it's amazing how much of a time difference there is between making these requests concurrently and sequentially :) |
|
implements #7 |
yes! 10s vs 177s |
|
Fab, thanks @xoen |
This is related to Issue #7.
Spawn a Tokio task per request instead of making them sequentially.
Details about how the data flows back from the green threads into the format we wants are given in the commit message here - hopefully makes sense.
As mentioned in that commit message I may experiment with the result unwrapping further to see if I can get to something even simpler/more readable (you should have seen the original version of this 😅)
I've made some tweaks/refactorings. Explaination is given in the individual commit messages.
If you really prefer I can split the refactorings commits into its own PR and then have a PR only for the concurrent change.