CAS is broken

[dormando]

Chris and I went over this a bit..

The only way around it without major refactoring would be to extend the
item suffix by 20 bytes. Don't know how well that would sit with other 
people on the list.

Other ideas include adding a tiny buffer that's associated during gets,
and disassociated right after a transmit.

Unfortunately that might require abstracting the message buffer one
more level (and wasting more bytes in the process). I don't want to do
a major network reworking just today. I toyed with the idea of abusing
the msg_control field of msghdr to manage this buffer.

It'd get the job done, but it's gross and what if someone ever _wants_
to use msg_control? :)

Anyone think of something simpler? Is extending the item suffix okay?

-Dormando

Tomash Brechko wrote:
> Hello,
> 
> Unfortunately CAS problems are not over, this time I see that gets
> command doesn't work when more than one key is queried.
> 
> You may try the test at the end of this mail.  When gets is used with
> one key it works OK (not true as we'll see shortly).  But when two or
> more keys are queried, reported CAS value is for the last key, last
> test fails (actually I made the test so, the truth is that flags and
> length are _also_ for the last key :/).
> 
> The problem is in process_get_command():
> 
>   if(return_key_ptr == true)
>   {
>     sprintf(suffix," %d %d %llu\r\n", atoi(ITEM_suffix(it) + 1),
>         it->nbytes - 2, it->cas_id);
>     if (add_iov(c, "VALUE ", 6) != 0 ||
>         add_iov(c, ITEM_key(it), it->nkey) != 0 ||
>         add_iov(c, suffix, strlen(suffix)) != 0 ||
> 
> 
> Array char suffix[255] is local to this function, it is filled with
> the info for current key, and passed to add_iov().  The bad news is
> that add_iov() _does not make a copy_, it simply stores the pointer to
> data.  So this pointer points to the suffix buffer with latest data,
> the data for the last key.  Not to say that it is long popped off the
> stack when used later :/.
> 
> I haven't fixed this because I'm not sure how it would be better to do
> that: where to allocate the string so it won't leak later.
> 
> 
> 
> diff --git a/trunk/server/t/cas.t b/trunk/server/t/cas.t
> index 48cf8b9..7613811 100644
> --- a/trunk/server/t/cas.t
> +++ b/trunk/server/t/cas.t
> @@ -1,7 +1,7 @@
>  #!/usr/bin/perl
>  
>  use strict;
> -use Test::More tests => 10;
> +use Test::More tests => 26;
>  use FindBin qw($Bin);
>  use lib "$Bin/lib";
>  use MemcachedTest;
> @@ -50,4 +50,40 @@ is(scalar <$sock>, "NOT_FOUND\r\n", "cas failed, foo does not exist");
>  # cas empty
>  print $sock "cas foo 0 0 6 \r\nbarva2\r\n";
>  is(scalar <$sock>, "ERROR\r\n", "cas empty, throw error");
> +# Then the server reads "barva2\r\n", and can't parse it either.
> +is(scalar <$sock>, "ERROR\r\n");
>  
> +
> +# Test that different keys have different CAS value.
> +print $sock "set foo1 0 0 1\r\n1\r\n";
> +is(scalar <$sock>, "STORED\r\n");
> +print $sock "set foo2 0 0 1\r\n2\r\n";
> +is(scalar <$sock>, "STORED\r\n");
> +
> +print $sock "gets foo1\r\n";
> +ok(scalar <$sock> =~ /VALUE foo1 0 1 (\d+)\r\n/);
> +my $foo1_cas = $1;
> +is(scalar <$sock>, "1\r\n");
> +is(scalar <$sock>, "END\r\n");
> +
> +print $sock "gets foo2\r\n";
> +ok(scalar <$sock> =~ /VALUE foo2 0 1 (\d+)\r\n/);
> +my $foo2_cas = $1;
> +is(scalar <$sock>, "2\r\n");
> +is(scalar <$sock>, "END\r\n");
> +
> +ok($foo1_cas != $foo2_cas);
> +
> +print $sock "gets foo1 foo2\r\n";
> +#ok(scalar <$sock> =~ /VALUE foo1 0 1 (\d+)\r\n/);
> +is(scalar <$sock>, "/VALUE foo1 0 1 (\d+)\r\n/");
> +$foo1_cas = $1;
> +is(scalar <$sock>, "1\r\n");
> +ok(scalar <$sock> =~ /VALUE foo2 0 1 (\d+)\r\n/);
> +$foo2_cas = $1;
> +is(scalar <$sock>, "2\r\n");
> +is(scalar <$sock>, "END\r\n");
> +
> +is($foo1_cas, "");
> +is($foo2_cas, "");
> +ok($foo1_cas != $foo2_cas);
> 
>