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

pymemcache.exceptions.MemcacheUnknownCommandError: b'cas' #233

Closed
rtroyanov opened this issue Jun 7, 2019 · 14 comments
Closed

pymemcache.exceptions.MemcacheUnknownCommandError: b'cas' #233

rtroyanov opened this issue Jun 7, 2019 · 14 comments
Labels

Comments

@rtroyanov
Copy link

rtroyanov commented Jun 7, 2019

from pymemcache.client.base import Client
client = Client(('localhost', 11211))
while True:
result, cas = client.gets('key')
if result is None:
result = "1"
else:
result += 1
if client.cas('key', result, cas):
break

Traceback (most recent call last):
File "zzz1.py", line 32, in
set = client.cas(key, result, cas)
File "/usr/local/lib/python3.6/site-packages/pymemcache/client/base.py", line 437, in cas
return self._store_cmd(b'cas', {key: value}, expire, noreply, cas)[key]
File "/usr/local/lib/python3.6/site-packages/pymemcache/client/base.py", line 842, in _store_cmd
self._raise_errors(line, name)
File "/usr/local/lib/python3.6/site-packages/pymemcache/client/base.py", line 732, in _raise_errors
raise MemcacheUnknownCommandError(name)
pymemcache.exceptions.MemcacheUnknownCommandError: b'cas'

@Dansyuqri
Copy link

It seems that you have not defined the variable cas when you are calling the client.cas('key', result, cas) function. This results in the error. According to the documentation, it states that you will need characters from '0'-'9' in either string or int type.

However, you will then run into either of the following errors:

TypeError: can't concat int to bytes

# or 
TypeError: can't concat str to bytes

So just remember to prepend convert the character to bytes by simply prepending the character with a b like so: client.cas('key', result, b'0').

@jogo jogo closed this as completed Aug 22, 2019
@jogo
Copy link
Contributor

jogo commented Aug 22, 2019

thank you @Dansyuqri

@jamesoliverh
Copy link

jamesoliverh commented Feb 3, 2021

@Dansyuqri unfortunately, that is not the correct answer to this question.

I was getting the same error, and after much frustration figured out what causes it.

If the key 'key' does not already exist in the cache, the following fails with pymemcache.exceptions.MemcacheUnknownCommandError: b'cas':

val, cas = client.gets('key')
client.cas('key', 'val', cas)

I have no idea why this happens. It is not mentioned in the docs, and does not happen with other python libraries for memcached.

You need to ensure the key is already in the cache, possibly by setting it to None explicitly at some point. Then it does not fail. For example:

client.set('key', None)
val, cas = client.gets('key')
client.cas('key', 'val', cas)

Hope this might be useful to someone else.

If this is intended behaviour (seems like pretty weird behaviour and a very obscure error message for that to be the case), then I would suggest it is documented. Otherwise I would suggest this issue be reopened as a bug (@jogo).

@jparise
Copy link
Collaborator

jparise commented Feb 3, 2021

I'll reopen this for reinvestigation. I agree we'd at least want to update the documentation if this is intended behavior (I'm not sure if it is).

@jparise jparise reopened this Feb 3, 2021
@jparise jparise added the bug label Feb 3, 2021
@cgordon
Copy link
Collaborator

cgordon commented Feb 3, 2021

I tried out these commands locally and was able to reproduce the error:

>>> import pymemcache
>>> c = pymemcache.Client('localhost')
>>> val, cas = c.gets('key')
>>> c.cas('key', 'value', cas)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/csgordon/TempPy3Env/lib/python3.9/site-packages/pymemcache/client/base.py", line 520, in cas
    return self._store_cmd(b'cas', {key: value}, expire, noreply,
  File "/Users/csgordon/TempPy3Env/lib/python3.9/site-packages/pymemcache/client/base.py", line 979, in _store_cmd
    self._raise_errors(line, name)
  File "/Users/csgordon/TempPy3Env/lib/python3.9/site-packages/pymemcache/client/base.py", line 821, in _raise_errors
    raise MemcacheUnknownCommandError(name)
pymemcache.exceptions.MemcacheUnknownCommandError: b'cas'

That is with a freshly started memcached and no previous commands run via the client. The issue is that the value of the cas variable is None, and the code for _store_command therefore does not add the extra field required by the memcached cas command. That results in memcached rejecting the request as an ERROR.

There are two possible fixes. The first is that we have the cas method check that cas is not None and throw a MemcacheIllegalInputError if cas is None. The second is that we default cas to '1' if it is set to None. I think I'm fine with either, and I don't see a backwards compatibility issue here (since passing None right now results in an exception). Thoughts?

@cgordon
Copy link
Collaborator

cgordon commented Feb 3, 2021

Just to be clear, @Dansyuqri already explained this in his comment, I'm just providing some alternatives to get a better error message when users encounter this issue.

@jparise
Copy link
Collaborator

jparise commented Feb 4, 2021

The cas argument of the cas() function is currently documented as:

cas: int or str that only contains the characters '0'-'9'.

I think the right thing to do is to validate that parameter according to that input specification and raise MemcacheIllegalInputError if it fails to match.

Further, I see that gets() offers an optional cas_default argument:

default: value that will be returned if the key was not found.
cas_default: same behaviour as default argument.

That argues against the if cas is None: cas = '1' alternative approach because None is being emphasized here as the "no-CAS-by-default" sentinel.

Knowing all this, we can tweak @cgordon's example to become this if we want CAS behavior:

>>> val, cas = c.gets('key', default_cas='1')
>>> c.cas('key', 'value', cas)

... and without the default_cas=, we would get the more useful proposed MemcacheIllegalInputError.

@jamesoliverh
Copy link

jamesoliverh commented Feb 4, 2021

There are a few things I don’t quite get about this.

First, if this is indeed what @Dansyuqri meant in his answer, then it wasn’t particularly clear (at least to me). He said:

It seems like you have not defined the variable cas

However, the variable cas is defined, it is just defined as None.

Second, why does this happen in the first place anyway? I am not an expert on these matters, but I feel that a valid cas value should be returned from client.gets() to check against, even if the key does not (yet) exist. I appreciate that there is the cas_default option but remark that this is not available for ClientPool, apparently.

Third, the proposed solution of supplying b'0' as the cas value still does not produce the behaviour I would expect: in the case that the key has been set in the interim, client.cas() returns False and the key is not set (as one would expect), but in the case it has not been set in the interim, client.cas() returns None (as is documented), but the key is not set:

val, cas = client.gets('key')
ret = client.cas('key', 'val', b'0')
print(ret)
>>> None
print(mc.get('key'))
>>> None

It seems to me a valid use case to want to set a (perhaps previously nonexistent) key to a particular value with a single client.cas() call, only if it has not been updated (or set) in the interim. I may well be misguided in that, however.

@cgordon
Copy link
Collaborator

cgordon commented Feb 4, 2021

I made a dumb remark about using 1 as the default value for the cas argument, so let me start over, and I'll try to explain everything a little more in depth.

First, I agree with @jamesoliverh that the current behavior is not user-friendly (at best), but it isn't inconsistent with the way the memcache protocol works. In the protocol, if you send a cas command for a key that does not exist, memcache returns a NOT_FOUND response, which we translate to a return of None, None for the gets command. There is no well-defined cas value that represents a key which does not exist (at least, not one that is defined in the memcached protocol). In fact, there is no way that I can find to set the initial value of a key with cas. It looks like you are expected to use the add command instead.

The goal of the cas command is to allow race-free read-modify-write operations. That is why it was dumb for me to recommend using 1 as the default value of the cas argument. Imagine two clients (A and B) doing operations against a memcached instance that starts out empty.

Client A                                   Client B
--------                                   --------
valA, casA = gets('k')
                                                set('k', '10')
if (valA is None)
    valA = 0
else
    valA = valA + 1
cas('k', valA, casA)

In this example, client A is attempting to increment the value of 'k' by one, and it would like to avoid race conditions with other writers, so it is using the cas command. Client B, on the other hand, is just setting the value of 'k' to 10 directly, without using the cas command. Client A would like the cas command to fail in this case, since the value of 'k' changed after client A read it, and therefore it is incrementing an old value by one (usually this would be done in a loop, so that client A could re-attempt the increment, this time using the new base value of 10). This is typically called an "optimistic update", or an "optimistic lock" (which you probably already knew, but just for completeness).

So, if we returned a default value of 1 from client A's call to gets, then the cas done by client A would succeed, which is not what we want to happen. If we use our current logic, and return a cas of None from gets, then the call to cas will throw an exception, which is also not what we want. This, I think, is the reason we have this issue in the first place.

The correct idiom for doing what client A is trying to do in my example above is this:

while True:
    val, cas = client.gets(key)
    if cas is None:
        # In this case, the key does not already exist, so we need to add it
        ok = client.add(key, 1)  # using 1 as the "initial value" here
        # add returns false if a different client updated key before us (that is, between our call to gets, and our call to add).
        # Otherwise we are done and can exit the loop.
        if ok:
            break
    else:
        # In this case, we found an existing value for the key, and we want to update it 
        ok = client.cas(key, val + 1, cas)
        # If cas returned None, then someone else deleted the key already. If cas returned False, then someone else updated
        # the key already. In either case, we want to retry from the top of the loop. Otherwise, if cas returned True, we are
        # done and can exit the loop.
        if ok:
            break

This is a really long argument for what @jparise was already proposing, which is that we should just raise a MemcacheIllegalArgumentException from cas when someone passes None to as the cas argument.

@jamesoliverh If it isn't completely clear why the code snippet above is the correct way to use cas, please let me know and I'll try to explain more. It's entirely possible that I'm still completely wrong about how to use cas, but having re-read the documentation this morning, this seems like the intended approach.

@jamesoliverh
Copy link

First, I agree with @jamesoliverh that the current behavior is not user-friendly (at best), but it isn't inconsistent with the way the memcache protocol works. In the protocol, if you send a cas command for a key that does not exist, memcache returns a NOT_FOUND response, which we translate to a return of None, None for the gets command. There is no well-defined cas value that represents a key which does not exist (at least, not one that is defined in the memcached protocol). In fact, there is no way that I can find to set the initial value of a key with cas. It looks like you are expected to use the add command instead.

@cgordon I also had a read of the memcached specification this morning and completely agree with your comment. I also agree that your example idiom using add() makes sense, though I can't honestly say that it seems like the easiest or most obvious way of doing things a priori: I feel like it would be nicer to be able to do this with gets() and cas() alone, but alas.

@jparise
Copy link
Collaborator

jparise commented Feb 4, 2021

With #304, we now raise MemcacheIllegalArgumentException when cas=None is passed to cas().

@jparise
Copy link
Collaborator

jparise commented Feb 5, 2021

What other improvements should we consider making before we close this out? Some ideas:

  1. Add a section on CAS operations to the documentation that includes much of the above.
  2. Introduce an abstraction (context manager?) for using the "correct" CAS pattern.

@jparise
Copy link
Collaborator

jparise commented Mar 4, 2021

My MemcacheIllegalArgumentException improvement (from #304) is now available in the 3.4.1 release.

If we don't want to pursue any other improvements in this area, we can close this issue.

@cgordon
Copy link
Collaborator

cgordon commented Mar 4, 2021

I'm alright with closing it.

@jparise jparise closed this as completed Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants