mirror of
https://github.com/zulip/zulip.git
synced 2026-07-09 21:21:47 +08:00
scheduled_email: Create ScheduledEmail objects in a transaction.
This fixes two bugs: the most obvious is that there is a race where a
ScheduledEmail object could be observed in the window between creation
and when users are added; this is a momentary instance when the object
has no users, but one that will resolve itself.
The more subtle is that .save() will, if no records were found to be
updated, _re-create_ the object as it exists in memory, using an
INSERT[1]. Thus, there is a race with `deliver_scheduled_emails`
between when the users are added, and when `email.save()` runs:
1. Web request creates ScheduledEmail object
2. Web request creates ScheduledEmailUsers object
3. deliver_scheduled_emails locks the former, preventing updates.
4. deliver_scheduled_emails deletes both objects, commits, releasing lock
5. Web request calls `email.save()`; UPDATE finds no rows, so it
re-creates the ScheduledEmail object.
6. Future deliver_scheduled_emails runs find a ScheduledEmail with no
attending ScheduledEmailUsers objects
Wrapping the logical creation of both of these in a single transaction
avoids both of these races.
[1] https://docs.djangoproject.com/en/3.2/ref/models/instances/#how-django-knows-to-update-vs-insert
This commit is contained in:
parent
717c26d82c
commit
9d97af6ebb
@ -345,27 +345,28 @@ def send_future_email(
|
||||
# For logging the email
|
||||
|
||||
assert (to_user_ids is None) ^ (to_emails is None)
|
||||
email = ScheduledEmail.objects.create(
|
||||
type=EMAIL_TYPES[template_name],
|
||||
scheduled_timestamp=timezone_now() + delay,
|
||||
realm=realm,
|
||||
data=orjson.dumps(email_fields).decode(),
|
||||
)
|
||||
with transaction.atomic():
|
||||
email = ScheduledEmail.objects.create(
|
||||
type=EMAIL_TYPES[template_name],
|
||||
scheduled_timestamp=timezone_now() + delay,
|
||||
realm=realm,
|
||||
data=orjson.dumps(email_fields).decode(),
|
||||
)
|
||||
|
||||
# We store the recipients in the ScheduledEmail object itself,
|
||||
# rather than the JSON data object, so that we can find and clear
|
||||
# them using clear_scheduled_emails.
|
||||
try:
|
||||
if to_user_ids is not None:
|
||||
email.users.add(*to_user_ids)
|
||||
else:
|
||||
assert to_emails is not None
|
||||
assert len(to_emails) == 1
|
||||
email.address = parseaddr(to_emails[0])[1]
|
||||
email.save()
|
||||
except Exception as e:
|
||||
email.delete()
|
||||
raise e
|
||||
# We store the recipients in the ScheduledEmail object itself,
|
||||
# rather than the JSON data object, so that we can find and clear
|
||||
# them using clear_scheduled_emails.
|
||||
try:
|
||||
if to_user_ids is not None:
|
||||
email.users.add(*to_user_ids)
|
||||
else:
|
||||
assert to_emails is not None
|
||||
assert len(to_emails) == 1
|
||||
email.address = parseaddr(to_emails[0])[1]
|
||||
email.save()
|
||||
except Exception as e:
|
||||
email.delete()
|
||||
raise e
|
||||
|
||||
|
||||
def send_email_to_admins(
|
||||
|
||||
Loading…
Reference in New Issue
Block a user