-
-
Notifications
You must be signed in to change notification settings - Fork 497
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for excon idempotent retries #356
Conversation
@myronmarston - great questions. I need to document this stuff better, so apologies there. I think you are basically hitting nails on heads.
I'll review the code more explicitly presently to see if there is anything else. |
@@ -201,7 +191,14 @@ def call(chunk, remaining_bytes, total_bytes) | |||
# | |||
# @private | |||
def read_body_from(response_params) | |||
@chunks.join('') | |||
if @chunks.none? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have repro cases? I don't think this should be happening, so would be good to just fix it if we need to (or at least understand why you are seeing this behavior).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad you asked about this. It felt like a hack and I have no idea what's going on that causes this branch to get hit..but I have a test that fails w/o it. The test case that requires this is here:
it 'properly records and plays back the response for unexpected status' do |
It's using a response_block to stream and also uses the :expects
option but is getting back a 404 instead of a 200.
The easiest way to repro this if you want to troubleshoot it is to clone VCR, bundle install, change this line to if false
(so the branch is never taken) and then run:
$ rspec spec/vcr/library_hooks/excon_spec.rb -e "unexpected status"
...and it'll run the one spec that fails without this branch.
As you say, it would be great to understand why this is needed and/or change Excon so it's not.
Otherwise looking pretty good I think. I wonder if it would be better for middlewares if there were relative things to use. ie something like |
I'd be in favor of this. Other middleware systems have features like this (e.g. Faraday). One gotcha I ran into: I initially put my middleware first and |
That's the version where Excon::Middleware::ResponseParser was introduced.
This causes problems when the response is written back to disk (e.g. when `:new_episodes` is used).
I'll try to dig in to that error a bit more, but don't think I can get to it until Friday or later. Yeah, I hadn't gotten far enough along to really dig into interface for middleware. Just got them working and then got into the middle of a lot of stuff. Better to design from knowing what is needed anyway, rather than just guessing. Issue around that here: excon/excon#324 (I welcome your thoughts/comments). Interesting point on responseparser. I can certainly see what you mean. I'll have to give it some consideration. Issue for this here: excon/excon#325 |
No worries, that sounds great. I may go ahead and merge and release with this before you get around to it -- we can always update it later, and mostly I just want to understand what's going on that requires that hack. One other gotcha I ran into: I found that I had a recorded response body that was mysteriously being cleared out on later playback and I couldn't figure out why. Took me a while to track down but I eventually found the source here: It would be really nice if that did not mutate the response body string provided by a middleware as that causes surprising/hard-to-track down bugs. I know that Excon is really focused on good perf, but I think that code path is only ever hit when a middleware sets the response body, as under normal usage |
Yeah, I think you describe that accurately. It should almost assuredly dup (or at least not slice!). Would you be up for a quick patch to fix that up? (dup would be fine given that as you said, I don't think that path is likely to be encountered often). Thanks! |
|
Fix for excon idempotent retries
@geemus -- I ran into a case where VCR wasn't recording responses properly due to the ordering semantics of Excon middlewares stack. I've found a solution by splitting VCR's middleware into two pieces that are injected into the middleware stack at separate spots. I wanted to run this by you and confirm that some assumptions I'm making here are intended public behaviors of Excon and not private implementation details that you may change. Here's how I'm injecting my middleware:
request_call
method called last in the chain before the request is sent out over the wire? VCR needs to intercept the request just before it is made, after all other middlewares have had a chance to do whatever they want with the request.Excon::Middleware::ResponseParser
and then insert my response middleware after it?