Fix for excon idempotent retries

[myronmarston]

@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:

        middlewares = ::Excon.defaults[:middlewares]

        middlewares << VCR::Middleware::Excon::Request
        response_parser_index = middlewares.index(::Excon::Middleware::ResponseParser)
        middlewares.insert(response_parser_index + 1, VCR::Middleware::Excon::Response)

• Is it correct to assume that middlewares appended on the end of the list will always get its 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.
• Given that VCR needs access to the response as early as possible, before other middlewares have had a chance to mutate it (or raise errors based on it), is it best to search for the index of Excon::Middleware::ResponseParser and then insert my response middleware after it?

[geemus]

@myronmarston - great questions. I need to document this stuff better, so apologies there. I think you are basically hitting nails on heads.

• I expect that in many cases it is desirable/required to create more than one middleware to better control timing.
• [request/response/error]_call are all pushed through the stack from front to back. So the last thing in the chain should certainly be the last thing to receive any/all calls (including request_call).
• Inserting your middleware after response parser sounds pretty reasonable given your requirements. I would love any ideas you might have about making it easier to manage/order middlewares, but what you have done seems reasonable in the interim.

I'll review the code more explicitly presently to see if there is anything else.

[geemus]

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).

[myronmarston]

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:

vcr/spec/support/shared_example_groups/excon.rb

Line 21 in acc66dc

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.

[geemus]

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 after(ResponseParser, VCRMiddleware) or something. Then there could be a defined way to resolve if multiple people call that (probably FIFO). That way multiple folks wouldn't accidentally stomp on each other by all fighting to be the first after response, for instance.

[myronmarston]

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 after(ResponseParser, VCRMiddleware) or something. Then there could be a defined way to resolve if multiple people call that (probably FIFO). That way multiple folks wouldn't accidentally stomp on each other by all fighting to be the first after response, for instance.

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 params[:response] was nil, and I wasn't sure why. It seems counterintuitive that reading the socket and putting the response into the params has is a middleware as it seems like the core functionality of Excon. I had to dig into Excon's source to figure out that I have to put my middleware after your ResponseParser middleware.

[geemus]

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

[myronmarston]

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.

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:

https://github.com/geemus/excon/blob/517398504da9f95b0f70588c49b800b4a3464fb0/lib/excon/connection.rb#L202

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 datum[:response][:body] would be nil when a :response_block has been provided, right?

[geemus]

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!

[myronmarston]

Would you be up for a quick patch to fix that up?

excon/excon#326