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

      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

          Activity

          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          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.

          Show
          Oleg Oleg [X] (Inactive) added a comment - 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.
          Hide
          deldel deldel added a comment -

          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.

          Show
          deldel deldel added a comment - 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.
          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          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.

          https://gist.github.com/8b8e98d22dd9c9b2ea31

          Show
          Oleg Oleg [X] (Inactive) added a comment - 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. https://gist.github.com/8b8e98d22dd9c9b2ea31
          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          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.

          Show
          Oleg Oleg [X] (Inactive) added a comment - 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.

            People

            • Assignee:
              Oleg Oleg [X] (Inactive)
              Reporter:
              deldel deldel
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development