Details
-
Type:
Bug
-
Status: Unverified Fix
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 3.0.10
-
Fix Version/s: 3.0.12-RC1, 3.1.0-a1
-
Component/s: Other
-
Labels:None
-
Environment:php 5.2.17, mysql 5.5.18, firefox 9.0.1
-
GitHub Pull Request URL:
Description
We've installed 3.0.10 a couple of days ago and found that our emails are not being sent. After some investigation I found something that looks like a bug.
File includes/functions_messenger.php, line 720:
if (!file_exists($this->cache_file) || filemtime($this->cache_file) > time() - $config['queue_interval'])
|
I believe it should be
if (!file_exists($this->cache_file))
|
as
|| filemtime($this->cache_file) > time() - $config['queue_interval'])
|
is a leftover from a stale lock check from old locking system, which used to be
if (!file_exists($this->cache_file) || (file_exists($this->cache_file . '.lock') && filemtime($this->cache_file) > time() - $config['queue_interval']))
|
and which used to work correctly due to && operator having higher precedence over || operator.
Issue Links
- is related to
-
PHPBB3-10604
Use lock_db class for queue locking in 3.1
-
- Open
-
- was caused by solution of
-
PHPBB3-9061
Race condition in queue locking
-
- Unverified Fix
-
Activity
If we don't have flock with the current locking system checking mtime of the queue file gives us absolutely nothing. The probability of concurrent access remains the same regardless of the time the last access occurred. But, yeah, as you said, with working flock or without it, this mtime check prevents any mails to be sent out if incoming rate is higher than 1 message per queue_interval.
Maybe there should be a configuration option to choose a locking mechanism?
Btw, just out of curiosity, why would a queue be implemented in a file rather than in a database? Locking would be much easier in a DB, queue insertion would be much faster, reading/writing a queue would not require to unserialize() the whole queue. 6.5Mbytes queue took 2.5 minutes on my machine (old core2 quad) to get read.
For 3.1 we will use the lock_db class we already have there for cron. (Note, this has nothing to do with serializing the queue.)
For 3.0 we should add the flag file as the lock class is not portable to 3.0.
Please try https://github.com/phpbb/phpbb3/pull/1120.
There is no need for a flag file as we already have a config variable holding the same data.
01ef46a5 is the commit in question (mine). With the current code, queue would never run if more messages are added to it before queue_interval runs out.
The old version checked mtime on cache_file, whereas it should probably have checked the mtime on the lock file instead.
With the current code, if we have flock and it works correctly we should be able to unconditionally process the queue as OP suggests. If however we don't have flock or it silently does nothing, which I think is what does in fact happen on some systems, then omitting mtime check will result in (more) concurrent queue processing.
If we want the queue to work somewhat correctly in the absence of locking I think we need to reinstate a flag file that exists while the queue is being processed and does not exist otherwise. (Existence and) mtime check should then be done against that file. The flag file should be created and destroyed under flock lock.