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

Already on GitHub? Sign in to your account

Issues with multi support #321

Open
joerixaop opened this Issue Jan 22, 2013 · 5 comments

Comments

Projects
None yet
2 participants
Collaborator

joerixaop commented Jan 22, 2013

There are a number of issues with the way the multi method behaves currently.

  • It does not check the return value for quiet operations. This is an issue because quiet operations still return "interesting" results, in the case of setq, addq and replaceq those are the failure cases.
  • It does not work correctly with multithreading.

E.g. of failure without threads

# memcached on local host only accepts values up to 1MB, this makes
# for a reliable error case, other error cases are of course possible in 
# practice, but not as reproducible
memc = Dalli::Client.new(["localhost"], value_max_bytes: 2*1024*1024)
result = memc.multi do memc.set("test", "a" * (2*1024*1024 - 200)); end
p result # true
result = begin memc.get("test"); rescue => e; result = [:error, e]; end
p result # [:error, #<Dalli::DalliError: Response error 3: Value too large>]
result = memc.get("test")
p result # nil # This is the response of the previous get
result = memc.set("test", "a"*10)
p result # nil # This is the response of the previous get
result = memc.get("test")
p result # true  # this is the response of the previous set
result = memc.get("test")
p result # "aaaaaaaaaa"  # This is the response of the previous get
result = memc.set("testeken", "a" * 10)
p result # "\x04\bI\"\x0Faaaaaaaaaa\x06:\x06ET"

As you can see the request/response cycle gets completely out of sync, with no way to recover.

Basically what needs to happen is that when a quiet operation is followed by a non-quiet operation that the response must be checked of the quiet operation first. Also at the end of a multi method a noop operation should be sent, so errors can be thrown (note that it must not stop at the first error, it needs to read all the responses, otherwise the cycle will still be out of sync).

I don't think this can work with the current threading behavior though. If a connection is shared between multiple threads there is no way to make sure that the exception is shown in the right thread.

Collaborator

joerixaop commented Jan 22, 2013

Ah, reading a bit more on the memcached binary protocol: the opaque field will be copied back into the response, I guess this can be used to keep things working in multi-threaded mode.

Collaborator

mperham commented Jan 28, 2013

Yeah, the API does not account for failure cases and it will require a lot of work to get it working. For thread safety, we'd need to lock the entire ring during the multi operation. Pipelining is simple when there is only one remote server but a huge pain when there's N remote servers and you don't know which operations are going to which server in advance.

Collaborator

mperham commented May 27, 2013

@joerixaop Is this issue still relevant? Please close if not.

Collaborator

joerixaop commented May 27, 2013

I believe it's still relevant. TBH I was working on a patch, but it wasn't fully compatible with the other changes I made to dalli (that you merged yesterday).

I'm going to revisit my patch-in-progress, see if my approach still makes sense and finish it (at the moment one of the problems with it is that it's only error detecting and not yet any recovery attempts.

I've pushed what I had to the add_opaque branch in the xaop fork: https://github.com/xaop/dalli/tree/add_opaque

Collaborator

mperham commented May 27, 2013

Cool, I just gave you committer access since you've helped a lot with get_multi recently. Feel free to work in your own feature branch in this repo if you'd like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment