set() method errors on expiry variable if var is int-inside-string #2

Closed
TJC opened this Issue Jul 4, 2012 · 2 comments

Projects

None yet

2 participants

@TJC
TJC commented Jul 4, 2012

The set() method accepts an option expiry time.

However if you have loaded your expiry time from a configuration file or something like that, which has caused it to be flagged as a string, then Couchbase::Client dies with an error "Expected numeric argument at .."

For eg:

my $expiry = "3600";
$client->set(Hello => "World", $expiry);

As a workaround, one can do this:

my $expiry = $c->config->{cache}{expiry};
$client->set(Hello => "World", int($expiry));
@mnunberg
Owner
mnunberg commented Jul 4, 2012

Ahh yes, the current code foolishly checks to see if SvIOK on the expiration. The code handling this is somewhat hairy as all those pretty entry points map to a single function.. (the expiry conversion is done in "interface" code, and not in the "core")..

A fix looks something like this, I'd prefer to test it and patch up the async client before placing it to CPAN or committing it.
The real problem with expiry (not so much with set, but more with get-and-touch, is that an expiry of 0 (as opposed to no expiry) will effectively delete the key immediately, hence the annoyances.

I would think the forced conversion via int() is a good method, though the code (as shown below) can make some good efforts to ensure that the string you're passing is indeed completely convertible to an integer.

diff --git a/xs/Client.xs b/xs/Client.xs
index 3e0a1a4..ca09ea3 100644
--- a/xs/Client.xs
+++ b/xs/Client.xs
@@ -242,11 +242,18 @@ static SV *PLCB_get_common(SV *self, SV *key, int exp_offset)
     if(items == (exp_idx - 1)) { \
         exp_var = 0; \
      } else if(items == exp_idx) { \
-        if(!SvIOK(ST(exp_idx-1))) { \
-            sv_dump(ST(exp_idx-1)); \
-            die("Expected numeric argument"); \
+        SV *_expsv = ST(exp_idx-1); \
+        UV _exp_real; \
+        if (SvIOK(_expsv)) { \
+            exp_var = SvUV(_expsv); \
+        } else if (SvPOK(_expsv)) { \
+            if (!grok_number(SvPVX(_expsv), SvCUR(_expsv), &_exp_real)) { \
+                die("Can't convert expiry string to int"); \
+            } \
+            exp_var = _exp_real; \
+        } else { \
+            die("Expiry is neither numeric nor a string"); \
         } \
-        exp_var = SvIV(ST((exp_idx-1))); \
     } else { \
         die(diemsg); \
     }
@mnunberg
Owner

Commited

@mnunberg mnunberg closed this Dec 26, 2012
@mnunberg mnunberg added a commit that referenced this issue Apr 16, 2013
@mnunberg GH-2, GH-8: Expiry and refcount handling
Forward port fixes from the master branch to allow for stringified
expiries. Also fix some issues with errors during multi calls
2efa6d0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment