Decouple from HTTP client library #173

ddeboer · 2015-02-20T20:54:07Z

Fix #53.

Goals:

Completely decouple our caching proxy clients from any HTTP client implementation.
Remove dependency on Guzzle (or any other) HTTP client.
Depend only on PSR-7 and the generic php-http/adapter instead.

Todo:

update docs to reflect changes
fix missing host test
Switch RTD default to stable, so users won't be confused by our updated (2.0) docs

dbu · 2015-02-21T09:14:25Z

doc/cache-invalidator.rst

@@ -195,6 +195,9 @@ implements ``\Psr\Log\LoggerInterface``. For instance, when using Monolog_::
 Then add the logger as a subscriber to the cache invalidator::

    use FOS\HttpCache\EventListener\LogSubscriber;
+    use FOS\HttpCache\CacheInvalidator;
+
+    $invalidator = new CacheInvalidator(


dbu · 2015-02-21T09:17:41Z

interesting. kind of sounds like the client abstraction should be a PSR that builds on top of PSR-7... but i guess for now we have to live with that.

i am not so sure if this is still a 1.x thing or we should bump to 2.0 for this. looks like a couple of protected methods severly change behaviour.

ddeboer · 2015-02-21T09:24:40Z

Yeah, I tried to limit BC issues by falling back to our current Guzzle 3 implementation, but it’s undoable to have no breaks at all. I’m okay with postponing this to 2.0, but would like to finish this PR up shortly to get it over and done with. I’m against opening a 2.0 branch as we decided to limit parallel branches as much as possible. Do you agree with changing the branch alias to 2.0 in this PR? I think we have some things that we should finish in 1.0 first, so we could hold off merging this for a little while.

dbu · 2015-02-21T14:46:56Z

i agree to bump the version to 2 in this and make 1.3 the last 1.x version of the bundle. we should withhold from tagging 2.0.0 for a bit after merging this however, to give it time to settle and give us some time for more bc breaks if needed.

about the whole http client abstraction layer: i wonder if it would be worth to move that to its own repository. then again, that will explode the complexity of the abstraction, as now we only need to provide the minimum we need to operate.

stof · 2015-02-21T15:01:33Z

There is already several such libraries: https://packagist.org/search/?q=http-adapter (and probably others with different names)

And given that PSR-7 is close, I think the best solution is probably to expect client with a single method ->request(PSR7Request $request): PSR7Response.
I don't know whether there is already an HTTP client library based on PSR-7 (either on its own or bridging existing libraries)

dbu · 2015-02-21T15:11:04Z

thanks @stof, indeed a lot of choice. the egolen / ivory one looks quite good at first glance. is that a good choice, or do you have any recommendations which one to pick?

and any idea how risky it is to implement PSR7 right now? if the interface changes before its finally accepted, we would become incompatible. and do you mean to write a wrapper around that wrapper? ivory seems to provide a sendRequest method that accepts a PSR request object...

dbu · 2015-02-22T09:41:27Z

https://github.com/saxulum/saxulum-http-client-interface another suggestion that came through twitter

ddeboer · 2015-02-22T20:16:46Z

Yes, let’s use one of the already existing HTTP adapters; it makes no sense to reinvent the wheel. I like ivory-http-adapter as it’s compatible with PSR-7 and has parallel requests, which we need.

We do require some extra functionality on top of pure HTTP messages:

calculating the message signature
replicating the invalidation requests over all proxy instances, which I do in a RequestQueue.

I guess I could make InvalidationRequest extend PSR’s RequestInterface to offer this functionality.

dbu · 2015-02-23T12:39:16Z

that sounds like a good plan to me.

ddeboer · 2015-04-06T20:34:21Z

Okay, switched to the egloen/http-adapter for HTTP library abstraction. Unfortunately, that lib relies on phly/http for its PSR-7 implementation, and phly/http does not allow PURGE as a HTTP method (which makes sense in a way, as PURGE is not proper HTTP, but still). I’m still looking for a way to work around this.

dbu · 2015-04-07T07:12:31Z

looks good!

voting for PSR-7 went into another iteration (though about documentation rather than code as far as i understood). i think we best not release any 2.0 version before it is final and we can rely on the official interfaces. lets hope this will happen within the next month or two.

ddeboer · 2015-04-14T19:34:17Z

Yes, let’s wait for PSR-7. While working on this PR, egeloen/http-adapter has changed its API multiple times. I hope that with PSR-7 the dust will settle and both HTTP adapters and message library (phly/http) become stable enough to use them here.

ddeboer · 2015-05-03T13:06:59Z

Update: ivory-http-adapter has been split into multiple packages and moved to a separate organisation: php-http.

ddeboer · 2015-05-26T10:25:06Z

composer.json

+    "repositories": [
+        {
+            "type": "vcs",
+            "url": "https://github.com/ddeboer/guzzle6-adapter"


Should be removed when the adapter has been merged.

ddeboer · 2015-05-26T20:12:17Z

src/ProxyClient/Request/RequestQueue.php

+
+    public function getAllowedSchemes()
+    {
+        return array('http');


As this is different for Nginx and Varnish, this should probably be moved back into the Varnish and Nginx client classes.

yeah lets make this method abstract here.

ah sorry. no, lets move it to the clients.

Just wondering: does getAllowedSchemes() actually make sense? Nginx and Symfony already allow both HTTP and HTTPS. What about Varnish? If you have a SSL terminator in front of Varnish (such as Nginx or Pound), couldn’t you also send invalidation requests to https://your-app.com (I may be mistaken here)? So shouldn’t we always allow both HTTP and HTTPS?

And related to this, should we perhaps remove the scheme guessing (from getDefaultScheme()) when setting a baseUrl as well?

It’s more explicit than what we have now.

Consequence: users need to always set http://fos.lo instead of fos.lo (http://fos.lo:123 instead of fos.lo:123). But the latter isn’t a valid URI anyway, so it’s weird to be able to pass it to a method called setBaseUrl(). We can see our confusion in the method’s docblock: Set application hostname, optionally including a base URL. Therefore removing the scheme guessing also removes this confusion.

The reason we did add the guessing was convenience for users, if I recall correctly. This may still be the case for proxy servers (e.g. Varnish at 127.0.0.1:456), but at least for the base URIs I think the convenience is outweighed by the confusion.

On the other hand, when sending invalidation requests, it is not the baseUrl’s scheme that is used, but that of the configured $servers.

yeah, lets just make protocol required and not guess neither validate it. most likely varnish invalidation should go directly to varnish and not via the ssl terminator, but if somebody really needs that i don't see why we should forbid it.

dbu · 2015-06-15T08:29:47Z

CHANGELOG.md

+2.0.0 (unreleased)
+------------------
+
+* Decoupled from the Guzzle HTTP client.


we could say what we use instead.

dbu · 2015-06-15T08:52:26Z

read through all of it again and added quite a few comments - sorry about that. but its mostly details or things not directly related to the PR. i propose to move most of them into issues so we can merge this soon - its getting big this PR.

ddeboer · 2015-06-15T20:39:29Z

@dbu No problem. On the contrary, thanks for the thorough review. 😄

ddeboer · 2015-06-16T20:34:04Z

src/ProxyClient/AbstractProxyClient.php

+            if ($response->getStatusCode() >= 400
+                && $response->getStatusCode() < 600
+            ) {
+                $exceptions[] = ProxyResponseException::proxyResponse($response);


Now throwing exceptions again for responses with error status codes. Unfortunately, I see no easy way to get the corresponding request. Although Guzzle should return responses in the same order as the requests, things get difficult when the responses are mixed with exceptions (for timeout errors).

well thats already a lot better.

dbu · 2015-06-17T07:35:12Z

there was a test failure.

apart from that i am fine with squash and merge if you want. won't be much online the next days so go ahead if you want.

Remove custom adapters Update to PSR-7, Guzzle 6 and php-http/adapter Remove PHP 5.3 from build matrix Adopt changes in http-adapter and use discovery Remove dependencies on Guzzle (components) We use Guzzle 6 for testing, so it needs to stay in require-dev. Use php-http/message-decorator Change branch alias to 2.0.x Add docs Allow failures on hhvm Fix connection timing issues Remove all Guzzle-specifics from the tests, too Change documentation links to stable Try to run HHVM on Guzzle 5 Remove Bash file Add version notice to docs Fix setPurgeLocation docs Don't mock exceptions * It’s easier not to * Mocking exceptions causes HHVM to fail with "Call to a member function mockery_getName() on a non-object (NULL)" Extract createUri() method Remove allow_failures for HHVM Add response code to cache hit/miss assertion message Throw exceptions for error status codes

Decouple from HTTP client library

vidarl · 2016-10-19T10:12:00Z

I realize this PR was merged quite some time ago, but I have a question

i propose we add the sleep (lets see if 100ms is also reliably enough) and open an issue at the varnish project to ask them if that is expected behaviour.

Did you open an issue on varnish project regarding this?

I experienced the same problem with v1.4 and came up with the following workaround in varnish.vcl instead of using a sleep in my tests:

sub vcl_deliver {
    (...)
    // This is the foshttpcache way of doing it, but it is not bulletproof.
    // Seems like for about 50000 usec after a purge, X-Varnish may also contain br_req ID and we'll falsely have a HIT flag
//      if (resp.http.X-Varnish ~ " ") {
//          set resp.http.X-Cache = "HIT";
//      } else {
//          set resp.http.X-Cache = "MISS";
//      }

    if (obj.hits > 0) {
      set resp.http.X-Cache = "HIT";
    } else {
      set resp.http.X-Cache = "MISS";
    }

I anyway don' know why you don't use obj.hits in order to detect a HIT or not as virtually everybody else do that, AFAIK

dbu · 2016-10-20T06:31:00Z

the obj.hits worked well with varnish 3. varnish 4 seems not to set obj.hits reliable anymore, see for example: https://coderwall.com/p/zksgmq/vanish-4-effectively-detect-hit-miss-in-vcl

if this was ammended in later versions of varnish 4, do you have some source about that? if we can avoid the sleep in favor of something more reliable, we would prefer that as well.

vidarl · 2016-10-25T06:45:28Z

I am using varnish 4.0.3 and have never experienced any issues with obj.hits.
Might be different story with 4.1.3 then?

ddeboer · 2016-10-25T08:43:05Z

The change in obj.hits was introduced with Varnish 4.0’s split between frontend and backend threads. This may have been fixed in 4.0.3 however.

That would allow us to revert the changes we made in our VCLs (and docs) for 4.0. @vidarl Do you have time to experiment with changing the VCLs and running the FOSHttpCache tests to see if you get the same results for hits/misses?

ddeboer force-pushed the http-client branch 3 times, most recently from 0da2378 to 0739cc4 Compare February 20, 2015 21:01

dbu reviewed Feb 21, 2015
View reviewed changes

ddeboer added the v2 label Feb 21, 2015

ddeboer mentioned this pull request Feb 26, 2015

Warning during composer install/update about guzzle/http being abandonned #176

Closed

dbu mentioned this pull request Mar 13, 2015

Fix Guzzle requirement #177

Merged

ddeboer force-pushed the http-client branch 2 times, most recently from 9a1f1dd to d87dd9d Compare April 6, 2015 20:32

ddeboer mentioned this pull request Apr 29, 2015

[RFC] Support for guzzlehttp/guzzle? #189

Closed

ddeboer force-pushed the http-client branch from d87dd9d to e3b7702 Compare May 26, 2015 10:19

ddeboer reviewed May 26, 2015
View reviewed changes

ddeboer force-pushed the http-client branch from e3b7702 to fb83897 Compare May 26, 2015 10:52

ddeboer reviewed May 26, 2015
View reviewed changes

dbu reviewed Jun 15, 2015
View reviewed changes

CHANGELOG.md

2.0.0 (unreleased)

------------------

* Decoupled from the Guzzle HTTP client.

Copy link

Contributor

dbu Jun 15, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could say what we use instead.

ddeboer force-pushed the http-client branch from b3066d1 to 3d7a957 Compare June 15, 2015 20:02

ddeboer reviewed Jun 16, 2015
View reviewed changes

ddeboer force-pushed the http-client branch from b0078c9 to 6a3f996 Compare June 19, 2015 17:30

dbu added a commit that referenced this pull request Jun 19, 2015

Merge pull request #173 from FriendsOfSymfony/http-client

c023e9c

Decouple from HTTP client library

dbu merged commit c023e9c into master Jun 19, 2015

dbu deleted the http-client branch June 19, 2015 18:49

ddeboer mentioned this pull request Nov 1, 2016

Simplify hits counting #326

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple from HTTP client library #173

Decouple from HTTP client library #173

ddeboer commented Feb 20, 2015

dbu Feb 21, 2015

dbu commented Feb 21, 2015

ddeboer commented Feb 21, 2015

dbu commented Feb 21, 2015

stof commented Feb 21, 2015

dbu commented Feb 21, 2015

dbu commented Feb 22, 2015

ddeboer commented Feb 22, 2015

dbu commented Feb 23, 2015

ddeboer commented Apr 6, 2015

dbu commented Apr 7, 2015

ddeboer commented Apr 14, 2015

ddeboer commented May 3, 2015

ddeboer May 26, 2015

sagikazarmark Jun 2, 2015

ddeboer May 26, 2015

dbu May 30, 2015

dbu May 30, 2015

ddeboer Jun 6, 2015

ddeboer Jun 6, 2015

ddeboer Jun 6, 2015

dbu Jun 7, 2015

dbu Jun 15, 2015

dbu commented Jun 15, 2015

ddeboer commented Jun 15, 2015

ddeboer Jun 16, 2015

dbu Jun 17, 2015

dbu commented Jun 17, 2015

vidarl commented Oct 19, 2016 •

edited

dbu commented Oct 20, 2016

vidarl commented Oct 25, 2016 •

edited

ddeboer commented Oct 25, 2016 •

edited

Decouple from HTTP client library #173

Decouple from HTTP client library #173

Conversation

ddeboer commented Feb 20, 2015

Choose a reason for hiding this comment

dbu commented Feb 21, 2015

ddeboer commented Feb 21, 2015

dbu commented Feb 21, 2015

stof commented Feb 21, 2015

dbu commented Feb 21, 2015

dbu commented Feb 22, 2015

ddeboer commented Feb 22, 2015

dbu commented Feb 23, 2015

ddeboer commented Apr 6, 2015

dbu commented Apr 7, 2015

ddeboer commented Apr 14, 2015

ddeboer commented May 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Jun 15, 2015

ddeboer commented Jun 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Jun 17, 2015

vidarl commented Oct 19, 2016 • edited

dbu commented Oct 20, 2016

vidarl commented Oct 25, 2016 • edited

ddeboer commented Oct 25, 2016 • edited

vidarl commented Oct 19, 2016 •

edited

vidarl commented Oct 25, 2016 •

edited

ddeboer commented Oct 25, 2016 •

edited