sasa1977 / con_cache

ets based key/value cache with row level isolated writes and ttl support
MIT License
910 stars 71 forks source link

Is there an easy way to flush the cache between tests? #11

Closed myronmarston closed 9 years ago

myronmarston commented 9 years ago

First off, thanks for the great library. It's working exactly as advertised.

One issue I'm running into is how to manage ConCache in my tests. I was having order-dependent failures until I realized that the cached values were persisting between tests. I looked for a ConCache.flush (or similar) function but could not fine one. For now I am manually calling ConCache.delete for each key that has been cached. This is pretty cumbersome and brittle so a function to simplify this would be much appreciated.

sasa1977 commented 9 years ago

Hi, sorry for the late response, I was away on a small holiday.

Currently there's no supported clear function. Introducing it is not quite easy, since it would require a more involved locking logic, which may affect performance. Thus I'm currently not inclined to support this feature, unless there's some real need in production for it.

Iteratively cleaning the table (which is what you seem to be doing) of course isn't consistent. The problem is that this operation is not exclusive, so any other process could add new entries while we're clearing the table. After clearing, there's still a chance the table won't be empty. However, if this approach works for your tests, you could extract it to a helper function and call it before every test, for example via setup.

Another option is to use different cache instances for different tests. In fact, I use something like this in con_cache tests. However, if you're creating the cache from some supervisor as the part of your supervision tree, this might not work. In this case, the solution could be to restart that supervisor process for every test. That would cause the cache process to be restarted, and you'd get a fresh cache. A variant of this is to stop the entire OTP application and start it again.

Yet another option is to get raw ets table via ConCache.ets/1 and then use :ets.delete_all_objects/1 to clear the table. This may work (I didn't try it), but it's not consistent, since cleanup process will still hold references to existing elements, try to clean them, and even invoke your callback functions.

If none of these options work for you, please create a simplified mix project that demonstrates your problem, and I'll look into the it and see what can be done.

myronmarston commented 9 years ago

Thanks for the detailed response!

Currently there's no supported clear function. Introducing it is not quite easy, since it would require a more involved locking logic, which may affect performance. Thus I'm currently not inclined to support this feature, unless there's some real need in production for it.

Makes sense. I'm new to erlang/elixir so I didn't even think about this.

However, if this approach works for your tests, you could extract it to a helper function and call it before every test, for example via setup.

Yep, that's what I'm doing now. But as you say, it's not consistent and it's cumbersome...plus it's too easy to forget to delete a particular key.

Another option is to use different cache instances for different tests. In fact, I use something like this in con_cache tests. However, if you're creating the cache from some supervisor as the part of your supervision tree, this might not work. In this case, the solution could be to restart that supervisor process for every test. That would cause the cache process to be restarted, and you'd get a fresh cache. A variant of this is to stop the entire OTP application and start it again.

This looks like a good approach but I couldn't figure out how to apply it to my case. It looks like you can't use a named ConCache process with that approach, right? My tests are simulating HTTP requests (using phoenix/plug) and thus don't have a good way to "inject" a different cache instance for each test since it's not invoking my code via a normal function call but rather via a simulated get /some/path call.

Yet another option is to get raw ets table via ConCache.ets/1 and then use :ets.delete_all_objects/1 to clear the table. This may work (I didn't try it), but it's not consistent, since cleanup process will still hold references to existing elements, try to clean them, and even invoke your callback functions.

This works and is the route I'm going to take for now.

That said, it feels a bit like I'm relying upon private APIs here (the fact that ConCache uses ETS internally is a private implementation detail, right?) and it has the caveats you mentioned, so I'd prefer an alternate solution (such as using a separate cache instance per test as you suggested above).

If none of these options work for you, please create a simplified mix project that demonstrates your problem, and I'll look into the it and see what can be done.

I paired my project down to a minimal example and put it up here:

https://github.com/myronmarston/con_cache_example

The tests are here:

https://github.com/myronmarston/con_cache_example/blob/master/test/unit/controllers/host_check_controller_test.exs

(Note that those tests are approaching a tautology because I stripped out so much of the logic. Hopefully that doesn't distract from the underlying issue).

Any suggestions?

sasa1977 commented 9 years ago

Yep, that's what I'm doing now. But as you say, it's not consistent and it's cumbersome...plus it's too easy to forget to delete a particular key.

From what I can tell, you're manually listing each key to delete. You could use :ets to fetch all keys, then iterate through them, and delete each entry. This is a one-off which you can just put in the setup:

setup do
  :host_results
  |> ConCache.ets
  |> :ets.tab2list
  |> Enum.each(fn({key, _}) -> ConCache.delete(:host_results, key) end)

  :ok
end

Or even extract to some helper function under your tests, and invoke it from every setup.

This looks like a good approach but I couldn't figure out how to apply it to my case. It looks like you can't use a named ConCache process with that approach, right? My tests are simulating HTTP requests (using phoenix/plug) and thus don't have a good way to "inject" a different cache instance for each test since it's not invoking my code via a normal function call but rather via a simulated get /some/path call.

That's why I mentioned the options of either restarting the supervisor which started the ConCache, or the entire application. This will cause ConCache to restart, and consequently, the previous cache contents is lost.

In your case, the cache resides in the top-level supervisor, so those two options are essentially the same. The following fix works for me:

setup do
  Application.stop(:pirosshki)
  Application.start(:pirosshki)
  :ok
end

This is not perfect if your initialization code is more involved.

A more fine-grained solution is to use the supervisor to stop, and restart the ConCache process only:

setup do
  Supervisor.terminate_child(Pirosshki.Supervisor, ConCache)
  Supervisor.restart_child(Pirosshki.Supervisor, ConCache)
  :ok
end

At this point, this solution looks like the best option to me.

the fact that ConCache uses ETS internally is a private implementation detail, right?

Not really, it's documented, and there's a function available to fetch the ETS handle. Also, I don't have plans to support multiple backends. So ConCache is not a "black-box" abstraction that tries to hide its underlying storage.

That said, you should be careful when using naked ETS for writes. However, if you want to do some more involved reads, there are various useful functions in :ets module, and by all means use them where applicable.

Let me know if this helps.

myronmarston commented 9 years ago

A more fine-grained solution is to use the supervisor to stop, and restart the ConCache process only:

Thanks, this worked perfectly for me. I hadn't yet read up on enough on how supervisors work to figure that out.

Is it worth adding a "Managing ConCache in Tests" section to the README that describes the various approaches we've discussed here?

sasa1977 commented 9 years ago

It could be beneficial. Perhaps a good place to put it is between "Process alias" and "Inner workings" section. Would you be willing to make a PR? :-)

myronmarston commented 9 years ago

Would you be willing to make a PR? :-)

Honestly, I'm still learning the right Elixir/Erlang/OTP terminology for things so the docs would probably be a lot clearer if you wrote them up. If you'd still like me to do I'll see what I can do.

sasa1977 commented 9 years ago

That’s fine, I’ll do it then :-)

On 29 Jun 2015, at 22:36, Myron Marston notifications@github.com wrote:

Would you be willing to make a PR? :-)

Honestly, I'm still learning the right Elixir/Erlang/OTP terminology for things so the docs would probably be a lot clearer if you wrote them up. If you'd still like me to do I'll see what I can do.

— Reply to this email directly or view it on GitHub https://github.com/sasa1977/con_cache/issues/11#issuecomment-116837383.

myronmarston commented 9 years ago

Thanks!