Skip to content
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 curl reset #248

Merged
merged 3 commits into from May 30, 2018
Merged

Fix curl reset #248

merged 3 commits into from May 30, 2018

Conversation

moufmouf
Copy link
Contributor

@moufmouf moufmouf commented Apr 13, 2018

Context

When using PHP-VCR with Guzzle 6 with async requests, the first request is playing correctly but the next request is putting Guzzle in an infinite loop.

It took me a whole day but I traced back this problem to the way the "curl_reset" hook is written. It clears the request but not the response. Hence, when another request is performed (even on a different URL) the same response is sent. This weird behaviour is causing an infinite loop in Guzzle 6.

This problem is probably exactly the same as the issue #211. This PR should fix it.

What has been done

The first commit of this PR contains only the failing test (to showcase the problem). You see that the test never completes (because of the infinite loop): https://travis-ci.org/php-vcr/php-vcr/jobs/366140696

The second commit is the fix. I simply added one line in the curl_reset hook to remove the response as well as the request.

Note: this PR is based on the branch from PR #246 since I need unit tests from Guzzle 6 that are not yet merged.

@moufmouf
Copy link
Contributor Author

Ping @renatomefi .

This PR is solving actual problems of a number of PHP-VCR users (actually all the users doing more than one async request with Guzzle 6).
I know open-source is often done on our free time (mine included!), but if you have some time to review this, that would be really super handy for a lot of us!

Thanks!

@Daniel15
Copy link

Daniel15 commented May 7, 2018

Thanks for working on this!

@monobook
Copy link

@renatomefi ping-pong :)

@jeremyharris
Copy link

jeremyharris commented May 28, 2018

I would be awesome if this could be merged, as it fixes the infinite loop problem described in #155

edit: There's still one issue I'm having, but I can't track it down because the libraries are quite complex. I am using the AWS S3 adapter for Flysystem (file abstraction library). The adapter uses the S3 SDK which uses Guzzle to make requests. For some reason, this works when I write a file (which makes 3 requests) but if I immediately try to read (which should be an additional 2 requests) VCR fails to write the new requests. I'll continue looking into it and try and report back.

@renatomefi renatomefi merged commit 13872a0 into php-vcr:master May 30, 2018
@renatomefi
Copy link
Member

Thanks so much @moufmouf

@moufmouf
Copy link
Contributor Author

You're welcome!

@moufmouf
Copy link
Contributor Author

@jeremyharris if you are still having the issue, could you try to reproduce it with a simple unit test? (like this one: https://github.com/php-vcr/php-vcr/blob/13872a0b291c1a398461ac178850837648998271/tests/integration/guzzle/6/test/AsyncTest.php )
That would help a lot.

@jeremyharris
Copy link

@moufmouf The test case passes. I even split requests into two separate clients/requests and the tests still passed. This PR definitely fixes multiple async requests. My tests that use a single Flysystem call to S3 work amazing after this PR 🎉

The remaining issue I'm having is multiple Flysystem calls to S3 fail to write the requests/responses following the first call. For the time being I split the calls to separate tests to generate the Cassettes (each which have multiple requests).

The work you've done here allows me to test those multi-calls once per testcase, so thanks so much for that. If I have time to dig into my mess of dependencies to learn what it's doing differently, I might be able to create a simplified failing test case for you to look at. I don't think it would be fair to send you a highly abstracted failing test case that uses multiple libraries for you to debug 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants