Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multilevel Cache: transaction/3 is attempting to change all levels multiple times. #35

Closed
sdost opened this issue Dec 5, 2018 · 3 comments

Comments

@sdost
Copy link
Contributor

sdost commented Dec 5, 2018

Environment

Elixir version (elixir -v): Elixir 1.7.4 (compiled with Erlang/OTP 21)
Nebulex version (mix deps): 1.0.0
Operating system: Debian Stretch

Expected behavior

Operations wrapped in a transaction should occur once.

test "check cache set and get in transaction" do
    value = ["old value"]

    Cache.set(:test, value)

    Cache.transaction(
      fn ->
        old_value = Cache.get(:test)

        assert old_value == value

        Cache.set(:test, ["new value" | old_value])
      end,
      keys: [:test]
    )

    assert Cache.get(:test) == ["new_value", "old_value"]
  end

Actual behavior

1) test check cache set and get in transaction (Cache.MultilevelTest)
      test/cache/multilevel_test.exs:14
      Assertion with == failed
      code:  assert old_value == value
      left:  ["new value", "old value"]
      right: ["old value"]
      stacktrace:
        test/cache/multilevel_test.exs:23: anonymous fn/0 in Cache.MultilevelTest."test check cache set and get in transaction"/1
        (nebulex_redis_adapter) lib/nebulex_redis_adapter.ex:118: NebulexRedisAdapter.do_transaction/5
        (nebulex) lib/nebulex/adapters/multilevel.ex:366: anonymous fn/4 in Nebulex.Adapters.Multilevel.eval/3
        (elixir) lib/enum.ex:1925: Enum."-reduce/3-lists^foldl/2-0-"/3
        (db) lib/cache.ex:2: Cache.with_hooks/3
        test/cache/multilevel_test.exs:19: (test)

More information

  ## Transaction

  @impl true
  def transaction(cache, fun, opts) do
    eval(cache, :transaction, [fun, opts], [])
  end

  ## Helpers

  defp eval(ml_cache, fun, args, opts) do
    eval(levels(opts, ml_cache), fun, args)
  end

  defp eval([l1 | next], fun, args) do
    Enum.reduce(next, apply(l1.__adapter__, fun, [l1 | args]), fn cache, acc ->
      ^acc = apply(cache.__adapter__, fun, [cache | args])
    end)
  end

The above code seems to try to run the transaction operation for all cache layers, but since each other operation (:get, :set, etc) also goes through this code path, all the changes will have occurred for all layers after running the function inside the transaction once. It looks like the transaction lock should be acquired for all layers, then the wrapped function should run once, then the transaction lock should be released for all layers.

@cabol
Copy link
Owner

cabol commented Dec 6, 2018

Yes, the current implementation is very naive, it should be like you describe. I will try to fix it as soon as possible. Thanks :)

@cabol
Copy link
Owner

cabol commented Jul 29, 2019

I've added a fix for this transactions issue, it was like an overall improvement for all built-in adapters (especially for the multilevel one). So please try it out again and let me know if it works for you. I stay tuned for your feedback!

@cabol
Copy link
Owner

cabol commented Jul 30, 2019

In the meantime, I'll close this issue, but feel free to re-open it if you find the problem persists or in case of any error, etc.

@cabol cabol closed this as completed Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants