It has been reported that upgrading a website to Memcache 6.x-1.3 breaks Mollom captcha's. I tracked this down to this commit.
In order to provide cache stampede protection, we no longer pass memcache an expire time. Instead, we set the expire time in the cache object, and then we check this when retrieving cached content with dmemcache_get(). This function assumes that $cache->expire will be a timestamp, but it can also be expressed as a number of seconds. The attach patch checks if $cache-expire is expressed in seconds and if so converts it to a timestamp so that the cache stampede protection works correctly.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | yh.patch | 1.03 KB | yhager |
| #5 | memcache.patch | 1.22 KB | jeremy |
| memcache.patch | 1.43 KB | jeremy |
Comments
Comment #1
jeremy commentedBTW: In most cases we don't notice this bug because if a cached item doesn't exist, we simply regenerate it. Mollom is using the cache a little differently, and if the cached item doesn't exist it can't validate the session_id and the Captcha validation fails.
Comment #2
chx commentedWell, this looks good however storing any data in cache that can't be regenerated is a bad idea. We learned our lesson with update status and form cache really should not be a cache either. Still: this patch is good.
Comment #3
jeremy commentedThis is the first time I've looked at the Mollom code, but I believe they're doing things right here. The content stored in cache_mollom can and is regenerated as necessary -- but it does require that the cached session can live in the cache long enough for someone to submit a comment, enter their login information, whatever. It was a memcache bug that was causing this content to be "flushed" instantly, breaking Mollom. When this happens, Mollom generates a new session and tries again, as it should using a cache_* table. With the patch attached above, things work as expected -- if the cache is flushed (ie memcache is restarted) Captcha validation will fail and the user will just think they misread the annoying cryptic letters that are Captchas and try again, then it will work...
Comment #4
jeremy commentedI believe this is also causing this bug:
#533524: High MySQL queries for 6.x-1.3 vs. 6.x-1.2
Comment #5
jeremy commentedThe previous patch fixed items that had an expiration defined in seconds, but it left CACHE_TEMPORARY still broken. The attached patch fixes both.
Comment #6
jeremy commentedWithout my patch I can duplicate the Mollom bug, and I can duplicate the page cache issue in that the page cache is rebuilt far too frequently. With the above patch, Mollom works and the page cache no longer is flushed artificially. I will commit this and roll a 6.x-1.4-rc1 to encourage further testing.
Comment #7
jeremy commentedFix applied:
http://drupal.org/cvs?commit=246944
Comment #8
damien tournoud commentedReopening.
cache_set() does not take a number of second as the $expire parameter, but only a timestamp. Let's not introduce hacks in memcache for my own programming errors.
Comment #9
damien tournoud commentedSee #548078: Mollom should call cache_set() with a timestamp, not a number of seconds.
Comment #10
miklAs Damien noted, this behavior is inconsistent with Drupal's own cache_set().
#548078: Mollom should call cache_set() with a timestamp, not a number of seconds has been committed to CVS, so I don't think there's any reason to keep this extra feature around :)
Comment #11
jeremy commentedMemcache supports either method of specifying the cache lifetime. The current memcache module code is still 100% compatible with Drupal core, while also exposing an extra memcache feature. I don't see any problem with this.
(The only reason this looks like a "hack" is because we've added cache stampede protection where we are handling cache expiration rather than memcache... but that "hack" can be quite a scalability boost.)
Comment #13
yhager commentedAttached a backport of this patch to 5.x version, as this issue was introduced there too.
Comment #14
mikl#11: It is not 100% compatible with Drupal core. It extends the API and makes it possible to make code that works correctly with your specific cache_set implementation, but not necessarily the one in Drupal core. Granted, it is a minor issue, but why support something that Drupal core doesn’t, if that only serves to make the resulting code non-portable?
Comment #15
jeremy commentedBy that argument why do we have any contrib modules, as they clearly provide functionality that's not offered by Drupal core.
You can keep your code 100% compatible with Drupal core if that is your requirement. But you should also be able to tap into memcache features not available to core if that's what you need.
Comment #16
mikl#15: We're not talking about new functionality here. In this case, a documented part of Drupal's API changes behaviour when your module is installed. If other modules changed the behaviour of the common API methods we all used, the Drupal ecosystem would be total chaos.
Comment #17
jeremy commentedWe extend the API, we do not modify the API.