Use iteration instead of recursion for connect
#291
Conversation
There was a problem hiding this comment.
Thanks for this! I like the general concept. A couple of quick thoughts:
The main thing we want is for the latest Response in a chain at any given point to have a history of the redirects that led to it. Right now, each response gets a cloned Vec. It might be sufficient for each Response to take the history Vec from the previous response and add one to it. That said, in the process of typing that out I realized it would make the relationship between one Request and the next a little more complicated, since we'd have to mutate the previous Request. So let's not do that.
We still have the Hist iterator type, but I don't think we need that anymore. That was just a hack to do iteration over a chain of Arcs. Now that we have a Vec, we can use Vec's iterator. We don't even strictly speaking need an iterator - but since Vec::iter() returns an ExactSizeIterator, using .last() is a perfectly efficient way to get the last item.
I'm not totally sold on the Retry concept, because it combines two somewhat unrelated purposes - retrying due to broken connection, and following a redirect. What do you think of leaving "retry due to broken connection" as a recursive operation (since it should never go more than one deep for any given Unit), and treating redirects as a more traditional loop?
For instance, we could have a connect_inner that returns Result<Response, Transport>, and connect would have a loop that calls connect_inner, and if the Response is a 3xx, it performs the rI don't totally like edirect logic and repeats the loop.
There was a problem hiding this comment.
Right now, each response gets a cloned Vec.
The vec is not cloned. This is the reason I moved previous out of do_from_request, so I would be able to access it directly even in case of an IO error.
What do you think of leaving "retry due to broken connection" as a recursive operation (since it should never go more than one deep for any given Unit)
Why will it never go more than one deep? I don't see any state tracking that.
Assuming it never goes more than one deep, I agree separating the two makes sense.
For instance, we could have a connect_inner that returns Result<Response, Transport>, and connect would have a loop that calls connect_inner, and if the Response is a 3xx, it performs the redirect logic and repeats the loop.
As in, move the 300 handling out of connect_inner? Yeah, that sounds a lot simpler.
There was a problem hiding this comment.
Why will it never go more than one deep? I don't see any state tracking that.
A client SHOULD NOT automatically retry a failed automatic retry.
This is implemented by the use_pooled parameter to connect. When we first arrive, it's true. If we do an automatic retry, it's false on the second call. We also gate both retry paths on is_recycled, which can only be true if we asked for a pooled connection and got one. This is definitely a bit convoluted and spread over more locations than it should be. We should clarify the code.
Yes, I think so - it just needs to be set before returning from |
This does require getting rid of the `Response`s, but those wouldn't be terribly useful anyway.
- Remove unnecessary `Hist` wrapper - Document the order requests are stored - Renamed `previous` to `history`
... and get rid of the confusing `Retry` abstraction.
|
I'm pretty happy with this :) let me know when/if I should squash the commits. |
There was a problem hiding this comment.
Yeah, this is looking really nice! I think this is ready to go. No need to squash the commits. When I go to merge, I'll use GitHub's "squash and merge" function.
I'll come back and give it a second look with fresh eyes in the morning before hitting the button.
|
Wow. This is quite a PR. Thanks! I'll go through it later today too. |
- Rename previous -> history - Reduce indentation and make the loop feel less like a `goto` - Don't give responses that were never redirected a history of 1
resp.history was always empty, because it's only set by `connect`
Previously, they would succeed no matter what, because the `history` didn't actually depend on what `connect` did. Or in other words, it tested the `Display` impl, not the application logic.
Look at this beautiful waste of time!
Closes #290.