Caching #177

pezra · 2015-02-03T23:55:49Z

Implementation of HTTP caching using rake-cache's persistance layer. Based on @Asmod4n's initial pass (https://github.com/Asmod4n/http/compare/caching) but much has changed. :) Addresses issue #77.

ixti · 2015-02-04T00:16:49Z

Huge! Some cleanups and refactorings and would be good to go. :D

tarcieri · 2015-02-04T00:20:59Z

Rakefile

@@ -23,7 +23,7 @@ end
 require "yardstick/rake/verify"
 Yardstick::Rake::Verify.new do |verify|
  verify.require_exact_threshold = false
-  verify.threshold = 58
+  verify.threshold = 58.1


does this not support >= 50 or something like that?

It is. And verify.threshold = 58.1 in conjunction with verify.require_exact_threshold = false (above) means >= 58.1. We bump it up to try to enforce ourselves to "keep moving forward" to the 100% API documentation coverage :D

tarcieri · 2015-02-04T00:21:36Z

Yeah, I'll second huge... looks interesting though.

pezra · 2015-02-04T01:16:01Z

It is pretty big but mostly self contained. Hopefully that will make it easier to understand. I'm fixing many of the RuboCop atm. (Pretty sure it hates me as much as i despise it.:) ) Those issues are why the build is broken (the tests already pass) so it should go green with that commit.

I'll integrate any feed back asap so comment away.

Asmod4n · 2015-02-04T05:04:46Z

\o/ I would replace the locking InMemoryStore with a https://github.com/ruby-concurrency/thread_safe collection, and maybe constantize the most use Strings for a performance boost.

Thanks for taking this over :)

pezra · 2015-02-04T05:25:33Z

@Asmod4n, no problem. Thanks for getting it started.

ixti · 2015-02-04T13:40:08Z

.rubocop.yml

@@ -59,3 +59,6 @@ Style/StringLiterals:

 Style/TrivialAccessors:
  Enabled: false
+
+Style/AlignParameters:
+  Enabled: false


Please do not disable this cop.

Done. I think this rule encourages harder to read code but it is your house. :)

It encourages consistency of alignment 😜

ixti · 2015-02-04T13:45:01Z

@pezra Can you please rename:

HTTP::Cache::RequestWithCacheBehavior -> HTTP::Request::Cached
HTTP::Cache::ResponseWithCacheBehavior -> HTTP::Response::Cached

HTTP::Cache::CacheControl -> HTTP::Cache::Control

Also, IMO CacheControl should be named Cache::Headers actually and should be a decorator/facade of HTTP::Headers instead, thus initializer will accept headers structure directly. I'll try to provide my proposal as commit. :))

pezra · 2015-03-04T18:56:41Z

@tarcieri, thanks for reviewing!

I change the file loading behavior so that rack, etc is not required until caching is enabled. For example, HTTP.with_cache(metastore: ...) causes the caching code, rack-cache, etc to be loaded. That deviates a little from your suggestion but seems to accomplish the goal while being a little easier on user.

tarcieri · 2015-03-04T19:02:14Z

Seems good, thanks! Will wait to hear what @ixti says

ixti · 2015-03-09T00:13:25Z

Gemfile

@@ -26,6 +26,7 @@ group :test do
  gem "rspec-its"
  gem "rubocop"
  gem "yardstick"
+  gem "fakefs", :require => "fakefs/safe"


Seems like not used (and thus useless) dependency

It was left over from a failed approach to testing the caching stuff. Removed.

ixti · 2015-03-09T00:28:35Z

Please, move let statements in specs to the top of blocks they belong to.

pezra · 2015-03-09T14:49:30Z

Please, move let statements in specs to the top of blocks they belong to.

Where you think they belong. :) I prefer to put the important part of the tests front and center and leave the less interesting support structures below the fold.

It's your code base, though, so i'll do that.

ixti · 2015-03-09T15:05:26Z

@pezra I mean following:

describe :a do
  let(:b) { 1 }
  describe :c do
    let(:d) { 2 }
  end
end

:b belongs to block :a.
:c belongs to :a (declared inside :a) so it has access to :b as well.
:d belongs to block :c.

I just prefer to read specs from top to the bottom. And it's a bit inconvenient to me to scroll down to understand "initial" data. :D

ixti · 2015-03-09T15:07:15Z

In other words. let goes first things in the block. Then goes subject if it is needed. Then before/after hooks. Then it examples. Then nested context/describes.

pezra · 2015-03-09T15:31:39Z

I just prefer to read specs from top to the bottom.

Of course. However, if a spec's purpose cannot be understood without referring to the let definitions i consider that a naming failure. Such failures are, imo, better fixed by finding highly indicative names rather than rearranging things so that the glossary comes first.

pezra · 2015-03-09T15:34:38Z

@ixti, i think all the issues you noted are sorted out now.

ixti · 2015-03-09T22:12:18Z

lib/http/request.rb

@@ -80,6 +82,8 @@ def initialize(verb, uri, headers = {}, proxy = {}, body = nil, version = "1.1")

      @headers["Host"]        ||= default_host
      @headers["User-Agent"]  ||= USER_AGENT
+      now = Time.now
+      @headers["Date"] = now.httpdate


Clients SHOULD only send a Date header field in messages that include an entity-body, as in the case of the PUT and POST requests, and even then it is optional. A client without a clock MUST NOT send a Date header field in a request.

The HTTP-date sent in a Date header SHOULD NOT represent a date and time subsequent to the generation of the message. It SHOULD represent the best available approximation of the date and time of message generation, unless the implementation has no means of generating a reasonably accurate date and time. In theory, the date ought to represent the moment just before the entity is generated. In practice, the date can be generated at any time during the message origination without affecting its semantic value.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.18

So, looks like this should go to Request::Caching instead. And also we MUST not override headers if they were explicitly given by user.

Actually I'm not sure if this still needed?

Good catch.

ixti · 2015-03-16T00:39:54Z

I have merged PR into master. Thanks!

pezra · 2015-03-16T01:49:53Z

Yay! Thanks @ixti.

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

Revert Caching (#177) (fixes #239)

ixti and others added 12 commits February 1, 2015 20:35

Remove obsolete BodyDelegator

7f749a9

Bump up YARDStick threshold

0900813

HTTP Caching

0b4c444

Typo

4310b9e

Small fixups

2533e01

Fix a couple of specs and remove some rubocop warnings

e7f753b

basic specs for caching

c4f1e45

extract some caching related helper objects and write specs

75d8068

improve cache options

b3499bd

remove dead code

153fe3b

remove unused argument to make rubocop happy

69a343d

remove debug puts

40fb8b6

tarcieri reviewed Feb 4, 2015
View reviewed changes

pezra added 4 commits February 3, 2015 20:04

remove many rubocop issues

bbfd8c3

swap find for detect to appease rubocop

2a57ce3

improve factoring of cache perform method

31a60f6

further factoring improvements to cache perform

08df766

pezra added 2 commits February 3, 2015 22:17

Convert docs to yard

f2346bd

blank lines break builds, apparently

4c10ea8

ixti reviewed Feb 4, 2015
View reviewed changes

ixti and others added 4 commits February 4, 2015 14:48

Enable align parameters cop back again

bcf947e

Rename *WithCacheBehavior to *::Cached

69dfa0a

Refacor CacheControl helper. Renamed it to Cache::Headers

156ce9d

reenabled parameter alignment cop

0adc2a1

pezra added 3 commits March 4, 2015 10:18

Merge remote-tracking branch 'refs/remotes/upstream/master' into caching

9956494

delay loading rack-cache until caching is requested

182d5db

reduce length of options to appease rubocop

2d1f847

ixti reviewed Mar 9, 2015
View reviewed changes

pezra added 3 commits March 9, 2015 08:33

removed unused dependency

833deae

move require to top of file

88dc181

simplify spec

48f3e5c

improved caching response spec

eeeb2c8

bury the lede in some specs

86741e2

ixti reviewed Mar 9, 2015
View reviewed changes

remove unnecessary/incorrect date addition code in request

c234d49

ixti closed this Mar 16, 2015

ixti mentioned this pull request Mar 27, 2015

Caching support (was: Conditional Gets) #77

Closed

tarcieri mentioned this pull request Jul 22, 2015

Revert Caching (#177) (fixes #239) #240

Merged

tarcieri added a commit that referenced this pull request Jul 22, 2015

Merge pull request #240 from httprb/tarcieri/revert-caching

1dd3da2

Revert Caching (#177) (fixes #239)

tarcieri mentioned this pull request May 28, 2018

Support observers #478

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Caching #177

Caching #177

pezra commented Feb 3, 2015

ixti commented Feb 4, 2015

tarcieri Feb 4, 2015

ixti Feb 4, 2015

tarcieri commented Feb 4, 2015

pezra commented Feb 4, 2015

Asmod4n commented Feb 4, 2015

pezra commented Feb 4, 2015

ixti Feb 4, 2015

pezra Feb 4, 2015

ixti Feb 4, 2015

ixti commented Feb 4, 2015

pezra commented Mar 4, 2015

tarcieri commented Mar 4, 2015

ixti Mar 9, 2015

pezra Mar 9, 2015

ixti commented Mar 9, 2015

pezra commented Mar 9, 2015

ixti commented Mar 9, 2015

ixti commented Mar 9, 2015

pezra commented Mar 9, 2015

pezra commented Mar 9, 2015

ixti Mar 9, 2015

ixti Mar 9, 2015

pezra Mar 9, 2015

ixti commented Mar 16, 2015

pezra commented Mar 16, 2015

Caching #177

Caching #177

Conversation

pezra commented Feb 3, 2015

ixti commented Feb 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarcieri commented Feb 4, 2015

pezra commented Feb 4, 2015

Asmod4n commented Feb 4, 2015

pezra commented Feb 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixti commented Feb 4, 2015

pezra commented Mar 4, 2015

tarcieri commented Mar 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixti commented Mar 9, 2015

pezra commented Mar 9, 2015

ixti commented Mar 9, 2015

ixti commented Mar 9, 2015

pezra commented Mar 9, 2015

pezra commented Mar 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixti commented Mar 16, 2015

pezra commented Mar 16, 2015