Codeigniter's memcached driver _get_lock function returns incorrect results

[billystalnaker]

I believe this guy was having this same issue, but he wasn't aware it was a bug: https://github.com/bcit-ci/CodeIgniter/issues/5543

We've just upgraded our CI framework from 3.1.4 to 3.1.10 and have since been experiencing the same error that he was experiencing. I thought it was an issue with converting to session_locking but now understand that session locking has always been there in Codeigniter as a "virtual" lock. Which is great, I'm glad I can use memcached and still have session locking. However I've found one line of code that seems to cause the issue and reverting it back to what it was on 3.1.4 seems to resolve the error we're experiencing. On line 351 of the Session_memcached_driver.php file there is this line: if ( ! $this->_memcached->$method($lock_key, time(), 300)) the method variable is obvi determined by whether the lock key exists in memcached or not. But it seems that it never exists in memcached at all during my debug session. So I just reverted it to: if ( ! $this->_memcached->set($lock_key, time(), 300)) and all is well again.. Here are my comments in the code hopefully we can resolve this together so I can revert the changes that I've made to the memcached driver file:

            $method = ($this->_memcached->getResultCode() === Memcached::RES_NOTFOUND) ? 'add' : 'set';
            //todo figure out why memcached is saying that the key doesn't exist but when we use "add" it fails causing the lock function to return incorrect results.
            //Continuing to debug this may prove that we are getting different session ids, that's why the lock key isn't found against the old session key, but is against the new one, so essentially locking is not doing it's job in this state..
            //Debugging the above statement proves that the _lock_key is not set yet, meaning there is no lock set, and so that's why we're trying to add every time
            //I'm now thinking that the expiration time is the issue here.. Not that it's not enough time, 300 seconds, just that memcached expiration times are a little tricky to deal with it seems: https://www.php.net/manual/en/memcached.expiration.php
            //Debugging the above statement shows that the expiration time doesn't matter. I removed the expiration time parameter (default to never expire) and still produces the same result..
            //Perhaps the session lock release is releasing too soon, or too frequently? I haven't got time to debug this, there are too many references to that function. This will have to do for now.
            // I have made this change to the system code, cause I don't feel like overwritting the library and creating a new one.
            if ( ! $this->_memcached->set($lock_key, time(), 300))
            {...

Please let me know if there are any questions.

[narfbg]

When two separate processes attempt to add() at the same time (i.e. because AJAX requests executing at the same time will cause this), one of them will fail. Changing this to set() means that they just overwrite each other and that's not good.

[billystalnaker]

@narfbg That is the time that we experienced this error, when multiple ajax calls were run one after another. Are you saying that I should slow down my ajax requests?

[Paradinight]

@billystalnaker Read: https://www.codeigniter.com/user_guide/libraries/sessions.html?#a-note-about-concurrency

[billystalnaker]

Ok, that is helpful. I guess I have to now write code not normally needed when working with codeigniter... That doesn't make sense to me, but I will try to get my org to fund us restructuring things so that I can have 50 session_write_close's in the code now??

[narfbg]

Well, that escalated quickly ...

No, I'm not saying you should slow down your AJAX requests and I have not hinted at that in any way. I'm not even saying there's no bug somewhere in here; all I'm saying is that changing that add() call to set() will cause different problems.

How you got to the conclusion that now you need extra restructuring, funding and whatever else all because of CI is beyond me.