search

rtomayko / rack-cache

Real HTTP Caching for Ruby Web Apps
http://rtomayko.github.io/rack-cache/
Other
822 stars 126 forks source link

Warn once on X-Content-Digest entity store miss #158

Closed dehzhas closed 5 years ago

dehzhas commented 5 years ago

Resolves issue https://github.com/rtomayko/rack-cache/issues/155 by warning once per class if purge isn't implemented.

A few notes:

  1. I could not figure out a satisfactory way to truly warn on initialization in a backward compatible manner.

    • The purge method itself has thrown NotImplementedError for a long time. It's really just the lookup method that needs resolution.
    • There is no way to know at load time what implementation/configuration is actually going to be used. The only real way to do understand is catch the NotImplementedError in the lookup. Anything more seems to imply some other architectural changes that would require more than a patch level version update so I went with this for now.

  2. I went with a simple warn call since that seems to be used elsewhere in the code.

  3. I'm not very familiar with MiniTest and googling around I wasn't able to figure out how to effectively ensure that warn was called exactly once per class. I left it at calling the method 2x and visually checking the warning only prints once. This makes me feel kinda icky though. I'm open to suggestions.

  4. The warn_once is not technically thread safe in the sense that there is a race condition where multiple threads could print a warning message if they check the key at the same time. I figured on occasional double warning is ok in this case rather than the overhead of dealing with a mutex.

  5. I went with the simple warn_once(key) and building the message in a block rather than passing it to the method for performance reasons. Normally I'd consider this premature optimization, but since this is a performance boosting gem I thought paying attention here would be good. Let me know if you want to switch to just using warn_once(message).

dehzhas commented 5 years ago

@grosser I have made the recommended changes. Tests pass locally, but looks like Travis-CI is unable get the ruby environment setup properly to run the automated tests.

grosser commented 5 years ago

bumping in another PR worked, so I'd assume this is good to go too

grosser commented 5 years ago

1.9.0