From 12f26abc40e07aa3bcfd5702c8261b2341c0ded1 Mon Sep 17 00:00:00 2001 From: Bob Vincent Date: Thu, 19 May 2011 21:28:50 -0400 Subject: [PATCH] Issue #361071 by MauMau, Sutharsan, Frans, BrockBoland, miro_dietiker, Fabianx, Vasudeva, pillarsdotnet: Use locking to eliminate duplicate emails from concurrent simplenews cron runs. --- includes/simplenews.mail.inc | 230 +++++++++++++++++++++++++++++------------- simplenews.module | 174 ++++++++++++++++++-------------- 2 files changed, 259 insertions(+), 145 deletions(-) diff --git a/includes/simplenews.mail.inc b/includes/simplenews.mail.inc index 0ec9d87cdf3e64820f163e63dad746084ea5e84f..66967df3ec777268ca165e5ad3d4cbe26e410de2 100644 --- a/includes/simplenews.mail.inc +++ b/includes/simplenews.mail.inc @@ -10,17 +10,19 @@ /** * Send newsletter node to subscribers. * - * @param integer or object $node Newsletter node to be sent. integer = nid; object = node object - * @param array $accounts account objects to send the newsletter to. - * account = object ( - * snid = subscription id. 0 if no subscription record exists - * tid = newsletter category ID - * uid = user id. 0 if subscriber is anonymous user. - * mail = user email address. - * name = . Added for compatibility with user account object - * language = language object. User preferred of default language - * ) - * NOTE: either snid, mail or uid is required. + * @param mixed $node + * The newsletter node to be sent. If an integer, the node nid; if an object, + * the node object. + * @param array $accounts + * An array of recipient accounts for this newsletter. Each account may + * have the following properties, but either snid, mail, or uid must be + * defined: + * - snid: The subscription id, or 0 if no subscription record exists. + * - tid: The newsletter category id. + * - uid: The subscriber's uid, or 0 if the subscriber has no user account. + * - mail: The subscriber's email address. + * - name: Emtpy; added for compatibility with user account object. + * - language: The language object of the subscriber's preferred language. */ function simplenews_send_node($node, $accounts = array()) { if (is_numeric($node)) { @@ -97,7 +99,9 @@ function simplenews_send_node($node, $accounts = array()) { /** * Send test version of newsletter. * - * @param integer or object $node Newsletter node to be sent. Integer = nid; Object = node object + * @param mixed $node + * The newsletter node to be sent. If an integer, the node nid; if an object, + * the node object. */ function simplenews_send_test($node, $test_addresses) { if (is_numeric($node)) { @@ -159,14 +163,17 @@ function simplenews_send_test($node, $test_addresses) { * Send a node to an email address. * * @param object $msgbase - * Mail message object as returned by simplenews_load_spool(): - * $msgbase->nid - * $msgbase->vid - * $msgbase->tid - * $msgbase->data - * @param $key email key [node|test] + * The mail message object as returned by simplenews_load_spool(), containing + * the following properties: + * - nid + * - vid + * - tid + * - data + * @param $key + * The email key, either 'node' or 'test'. * - * @return TRUE if email is successfully delivered by php mail() + * @return boolean + * TRUE if the email was successfully delivered; otherwise FALSE. */ function simplenews_mail_mail($msgbase, $key = 'node') { static $cache; @@ -270,7 +277,7 @@ function simplenews_mail_spool($nid = NULL, $vid = NULL, $limit = NULL) { // Send pending messages from database cache // A limited number of mails is retrieved from the spool $limit = isset($limit) ? $limit : variable_get('simplenews_throttle', 20); - if ($spool_list = simplenews_get_spool(SIMPLENEWS_SPOOL_PENDING, $nid, $vid, $limit)) { + if ($spool_list = simplenews_get_spool(array(SIMPLENEWS_SPOOL_PENDING, SIMPLENEWS_SPOOL_IN_PROGRESS), $limit)) { // Prevent session information from being saved while sending. if ($original_session = drupal_save_session()) { drupal_save_session(FALSE); @@ -337,16 +344,14 @@ function simplenews_mail_spool($nid = NULL, $vid = NULL, $limit = NULL) { * Save mail message in mail cache table. * * @param array $spool - * Data array to be stored in the spool table. - * $spool['mail'] - * $spool['nid'] - * $spool['vid'] - * $spool['tid'] - * $spool['status'] (Default: 1 = pending) - * $spool['time'] (default: current unix timestamp) - - * @param array $spool Mail message array - * @todo Replace time(): http://drupal.org/node/224333#time + * The message to be stored in the spool table, as an array containing the + * following keys: + * - mail + * - nid + * - vid + * - tid + * - status: (optional) Defaults to SIMPLENEWS_SPOOL_PENDING + * - time: (optional) Defaults to REQUEST_TIME. */ function simplenews_save_spool($spool) { $status = isset($spool['status']) ? $spool['status'] : SIMPLENEWS_SPOOL_PENDING; @@ -366,43 +371,102 @@ function simplenews_save_spool($spool) { ->execute(); } +/* + * Returns the expiration time for IN_PROGRESS status. + * + * @return int + * A unix timestamp. Any IN_PROGRESS messages with a timestamp older than + * this will be re-allocated and re-sent. + */ +function simplenews_get_expiration_time() { + $timeout = variable_get('simplenews_spool_progress_expiration', 3600); + $expiration_time = REQUEST_TIME - $timeout; + return $expiration_time; +} + /** - * Retrieve data from mail spool + * This function allocates messages to be sent in current run. * - * @param string $status Status of data to be retrieved (0 = hold, 1 = pending, 2 = send) - * @param integer $nid node id - * @param integer $vid node version id - * @param integer $limit The maximum number of mails to load from the spool + * Drupal acquire_lock guarantees that no concurrency issue happened. + * If the message status is SIMPLENEWS_SPOOL_IN_PROGRESS but the maximum send + * time has expired, the message id will be returned as a message which is not + * allocated to another process. * - * @return array Spool data - * $spool['msid'] - * $spool['mail'] - * $spool['nid'] - * $spool['tid'] - * $spool['status'] - * $spool['time'] - * @todo Convert output to array of objects. + * @param string $status + * The status of data to be retrieved. Must be one of: + * - 0: hold + * - 1: pending + * - 2: send + * - 3: in progress + * @param integer $limit + * The maximum number of mails to load from the spool. + * + * @return array + * An array of message ids to be sent in the current run. */ -function simplenews_get_spool($status, $nid = NULL, $vid = NULL, $limit = 0) { - $spool = array(); +function simplenews_get_spool($status, $limit = NULL) { + $messages = array(); + $clauses = array(); + $params = array(); + + if (!is_array($status)) { + $status = array($status); + } + + foreach ($status as $s) { + if ($s == SIMPLENEWS_SPOOL_IN_PROGRESS) { + // Select messages which are allocated by another process, but whose + // maximum send time has expired. + $clauses[] = '(s.status = :status AND s.timestamp < :expiration)'; + $params[':status'] = $s; + $params[':expiration'] = simplenews_get_expiration_time(); + } + else { + $clauses[] = 's.status = :status'; + $params[':status'] = $s; + } + } $query = db_select('simplenews_mail_spool', 's') ->fields('s') - ->condition('s.status', $status) + ->where(implode(' OR ', $clauses), $params) ->orderBy('s.timestamp', 'ASC'); - if ($limit) { - $query->range(0, $limit); - } - foreach ($query->execute() as $row) { - if (strlen($row->data)) { - $row->data = unserialize($row->data); + + /* BEGIN CRITICAL SECTION */ + // The semaphore ensures that multiple processes get different message ID's, + // so that duplicate messages are not sent. + + if (lock_acquire('simplenews_acquire_mail')) { + // Get message id's + // Allocate messages + if (is_numeric($limit)) { + $query->range(0, $limit); + } + foreach ($query->execute() as $message) { + if (strlen($message->data)) { + $message->data = unserialize($message->data); + } + else { + $mail = (object)array('mail' => $message->mail); + $message->data = simplenews_get_subscription($mail); + } + $messages[$message->msid] = $message; } - else { - $row->data = simplenews_get_subscription((object)array('mail' => $row->mail)); + if (count($messages) > 0) { + // Set the state and the timestamp of the messages + simplenews_update_spool( + array_keys($messages), + array('status' => SIMPLENEWS_SPOOL_IN_PROGRESS) + ); } - $spool[$row->msid] = $row; + + lock_release('simplenews_acquire_mail'); } - return $spool; + + /* END CRITICAL SECTION */ + + + return $messages; } /** @@ -413,9 +477,13 @@ function simplenews_get_spool($status, $nid = NULL, $vid = NULL, $limit = 0) { * @param array $msids * Array of Mail spool ids to be updated * @param array $data - * Array containing email sent results - * 'status' => (0 = hold, 1 = pending, 2 = send) - * 'error' => error id (optional; defaults to '') + * Array containing email sent results, with the following keys: + * - status: An integer indicating the updated status. Must be one of: + * - 0: hold + * - 1: pending + * - 2: send + * - 3: in progress + * - error: (optional) The error id. Defaults to 0 (no error). */ function simplenews_update_spool($msids, $data) { db_update('simplenews_mail_spool') @@ -431,17 +499,32 @@ function simplenews_update_spool($msids, $data) { /** * Count data in mail spool table. * - * @param integer $nid newsletter node id - * @param integer $vid newsletter revision id - * @param string $status email sent status + * @param integer $nid + * Newsletter node id. + * @param integer $vid + * Newsletter revision id. + * @param array $status + * Array of status flags. * - * @return array Mail message array + * @return integer + * Count of mail spool elements which own the attributes passed in as params. */ -function simplenews_count_spool($nid, $vid, $status = SIMPLENEWS_SPOOL_PENDING) { +function simplenews_count_spool($nid, $vid, $status = array(SIMPLENEWS_SPOOL_PENDING, SIMPLENEWS_SPOOL_IN_PROGRESS)) { + $clauses = array(); + $params = array(); + + if (!is_array($status)) { + $status = array($status); + } + + foreach ($status as $s) { + $clauses[] = "s.status = $s"; + } + $query = db_select('simplenews_mail_spool') - ->condition('nid', $nid) - ->condition('vid', $vid) - ->condition('status', $status); + ->condition('nid', $nid) + ->condition('vid', $vid) + ->where(implode(' OR ', $clauses)); return $query->countQuery()->execute()->fetchField(); } @@ -619,11 +702,13 @@ function _simplenews_set_from($category = NULL) { * * Converts some special HTML characters in addition to drupal_html_to_text() * - * @param string $text Source text with HTML and special characters + * @param string $text + * The source text with HTML and special characters. * @param boolean $inline_hyperlinks * TRUE: URLs will be placed inline. * FALSE: URLs will be converted to numbered reference list. - * @return string Target text with HTML and special characters replaced + * @return string + * The target text with HTML and special characters replaced. */ function simplenews_html_to_text($text, $inline_hyperlinks = TRUE) { // By replacing tag by only its URL the URLs will be placed inline @@ -706,8 +791,11 @@ function _simplenews_html_replace() { /** * Helper function to measure PHP execution time in microseconds. * - * @param bool $start TRUE reset the time and start counting. - * @return float: elapsed PHP execution time since start. + * @param bool $start + * If TRUE, reset the time and start counting. + * + * @return float + * The elapsed PHP execution time since the last start. */ function _simplenews_measure_usec($start = FALSE) { // Windows systems don't implement getrusage(). There is no alternative. diff --git a/simplenews.module b/simplenews.module index abf2a6c3b9f5057548028e6ac7ba71a940d32c5e..d2aa0fa5a468a094784d7cdc5e3433e2c081f690 100644 --- a/simplenews.module +++ b/simplenews.module @@ -58,6 +58,7 @@ define('SIMPLENEWS_STATUS_SEND_READY', 2); define('SIMPLENEWS_SPOOL_HOLD', 0); define('SIMPLENEWS_SPOOL_PENDING', 1); define('SIMPLENEWS_SPOOL_DONE', 2); +define('SIMPLENEWS_SPOOL_IN_PROGRESS', 3); /** * AFTER EACH 100 NEWSLETTERS @@ -845,7 +846,7 @@ function simplenews_simplenews_category_delete($category) { * Implementation of hook_form_FORM_ID_alter(). * * Add simplenews subscription fields to user register form. - * @todo: mode this function to another place in the module. + * @todo mode this function to another place in the module. */ function simplenews_form_user_register_form_alter(&$form, &$form_state) { $options = $default_value = $hidden = array(); @@ -1359,15 +1360,18 @@ function simplenews_subscribe_user($mail, $tid, $confirm = TRUE, $source = 'unkn * FALSE = The user is unsubscribed * TRUE = User receives an email to verify the address and complete the subscription cancellation * - * @param string $mail The email address to unsubscribe from the mailing list. - * @param integer $tid The term ID of the list. - * @param boolean $confirm TRUE = send confirmation mail; FALSE = unsubscribe immediate from the list + * @param string $mail + * The email address to unsubscribe from the mailing list. + * @param integer $tid + * The term ID of the list. + * @param boolean $confirm + * If TRUE, send a confirmation mail; if FALSE, unsubscribe immediately. * @param string $source - * Indication of the unsubscribe source. Simplenews uses these sources: - * website: via any website form (with or without confirmation email) - * mass subscribe: mass admin UI - * mass unsubscribe: mass admin UI - * action: Drupal actions + * Indicates the unsubscribe source. Simplenews uses these sources: + * - website: Via any website form (with or without confirmation email). + * - mass subscribe: Mass admin UI. + * - mass unsubscribe: Mass admin UI. + * - action: Drupal actions. */ function simplenews_unsubscribe_user($mail, $tid, $confirm = TRUE, $source = 'unknown') { $account = (object) array( @@ -1414,13 +1418,16 @@ function simplenews_unsubscribe_user($mail, $tid, $confirm = TRUE, $source = 'un /** * Check if the email address is subscribed to the given mailing list. * - * @param string $mail email address - * @param integer $tid mailing list id + * @param string $mail + * The email address to be checkd. + * @param integer $tid + * The mailing list id. * - * @return boolean TRUE = email address is subscribed to given mailing list id + * @return boolean + * TRUE if the email address is subscribed; otherwise false. * - * @todo only return active subscriptions. - * @todo This function can do without caching. simplenews_get_subscription() should cache. + * @todo Only return active subscriptions. + * @todo Caching should be done in simplenews_get_subscription(). */ function simplenews_user_is_subscribed($mail, $tid, $reset = FALSE) { static $subscribed = array(); @@ -1470,36 +1477,38 @@ function simplenews_subscriber_defaults($account = NULL) { * If the account is not subscribed a default subscription object is returned * containing all available account info. * - * @param object $account account details. Containing one or none of these items: - * object( - * snid : subscription id - * mail : email address - * uid : user id - * ) + * @param object $account + * The account details, which may contain the following properties: + * - snid: The subscription id. + * - mail: The email address. + * - uid: The user uid. * - * @return subscription object - * object( - * snid : subscription id. 0 if account is not subscribed - * tids : array of tid's of active subscriptions - * newsletter_subscriptions : array of newsletter subscription objects - * uid : user id. 0 if account is anonymous user - * mail : user email address. empty if email is unknown - * name : always empty. Added for compatibility with user account object - * language : language object. User preferred or default language - * ) + * @return object + * The subscription object, which contains the following properties: + * - snid: The subscription id, or 0 if the account is not subscribed. + * - tids: The array of tid's of active subscriptions for the account. + * - newsletter_subscriptions: The array of newsletter subscription objects. + * - uid: The user id, or 0 if the subscription is not associated with a user. + * - mail: The user email address, or empty if unknown. + * - name: An empty string added for compatibility with user account object. + * - language: The user-prefered or default language object. * - * @todo Consider changing the subscription object: - * subscribed : array of subscription objects of newsletters the user is subscribed to - * unsubscribed : array of subscription objects of newsletters the user is unsubscribed from - * Both arrays have newsletter ID (tid) as key. - * Drop 'tids' and 'newsletter_subscriptions' - * @todo Cache the $subscription + * @todo Consider adding the following properties to the return object: + * - subscribed: An array of subscription objects of newsletters to which the + * user is subscribed. + * - unsubscribed: An array of subscription objects of newsletters to which the + * user is not subscribed. + * Both arrays should have newsletter ID (tid) as the key. Then the 'tids' and + * 'newsletter_subscriptions' properties could be dropped. + * @todo Cache the $subscription object. * @todo Combine the two queries into one. - * @todo Load subscription object into tids[]. + * @todo Load the subscription object into the 'tids' array. */ function simplenews_get_subscription($account) { - // Load subscription data based on available account information - // NOTE that the order of checking for snid, mail and uid is critical. mail must be checked *before* uid. See simplenews_subscribe_user() + // Load subscription data based on available account information. + // NOTE that the order of checking for 'snid', 'mail' and 'uid' is critical. + // The 'mail' property must be checked *before* 'uid'. See + // simplenews_subscribe_user() $query = db_select('simplenews_subscriber', 's'); $query->leftJoin('users', 'u', 'u.uid = s.uid'); $query->fields('s') @@ -1536,7 +1545,8 @@ function simplenews_get_subscription($account) { $subscription->language = user_preferred_language($subscription)->language; } else { - // Account is unknown in subscription table. Create default subscription object + // The account was not found in the subscription table. + // Create and return a default subscription object. return simplenews_subscriber_defaults($account); } @@ -1598,7 +1608,7 @@ function simplenews_subscription_save($subscription) { * Delete subscriptions. * * @param $conditions - * Associative array of conditions matching the records to be delete. + * An associative array of conditions matching the records to be delete. * Example: array('tid' => 5, 'snid' => 12) * Delete the subscription of subscriber 12 to newsletter tid 5. */ @@ -1705,7 +1715,8 @@ function simplenews_subscriber_delete($subscriber) { /** * Build subscription manager form. * - * @param object $subscription subscription object + * @param object $subscription + * The subscription object. */ function _simplenews_subscription_manager_form($subscription) { $form = array(); @@ -1771,11 +1782,12 @@ function _simplenews_subscription_manager_form($subscription) { * Create a list of recent newsletters. * * @param integer $tid - * Newsletter category id + * The newsletter category id. * @param integer $count - * Number of newsletters + * The number of newsletters. * - * @todo Replace 'class' string by array: http://drupal.org/node/224333#class_attribute_array + * @todo Replace 'class' string by array. + * @see http://drupal.org/node/224333#class_attribute_array * @todo Replace this list by a View. */ function simplenews_recent_newsletters($tid, $count = 5) { @@ -1805,8 +1817,8 @@ function simplenews_recent_newsletters($tid, $count = 5) { * * @see simplenews_block_form_validate() * @see simplenews_block_form_submit() - * - * @todo Add $form to drupal_get_form() callback functions: http://drupal.org/node/224333#hook_forms_signature + * @todo Add $form to drupal_get_form() callback functions. + * @see http://drupal.org/node/224333#hook_forms_signature */ function simplenews_block_form($form, &$form_state, $tid) { global $user; @@ -1916,21 +1928,33 @@ function simplenews_block_form_submit($form, &$form_state) { * Send simplenews mails using drupal mail API * @see drupal_mail() * - * @param $key: node | test | subscribe | unsubscribe - * @param array $message message array - * [from] - * [headers][From] - * [language] : preferred message language - * @param array $params parameter array - * [context][node] : node object of message to be sent - * [context][snid] : used for $key = subscribe or unsubscribe - * [context][from_name] : name of mail sender or site name (optional) - * [context][account] : account details of recipient - * [from] : array('address' => 'noreply@example.org', 'formatted' => 'site name ') - * [newsletter] : newsletter object (tid, name) - * [tokens] : tokens for variable replacement. Defaults to: user_mail_tokens() - * - * @todo Replace drupal_clone: http://drupal.org/node/224333#drupal_clone + * @param string $key + * Must be one of: + * - node + * - test + * - subscribe + * - unsubscribe + * @param array $message + * The message array, containing at least the following keys: + * - from + * - headers: An array containing at least a 'From' key. + * - language: The preferred message language. + * @param array $params + * The parameter array, containing the following keys: + * - context: An array containing the following keys: + * - node: The node object of the message to be sent. + * - snid: Used when $key is 'subscribe' or 'unsubscribe'. + * - from_name: (optional) The mail sender or site name. + * - account: The recipient account details. + * - from: An array containing the following keys: + * - address: The sender email address, e.g., 'noreply@example.com'. + * - formatted: The formatted email address, e.g., + * '"site name" '. + * - newsletter: The newsletter object, containing the following properties: + * - tid + * - name + * - tokens: (optional) An array of tokens for variable replacement. Defaults + * to user_mail_tokens(). */ function simplenews_mail($key, &$message, $params) { $params['key'] = $key; @@ -2047,7 +2071,7 @@ function simplenews_build_node_mail(&$message, $params) { // Set the active language to the node's language. // This needs to be done as otherwise the language used to send the mail // is the language of the user logged in. - // @todo rewrite this code to drupal 7 + // @todo Rewrite this code for drupal 7. // if (module_exists('i18n')) { // i18n_selection_mode('node', $node->language); // } @@ -2088,7 +2112,7 @@ function simplenews_build_node_mail(&$message, $params) { } // Reset the language to the original settings. - // @todo rewrite this code to drupal 7 + // @todo Rewrite this code for drupal 7. // if (module_exists('i18n')) { // i18n_selection_mode('reset'); // } @@ -2451,9 +2475,9 @@ function simplenews_newsletter_delete($newsletter) { * @return string * Formatted Token information * - * @todo Add Token types description - * @todo Use Theme function - * @todo Is this available in Core?? + * @todo Add Token types description. + * @todo Use Theme function. + * @todo Is this available in Core? */ function _simplenews_get_token_info($modules) { $tokens = array(); @@ -2744,14 +2768,16 @@ function simplenews_help($path, $arg) { /** * Helper function to translate a newsletter name if required. * - * @param $newsletter - * Newsletter category object. Typically from simplenews_category_load(). - * $newsletter -> tid newsletter category id - * $newsletter -> name newsletter name - * @param $langcode - * Optional language code (defaults to current global $language); + * @param object $newsletter + * Newsletter category object, typically from simplenews_category_load(). + * Contains at least the following properties: + * - tid: The newsletter category id. + * - name: The newsletter name. + * @param string $langcode + * (optional) The language code. Defaults to $GLOBALS['language']. * - * @return translated newsletter name. + * @return string + * The translated newsletter name. */ function _simplenews_tt_newsletter_name($newsletter, $langcode = NULL) { // @todo Translate newsletter name -- 1.7.4.1