r4131 - in trunk/plugins/ullMailPlugin: config lib/task test/unit

[s...@ull.at]

Author: klemens
Date: 2014-07-09 11:13:31 +0200 (Wed, 09 Jul 2014)
New Revision: 4131

Modified:
trunk/plugins/ullMailPlugin/config/ullMailPluginConfiguration.class.php
trunk/plugins/ullMailPlugin/lib/task/SpoolEmailsTask.class.php
trunk/plugins/ullMailPlugin/test/unit/spoolEmailsTaskTest.php
Log:
ullNewsletter: fixed double spool bug - db lock was released to soon

Modified: trunk/plugins/ullMailPlugin/config/ullMailPluginConfiguration.class.php
===================================================================
--- trunk/plugins/ullMailPlugin/config/ullMailPluginConfiguration.class.php 2014-07-09 07:36:14 UTC (rev 4130)
+++ trunk/plugins/ullMailPlugin/config/ullMailPluginConfiguration.class.php 2014-07-09 09:13:31 UTC (rev 4131)
@@ -184,7 +184,7 @@


/**
- * Enable automatichtml to text transformation for html emails
+ * Enable automatic html to text transformation for html emails
*
* @param sfMailer $mailer
*/

Modified: trunk/plugins/ullMailPlugin/lib/task/SpoolEmailsTask.class.php
===================================================================
--- trunk/plugins/ullMailPlugin/lib/task/SpoolEmailsTask.class.php 2014-07-09 07:36:14 UTC (rev 4130)
+++ trunk/plugins/ullMailPlugin/lib/task/SpoolEmailsTask.class.php 2014-07-09 09:13:31 UTC (rev 4131)
@@ -13,7 +13,7 @@

[php symfony {$this->namespace}:{$this->name}|INFO]

- This task usually is invoked by a frequently run cronjob.
+ This task is usually invoked by a cronjob every 5 minutes.
EOF;

$this->addArgument('application', sfCommandArgument::OPTIONAL,
@@ -21,12 +21,18 @@
$this->addArgument('env', sfCommandArgument::OPTIONAL,
'The environment', 'dev');
$this->addOption('less-noisy', null, sfCommandOption::PARAMETER_NONE,
- 'Be less noisy. Output interesting stuff only. Used for cron jobs');
+ 'Be less noisy. Output interesting stuff only. Used for cron jobs');
+ $this->addOption('release-lock-age', null, sfCommandOption::PARAMETER_OPTIONAL,
+ 'For testing, in seconds', 86400 /* 1 day */);
+ $this->addOption('test-delay', null, sfCommandOption::PARAMETER_OPTIONAL,
+ 'For testing, delay edition iteration, in seconds', 0);
}


protected function execute($arguments = array(), $options = array())
{
+ $startTime = microtime(true);
+
if ($options['less-noisy'])
{
$this->setIsLessNoisy(true);
@@ -46,8 +52,9 @@
$uniqueId = uniqid(getmypid(), true);

//remove old newsletter edition locks (> 15 min)
- $table = Doctrine_Core::getTable('UllNewsletterEdition');
- $lockingManager->releaseAgedLocks(900, $table->getComponentName());
+ //TODO: maybe this is much to low!!! spooling may take longer!
+ $lockingManager->releaseAgedLocks($options['release-lock-age'],
+ 'UllNewsletterEdition');

$editions = UllNewsletterEditionTable::findEditionsToBeSpooled();

@@ -61,15 +68,19 @@

foreach ($editions as $edition)
{
- //request exclusive lock for the newsletter object
+ //request exclusive lock for the UllNewsletterEdition object
if (!$this->requestLock($lockingManager, $edition, $uniqueId))
{
- $this->logNoisySection($this->name, 'Could not retrieve lock for newsletter edition with id: ' .
- $edition['id']);
+ $this->logNoisySection($this->name,
+ 'Could not retrieve lock for newsletter edition with id: ' .
+ $edition['id'] . ' (' . $edition['subject'] . ')' .
+ ' at ' . date('Y-m-d H:i:s')
+ );
continue;
}

- $this->logNoisySection($this->name, 'Now spooling ' . $edition['subject']);
+ $this->logNoisySection($this->name, 'Now spooling ' . $edition['subject'] .
+ ' (id: ' . $edition['id'] . ') at ' . date('Y-m-d H:i:s'));

$context->getUser()->setCulture($edition['sender_culture']);

@@ -87,8 +98,9 @@
$numSent = 0;

$connection = Doctrine_Manager::connection();
- $transaction = $connection->beginTransaction();
+ $connection->beginTransaction();
$transaction = $connection->transaction;
+ // SERIALIZEABLE is the most secure isolation type.
$transaction->setIsolation('SERIALIZABLE');

$this->logNoisySection($this->name, 'Beginning to spool');
@@ -115,12 +127,15 @@

$this->createFailedLoggedMessage($currentMail, $recipient);
}
- }
+ } // end of recipient iteration

+ //delay for testing
+ sleep($options['test-delay']);
+
$edition['queued_at'] = date('Y-m-d H:i:s');
$edition->save();

- $this->logNoisySection($this->name, 'Comitting all spooled messages');
+ $this->logNoisySection($this->name, 'Committing all spooled messages');
$connection->commit();
}
catch (Exception $e)
@@ -142,6 +157,9 @@
$this->logNoisySection($this->name, print_r($failedRecipients, true));
}
} // end of foreach edition
+
+ $runtime = microtime(true) - $startTime;
+ $this->logNoisySection($this->name, 'Runtime (sec): ' . $runtime);
}

/**

Modified: trunk/plugins/ullMailPlugin/test/unit/spoolEmailsTaskTest.php
===================================================================
--- trunk/plugins/ullMailPlugin/test/unit/spoolEmailsTaskTest.php 2014-07-09 07:36:14 UTC (rev 4130)
+++ trunk/plugins/ullMailPlugin/test/unit/spoolEmailsTaskTest.php 2014-07-09 09:13:31 UTC (rev 4131)
@@ -2,10 +2,26 @@

include dirname(__FILE__) . '/../../../../test/bootstrap/unit.php';

+/**
+ * Create and "send" a newsletter edition to be spooled
+ * @param string $subject
+ */
+function createAndSendEdition($subject)
+{
+ $msg = new UllNewsletterEdition();
+ $msg->subject = $subject;
+ $msg->body = 'Dear mum, ...';
+ $msg->submitted_at = new Doctrine_Expression('NOW()');
+ $msg->Sender = Doctrine::getTable('UllUser')->find(1); // Master Admin
+ $msg->UllNewsletterMailingLists[] =
+ Doctrine::getTable('UllNewsletterMailingList')->findOneBySlug('product-news');
+ $msg->save();
+}
+
// create context since it is required by ->getUser() etc.
sfContext::createInstance($configuration);

-$t = new sfDoctrineTestCase(9, new lime_output_color, $configuration);
+$t = new sfDoctrineTestCase(10, new lime_output_color, $configuration);
$path = dirname(__FILE__);
$t->setFixturesPath($path);

@@ -17,7 +33,7 @@

$t->diag('Create and send newsletter');

-// Create invalid email address
+// Create user with invalid email address
$testUser = Doctrine::getTable('UllUser')->findOneByEmail('test...@example.com');
$testUser->email = 'invalid.@example.com';
$testUser->save();
@@ -48,4 +64,31 @@

$t->is($msg->num_failed_emails, 1, 'UllMailEdition::num_failed_emails is set to 1');

+
+$t->diag('Prevent duplicate spooling');

+ createAndSendEdition('Spool test 1');
+ $t->diag(shell_exec(
+ 'php symfony ull_mail:spool-emails --test-delay=1 frontend test'
+ . '&php symfony ull_mail:spool-emails frontend test'
+ ));
+
+ // This is how we can provoke a dangerous double spooling because the lock is
+ // cleaned too soon
+ /*
+ $t->diag(shell_exec(
+ 'php symfony ull_mail:spool-emails --release-lock-age=1 --test-delay=5 frontend test'
+ . '&sleep 2;php symfony ull_mail:spool-emails --release-lock-age=1 frontend test'
+ ));
+ */
+
+ $t->is(
+ Doctrine::getTable('UllMailQueuedMessage')->count(),
+ 2,
+ 'Edition got spooled only once, so we have two messages'
+ );
+// --release-lock-age=1
+// $t->diag(shell_exec('php symfony ull_mail:spool-emails --release-lock-age=1 frontend test'));
+
+
+