pylibmc should raise if a negative timeout is used in binary mode

[edmorley]

The pylibmc docs say:

Negative timeouts are treated the same way as zero timeouts are in pylibmc. python-memcached treats this as immediate expiry, returning success while not setting any value. This might raise exceptions in the future.

I interpreted this as meaning:

• python-memcached
  • negative -> immediate expiry
  • zero -> never expire
• pylibmc:
  • negative -> never expire
  • zero -> never expire

However the behaviour actually seems to vary depending on whether binary mode is used, eg in an Ubuntu Trusty vagrant environment:

Python 2.7.11 (default, Dec 15 2015, 16:48:05)
[GCC 4.8.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import pylibmc
>>> import _pylibmc
>>> _pylibmc.libmemcached_version
'1.0.8'
>>> mc = pylibmc.Client(["127.0.0.1"], binary=False)
>>> mc.set("foo", "bar", -1)
False
>>> mc.get("foo")
>>> bc = pylibmc.Client(["127.0.0.1"], binary=True)
>>> bc.set("foo2", "bar", -1)
True
>>> bc.get("foo2")
>>>

...ie the return value of the .set() varies.

However on Travis the differences are more extreme - the return value of the .get() isn't None when using binary mode, eg: https://github.com/lericson/pylibmc/compare/master...edmorley:test-libmemcached-bug https://travis-ci.org/edmorley/pylibmc/builds/105429852

Any ideas? :-)

[edmorley]

Or to be more specific:

1. Is my interpretation of the docs correct? (I'm happy to open a PR to tweak the wording to make it clearer either way).
2. Should pylibmc instead follow the python-memcached behaviour for consistency?
3. I'm presuming it's not expected for the behaviour to vary based on whether binary mode is being used?
4. Perhaps negative values should just be forbidden? I'm not sure I see the use case for them?

The reason for filing this is that my tests for https://github.com/django-pylibmc/django-pylibmc/pull/36 are failing due to this: https://travis-ci.org/django-pylibmc/django-pylibmc/builds/105396972

It also affects Django supporting binary mode natively in the future: https://code.djangoproject.com/ticket/15815#comment:16

Many thanks :-)

[edmorley]

I've done some further testing on Travis - I can reproduce the issue under all three combinations of:

• legacy infra Ubuntu precise
• container Ubuntu precise
• GCE Ubuntu trusty

(eg https://travis-ci.org/edmorley/memcached-test/builds/105901400)

...however I'm not able to do so in my local Vagrant Ubuntu 14.04 environment, even though both it and the 14.04 Travis run are using libmemcached 1.0.8 with memcached server 1.4.14.

python-binary-memcached doesn't handle negative binary times at all (eg [1]), and the memcached protocol docs don't specify expected behaviour, so I've filed an issue to clarify it: https://github.com/memcached/memcached/issues/142

[1] https://travis-ci.org/edmorley/memcached-test/jobs/105901402#L129

[lericson]

Interesting. I'll look at this and get back to you in a second.

[lericson]

The way pylibmc does it at the moment is just pass the value on to libmemcached, but since it's unclear what memcached even should be doing with negative expiration times, I'm not sure pylibmc is the place to fix this. What does the Django cache backend using python-memcached do here?

[edmorley]

I've used this file to test the behaviour against python-memcached, python-binary-memcached & pylibmc (with both binary mode True and False): https://github.com/edmorley/memcached-test/blob/d5a15411cb17f1de7cbee65e02cfbe688333284d/test.py (latest released versions of each)

Results:

Results	Vagrant 14.04 x86	Vagrant 15.10 x64	Travis Precise	Travis container	Travis Trusty
python-memcached SET	True	True	True	True	True
python-memcached GET	None	None	None	None	None
pylibmc binary=False SET	False	True	True	True	True
pylibmc binary=False GET	None	None	None	None	None
pylibmc binary=True SET	True	True	True	True	True
pylibmc binary=True GET	None	VALUE	VALUE	VALUE	VALUE
bmemcached SET	ERROR	ERROR	ERROR	ERROR	ERROR
bmemcached GET	N/A	N/A	N/A	N/A	N/A
memcached server	1.4.14	1.4.24	1.4.13	1.4.13	1.4.14
libmemcached	1.0.8	1.0.18	0.44	0.44	1.0.8

Original output...

Local testing with Ubuntu 14.04 in Vagrant: https://emorley.pastebin.mozilla.org/8860655

Local testing with Ubuntu 15.10 in Vagrant: https://emorley.pastebin.mozilla.org/8860656

The Travis runs are here: https://travis-ci.org/edmorley/memcached-test/jobs/110992864 https://travis-ci.org/edmorley/memcached-test/jobs/110992865 https://travis-ci.org/edmorley/memcached-test/jobs/110992866

[edmorley]

So there are a few issues:

1. At least in my local Vagrant 14.04 environment, the pylibmc .set() with a negative expiry gives a False return value, when not using binary mode, unlike every other case.
2. If binary mode is used with pylibmc, then behaviour is (a) inconsistent with pylibmc binary=False, and more importantly (b) not consistent with python-memcached.
3. python-binary-memcached (bmemcached) fails completely with negative expiry times.

We really need the spec to be clarified IMO: https://github.com/memcached/memcached/issues/142

[edmorley]

Ah the local Vagrant Ubuntu 14.04 environment is x86 (I'd completely forgotten; table now updated above), which explains the difference in results (I'm presuming the -1 wraps around to a different positive int depending on the architecture's size of PyLong_AS_LONG - and therefore is either a valid unixtime or not - or something along those lines).

Judging by the comments about the spec (https://github.com/memcached/memcached/issues/142#issuecomment-187306659) it seems like negative timeout values should just raise an exception since their behaviour is not deterministic across architectures and/or binary={True,False}.

[lericson]

I agree, negative timeouts should raise an exception.

[edmorley]

The memcached docs have just been updated: https://github.com/memcached/memcached/issues/142#issuecomment-222334563

In binary mode the time must be unsigned. In ASCII mode, a negative time means "immediately expire".

As such, it would be great if pylibmc generated an exception if a negative timeout is used in binary mode :-)

[edmorley]

@lericson what's the best way to go about implementing this? Should it be in libmemcached, or is that unlikely to happen given it's poorly-maintained, and so best handled in pylibmc? If the latter, where should I start? (Sorry my Python-C binding foo is basically non-existent)

Many thanks :-)

[lericson]

Sorry been super busy, I’ll get back to you in the next 48 hours.

On 1 Sep 2016, at 15:44, Ed Morley [email protected] wrote:

@lericson https://github.com/lericson what's the best way to go about implementing this? Should it be in libmemcached, or is that unlikely to happen given poorly maintained, and so best handled in pylibmc? If the latter, where should I start? (Sorry my Python-C binding foo is basically non-existent)

Many thanks :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lericson/pylibmc/issues/202#issuecomment-244082734, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC3NWr1x2yqYcuLZvFGTyMPhZluQlsbks5qltalgaJpZM4HOYCU.

[lericson]

Ah yes I think it would make sense to check for signedness at the Python C API level; alternatively fixing libmemcached. Whichever you prefer. Sorry, those 48 hours got drawn out to almost a year.