On a particular site I'm using the latest release of SMTP module to send emails and Queue Mail to queue them for sending. When everything's working fine it's great. However, I've noticed that if the message fails to be delivered that it logs this fact in watchog bug does not add the message back into the queue to be tried again later, instead the message just disappears. This obviously goes against one of the reasons to use a queue in the first place. I'm marking this as critical as it involves data loss, i.e. the emails disappear.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

Steven Jones’s picture

So i think that all we need to do is throw an exception if the mail fails to send, and then I think that the Drupal queue system should not remove the item from the queue.

jacobischwartz’s picture

Did you guys find a good solution to this?

jacobischwartz’s picture

Status: Active » Needs review
FileSize
1.44 KB

I've gone ahead and built a fix.

Attached is a patch that causes the queue worker to throw an exception. I've also added a try-catch around the drush operation, so that the queue can progress and Drupal can exit gracefully (which is the workflow in drupal_cron_run()).

A bit more information:
- The smtp module will only pass a send failure back to this module if you have that module's queue turned off. So if you're using the smtp module's queue, this module won't know about the failure and won't be able to re-process it.
- The Drupal queue system uses a lease lock on items when claiming them for worker functions. By throwing an exception, we are preventing the item from being deleted from the queue (either via cron run or drush). The item's lease then expires. At the next Drupal cron run, the expiry is cleared. Once the expiry is cleared, the queue claim method will pick up the item for processing again. So basically the item will be re-processed AFTER the next Drupal cron run, then AT the next queue processing moment. That could be either a cron run or a drush call.

amontero’s picture

Status: Needs review » Needs work

About the watchdog_exception call, from the watchdog() docs:

$type The category to which this message belongs. Can be any string, but the general practice is to use the name of the module calling watchdog().

Minor comment text nitpick:
// Log errors and throw exception so that failed item remains in queue.

Otherwise, I'm +1 on the idea. Throwing an exception in case of error is the way to go in queue worker code. If not done like this, failed messages simply get deleted from queue as if they were successfully processed, thus getting lost forever.

sinn’s picture

Status: Needs work » Needs review
FileSize
1.52 KB

Updated comment #4

Steven Jones’s picture

Status: Needs review » Needs work

@sinn any chance you can incorporate the suggestion in #5 into the patch?

sinn’s picture

Status: Needs work » Needs review

Actually I did it.

sinn’s picture

  • sinn committed cb00ef4 on 7.x-1.x
    Issue #2442467 by sinn, jacobischwartz: Emails fail to send, not added...
sinn’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.