Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use celery to send emails (and retry on failure) #3121

Merged
merged 16 commits into from
Oct 31, 2017

Conversation

ThiefMaster
Copy link
Member

@ThiefMaster ThiefMaster commented Oct 24, 2017

This also fixes #2981

@ThiefMaster ThiefMaster changed the title Use celery to send emails (and retry on failure) WIP: Use celery to send emails (and retry on failure) Oct 24, 2017
@ThiefMaster ThiefMaster force-pushed the emails-celery branch 5 times, most recently from 0748974 to cb5ee1b Compare October 25, 2017 07:46
do_send_email(email, log_entry)
except Exception as exc:
attempt = task.request.retries + 1
delay = (DELAYS + [0])[task.request.retries] if not config.DEBUG else 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why + [0]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the last failed attempt, task.request.retries is 9 so we still need to get a delay to pass to task.retry() to let it raise the MaxRetriesExceededError.

FWIW, we could just use custom code to handle giving up after 10 attempts, but I think it's cleaner to use the functionality already provided by celery.

@ThiefMaster ThiefMaster changed the title WIP: Use celery to send emails (and retry on failure) Use celery to send emails (and retry on failure) Oct 25, 2017
@ThiefMaster ThiefMaster force-pushed the emails-celery branch 5 times, most recently from 036ba53 to 3ee95b0 Compare October 31, 2017 12:54


def do_send_email(email, log_entry=None, _from_task=False):
"""Send an email"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe document the arguments?

@pferreir
Copy link
Member

A massive LGTM 👍

There doesn't seem to be any decent library that handles the quirks of
properly building emails out there and Django is BSD-licensed so we can
just vendorize the code...
In case the celery broker (redis) is down this may result in lost
emails, but this is a very unlikely case (especially since usually
sessions wouldn't work either with redis being down) and without
committing first, an event log entry may not be persisted in the
database yet when the task runs and wants to update its state.
Like this someone can retry sending it manually
During development few people run celery in the background and emails
usually go to maildump anyway.
In case of redis failures, the cache might not be available.
- if flushing failed, mark it as failed
- if celery is not used and the email was sent, mark it as sent
- commit either of these state updates
@ThiefMaster ThiefMaster merged commit 771e6ea into indico:master Oct 31, 2017
@ThiefMaster ThiefMaster deleted the emails-celery branch October 31, 2017 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix sending emails to non-ascii email addresses
4 participants