fix delete parsing for memcache

[charsyam]

I think we should support memcache delete argument. because memcached delete spec allow arguments.

See process_delete_command in https://github.com/memcached/memcached/blame/master/memcached.c

    if (ntokens > 3) {
        bool hold_is_zero = strcmp(tokens[KEY_TOKEN+1].value, "0") == 0;
        bool sets_noreply = set_noreply_maybe(c, tokens, ntokens);
        bool valid = (ntokens == 4 && (hold_is_zero || sets_noreply))
            || (ntokens == 5 && hold_is_zero && sets_noreply);
        if (!valid) {
            out_string(c, "CLIENT_ERROR bad command line format.  "
                       "Usage: delete <key> [noreply]");
            return;
        }
    }

so delete command syntax like below:

delete <key>
delete <key> 0
delete <key> <numbers> noreply

Actually, memcached doesn't handle but, it doesn't return ERROR, just it ignores internally.
so. I just follow current memecached behavior. :)

[charsyam]

This bug cause to fail to delete in some library.
for example,
in python-memecached, if someone use delete_multi method. it fails to delete keys.

[charsyam]

@idning could you review this patch?
It occurs errors with python-memcached library when user call delete_multi command.

[idning]

we have tested delete_multi here:

https://github.com/idning/test-twemproxy/blob/master/test_memcache/test_gets.py#L78

and the protocol say nothing about this <number> argument.

https://github.com/memcached/memcached/blob/master/doc/protocol.txt#L249-L254

should we support it?

[charsyam]

@idning, I think it is better to support it. because Memcached accept this case.

the document is not applied real code.
in current version of memcached(especially, in ascii protocol)

https://github.com/memcached/memcached/blob/master/memcached.c#L3320

The code can accept when number is 0.
so. "delete 0" is accepted in Memcached.

I also agree that we don't need to support some irregular cases.
but in this cases, Memcached supports this, so I think it is better to support numbers when it is 0.

[idning]

we should follow the protocol but not the implement.

because that's other implement of memcache like twemcache.

[charsyam]

@idning I also agree with you. but, I think this case are somewhat different.
because users can be confused and they will think twemproxy has bug.
so. to prevent misunderstand for twemproxy. I think it is needed. Thank you :)

I found this bug with python-memcached. and I found this testcase also has bug.
https://github.com/idning/test-twemproxy/blob/master/test_memcache/test_gets.py#L110

we can reproduce this.

1. python-memcached send "delete key 0" in delete_multi.
2. twemrpoxy close client connection because it has invalid parameter.
3. python-memcached send "get key"
   -> but this time, client can't connect twemproxy, so it just return null array or null value.
4. test-twemproxy get "{}" so, it think test is ok.
   -> but this is wrong.

so. if we tried get code several times. we can get original data that is not deleted.

[manjuraj]

we shouldn't do this - this behavior of memcache's delete handling is not in spec. In fact, as you just mentioned it ignores the <numbers>. So, basically someone added code to memcache and forgot to make use of it.

[manjuraj]

I'm gonna close this request for now - feel free to reopen if you folks disagree

[charsyam]

@manjuraj @idning , You make sense. to follow spec is important.

but I think I can't supply enought information about this problem.

before starting, below is another problem and we can ignore this.(this is actually memcached bug)

delete key <number> <noreply>

but real problem that I think is with python-memcached library.(python-memcached is de-factor python
library to use memcached).

historically to support defer_delete(but this is only exist in 1.2.x of memcache, current version 1.4.x just allow number is 0) python-memcached has default time parameter(and it's default value is 0)

and it has two delete functions(delete and delte_multi)

1. fortunately, delete is ok. it's python-memecached code.
   time's default value is 0, so this is not problem. twemproxy will pass this.

        if time is not None and time != 0:
            cmd = "%s %s %d%s" % (cmd, key, time, extra)
        else:
            cmd = "%s %s%s" % (cmd, key, extra)

1. unfortunately, delete_multi is always broken.
   it just check time is not None, so it always make "delete key 0"
   and twemrpoxy always close this connection because it is invalid parameter.

            if time is not None:
                for key in server_keys[server]:  # These are mangled keys
                    write("delete %s %d%s\r\n" % (key, time, extra))
            else:
                for key in server_keys[server]:  # These are mangled keys
                    write("delete %s%s\r\n" % (key, extra))

to avoid this problem. python-memcached users can set time option to None explcit, delete_multi like this

conn.delete_multi['key1','key2'], time=None]

not 

conn.delete_multi(['key1','key2']

I just wonder that python-memcached users can get bad prejudice like that
"twemproxy has some deletion bug." event though it is actually not twemproxy's bug.
It is easy to misunderstand.

[manjuraj]

maybe the right fix is

• submit patch to python-memcached or
• a note somewhere in the notes/ folder that talks about this behavior of python-memcached

I think the latter makes sense

[idning]

@charsyam sorry for the delay.

you are right about the bug in our testcase (I assume that it will raise a Exception if server close connection.)

About this isuue, i think we should fix delete_multi in python-memcache and I see @charsyam 's PR here: https://github.com/linsomniac/python-memcached/pull/62/files

that's great.

[rohitpaulk]

Note: The usage in python-memcached was fixed in linsomniac/python-memcached#27