Revert Caching (#177) (fixes #239)

[tarcieri]

Reverts support for client-side caching using rack-cache, added here:

#177

There have been ongoing maintenance problems with this code
(e.g. fd04847) and it's also implicated in
breakages and performance problems on JRuby 9000:

#239

[tarcieri]

Heads up @pezra. We've been having a lot of problems with this code lately and we're not sure it's actually worth keeping, at least in its current form. I was a fan of Resourceful and excited to see this land, but as someone who's not using it and only dealing with the maintenance I'm not sure it's worth keeping.

I think it might make more sense as a separate gem, provided you can get some decent hooks into http.rb to do so.

[tarcieri]

This might also warrant a version bump if we land it (0.9.0?) I know there was some other stuff on the roadmap for that milestone, but it seems like a big enough change to warrant a minor version change.

[zanker]

LGTM, agree on bumping to 0.9.0 due to breaking changes.

[tarcieri]

Confirmed #239 is fixed: build is now passing on JRuby 9000 /cc @headius

[pezra]

I must say I'm a little disappointed to have it reverted wholesale without even a chance to try to salvage it. I think the internal design of http.rb makes it pretty much impossible to implement caching as a gem without monkey patching way more than is advisable.

Fwiw, an http client without caching is pretty much useless to me. I guess resourceful is still an option. 😐

[ixti]

@pezra PR is not yet merged. The only way of salvation I see is refactoring the whole thing to get rid of rack-cache. I agree that current internals are not perfect to have cache support completely as a separate gem, but it's possible to find a way to make it transparent and lightweight.

[ixti]

@tarcieri @zanker I think we can move exisitng 0.9 issues to 0.10, and release cache-less version as 0.9.

[pezra]

@ixti, is rack cache the main problem or is it the issues described in #239? I think removing the dependency on rack is doable. (Probably by extracting the storage bits of rake-cache into a separate gem.) The things in #239 all look worth solving and i'd be happy to work on them. (But of course my time is somewhat constrained).

I will think about how we might enable an external caching gem.

[ixti]

@pezra the problem is complex. :D rack-cache dependency is one bit of it. I understand time is not elastic. And I was also hoping to get my hands on improvements of cache layer - but absolutely out of time (summer time - kids are not in school). And sometimes it worth destroy legacy to build something new. In other words http.rb will have cache support again some time later :D

[zanker]

To step back a little, what do you need innate caching support for @pezra? It seems like something you could implement on top of http.rb without much issue. Even if it's just a wrapper around it, and it'd be a lot simpler.

[tarcieri]

I must say I'm a little disappointed to have it reverted wholesale without even a chance to try to salvage it.

@pezra well, this is the scorched earth option. We can explore others.

But like @zanker I wonder if this sort of functionality could be layered on top of http.rb rather than having to be inside of it.

[tarcieri]

So it looks like JRuby 9000 is out:

http://blog.jruby.org/2015/07/jruby_9000/

I'm going to land this for now and release it as 0.9.0.pre

@pezra if you're interested in fixing things up so they work on JRuby 9000 again, we can revert the "revert" and bring caching back.

[tarcieri]

I just deployed this in a JRuby 9000 app today and everything is looking good.

Unless there's any objections, I'd like to cut a 0.9.0 release tomorrow with JRuby 9000 support.

[tarcieri]

@pezra happy to circle back with you and address the issues so we can get this back in. Perhaps we can find some better hooks so you don't need to use delegate.rb for this.

[pezra]

OK. Some extension mechanisms and an external gem seem to be the best approach since the http.rb community is not interested in maintaining caching functionality. I'll take a look at what it would take to make that happen as time permits. For now i'll just lock all my projects to 0.8. 😞

[pezra]

@zanker, @tarcieri, I don't see how it can be implemented as pure wrapper. Any header can affect the cache lookup (see Vary header) so the cache needs to have the request with all the headers that will be put on the wire.

Also, we don't want to bypass the cache when following redirections.

[tarcieri]

@pezra we'd like to add some (optional) hooks to the request/response lifecycle. See #209

[pezra]

@tarcieri, that looks like it might be able support an add-on cache system. i'll continue this discussion on that issue.

[zanker]

@pezra Sorry, I'm still not quite sure what you mean. Why wouldn't:

class CachedHttpRb
  def initialize(httprb)
    @httprb = httprb
  end

  def get(path)
    if @response && @response.headers['Expires'] > Time.now
      return @response
    end

    @response = @httprb.get(path)
  end
end

As a rough example?

[pezra]

@zanker, consider the following scenario:

A rails app that

1. receives requests with a bearer token authorization header
2. creates a new httprb based on that bearer token and wraps in a simple cacher
3. makes requests against other servers on behalf of the user
4. ...

Step 3 almost certainly returns a cached response intended for an earlier user. This is only a problem if the cache store is shared between requests but the cache would be pretty unhelpful it that were not the case.

This same sort of issue applies to Accept, Cookie, Prefer and any number of other header fields, all of which would likely be set below the caching wrapper.

[jrochkind]

Hmm, I'm super interested in this feature, sad to find it was here but left along time ago.

I wonder if I can reintroduce it as third-party code, does http have any kind of plug-in middleware architecture that might let me add it back?

[tarcieri]

@jrochkind #482 added something which may be applicable /cc @paul

[jrochkind]

Not sure if the "observers" (at present?) will let something interupt a request and substitute a response without the client ever going out to the network?

I also see that #223 is still open... not sure if you mean it to be. :)

(I think there are a variety of ways to work around the "earlier user" problem, including but not limited to servers actually using HTTP caching headers like Vary appropriately, and caching with keys including user_id if necessary. Plus a bunch more, the right one may depend on the application and it's usage patterns. I think caching would be really sweet, but I don't know if it's http.rb's responsibility to provide it or provide hooks so someone else can provide it, but one of the two would be great. Hmm, I think faraday maybe supports caching, I guess http.rb wrapped in faraday might work... in general I'm not a big faraday fan).

[paul]

@jrochkind I think what you're looking for is #482. It needs a small change to available_features (https://github.com/httprb/http/blob/master/lib/http/options.rb#L25) to allow additional features to be inserted, but it should be possible to add an HTTP cache as a "Feature" like this. (well, maybe. Now that I look at it, I guess you can't halt the request and substitute a response from a cache without some additional changes to the API. Maybe Client#build_request can check if any of the features returned a Response instead of a Request, and just use that...)

[pezra]

@paul, or a Feature could, optionally, wrap the connection too.

[tarcieri]

@paul definitely curious about ways we could extend the feature to further control the request flow