Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

send_email: Add setting to control max connection lifetime. 

Fixes #21900.
The default for the setting is set to 0, because the connection
keep-alive is an optimization that most small installs won't need.
When the setting is set to 0, we close the existing connection
as soon as we are done sending emails.
A lot of the connection opening and closing logic present in
send_email.py has been moved into the overridden methods in
PersistentSMTPEmailBackend.
The commit also removes the status=-1 logic and simplifies it a bit to
not have random statuses.
This commit is contained in:
Shubham Padia 2024-06-04 08:26:57 +00:00
parent eef65d7e30
commit 2601472beb
6 changed files with 275 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
zerver/lib/send_email.py
View File

@ -17,7 +17,6 @@ import orjson
from django.conf import settings
from django.core.mail import EmailMultiAlternatives, get_connection
from django.core.mail.backends.base import BaseEmailBackend
from django.core.mail.backends.smtp import EmailBackend
from django.core.mail.message import sanitize_address
from django.core.management import CommandError
from django.db import transaction
@ -34,7 +33,6 @@ from zerver.lib.logging_util import log_to_file
from zerver.models import Realm, ScheduledEmail, UserProfile
from zerver.models.scheduled_jobs import EMAIL_TYPES
from zerver.models.users import get_user_profile_by_id
from zproject.email_backends import EmailLogBackEnd, get_forward_address
if settings.ZILENCER_ENABLED:
from zilencer.models import RemoteZulipServer
@ -316,32 +314,7 @@ def initialize_connection(connection: BaseEmailBackend | None = None) -> BaseEma
connection = get_connection()
assert connection is not None
if connection.open():
# If it's a new connection, no need to no-op to check connectivity
return connection
if isinstance(connection, EmailLogBackEnd) and not get_forward_address():
# With the development environment backend and without a
# configured forwarding address, we don't actually send emails.
#
# As a result, the connection cannot be closed by the server
# (as there is none), and `connection.noop` is not
# implemented, so we need to return the connection early.
return connection
# No-op to ensure that we don't return a connection that has been
# closed by the mail server.
if isinstance(connection, EmailBackend):
try:
assert connection.connection is not None
status = connection.connection.noop()[0]
except Exception:
status = -1
if status != 250:
# Close and connect again.
connection.close()
connection.open()
connection.open()
return connection

247
zerver/tests/test_send_email.py
View File

@ -1,9 +1,12 @@
from datetime import timedelta
from smtplib import SMTP, SMTPDataError, SMTPException, SMTPRecipientsRefused
from unittest import mock
import time_machine
from django.core.mail.backends.locmem import EmailBackend
from django.core.mail.backends.smtp import EmailBackend as SMTPBackend
from django.core.mail.message import sanitize_address
from django.test import override_settings
from django.utils.timezone import now as timezone_now
from zerver.lib.send_email import (
EmailNotDeliveredError,
@ -14,6 +17,7 @@ from zerver.lib.send_email import (
send_email,
)
from zerver.lib.test_classes import ZulipTestCase
from zproject.email_backends import PersistentSMTPEmailBackend
class TestBuildEmail(ZulipTestCase):
@ -70,39 +74,230 @@ class TestBuildEmail(ZulipTestCase):
class TestSendEmail(ZulipTestCase):
@override_settings(EMAIL_MAX_CONNECTION_LIFETIME_IN_MINUTES=None)
@time_machine.travel(timezone_now(), tick=False)
def test_initialize_connection(self) -> None:
# Test the new connection case
with mock.patch.object(EmailBackend, "open", return_value=True):
backend = initialize_connection(None)
self.assertTrue(isinstance(backend, EmailBackend))
backend = mock.MagicMock(spec=SMTPBackend)
backend.connection = mock.MagicMock(spec=SMTP)
backend = PersistentSMTPEmailBackend()
backend.open()
self.assertEqual(backend.opened_at, timezone_now())
self.assertTrue(isinstance(backend, SMTPBackend))
# Test the old connection case when it is still open
backend.open.return_value = False
backend.connection.noop.return_value = [250]
initialize_connection(backend)
self.assertEqual(backend.open.call_count, 1)
self.assertEqual(backend.connection.noop.call_count, 1)
# Test the old connection case when it was closed by the server
backend.connection.noop.return_value = [404]
backend.close.return_value = False
initialize_connection(backend)
# 2 more calls to open, 1 more call to noop and 1 call to close
self.assertEqual(backend.open.call_count, 3)
self.assertEqual(backend.connection.noop.call_count, 2)
self.assertEqual(backend.close.call_count, 1)
# Test backoff procedure
backend.open.side_effect = OSError
with self.assertRaises(OSError):
with (
mock.patch.object(backend, "_open", wraps=backend._open) as _open_call,
mock.patch.object(backend, "close", wraps=backend.close) as close_call,
):
# In case of an exception when trying to do the noop
# operation, the connection will open and close.
# 1 call to _open is to check whether the connection is
# open and second is to actually open the connection.
backend.connection = mock.MagicMock(spec=SMTP)
backend.connection.noop.side_effect = AssertionError
initialize_connection(backend)
# 3 more calls to open as we try 3 times before giving up
self.assertEqual(backend.open.call_count, 6)
self.assertEqual(_open_call.call_count, 2)
self.assertEqual(close_call.call_count, 1)
close_call.reset_mock()
_open_call.reset_mock()
backend.connection = mock.MagicMock(spec=SMTP)
backend.connection.noop.return_value = [250]
initialize_connection(backend)
self.assertEqual(_open_call.call_count, 1)
self.assertEqual(backend.connection.noop.call_count, 1)
close_call.reset_mock()
_open_call.reset_mock()
# Test the old connection case when it was closed by the server
backend.connection.noop.return_value = [404]
initialize_connection(backend)
# 2 calls to open and 1 call to close
self.assertEqual(_open_call.call_count, 2)
self.assertEqual(close_call.call_count, 1)
close_call.reset_mock()
_open_call.reset_mock()
# Test backoff procedure
_open_call.side_effect = OSError
with self.assertRaises(OSError):
initialize_connection(backend)
# 3 calls to open as we try 3 times before giving up
self.assertEqual(_open_call.call_count, 3)
@time_machine.travel(timezone_now(), tick=False)
@override_settings(EMAIL_MAX_CONNECTION_LIFETIME_IN_MINUTES=0)
def test_max_connection_lifetime_zero(self) -> None:
backend = PersistentSMTPEmailBackend()
with (
mock.patch.object(backend, "_open", wraps=backend._open) as _open_call,
mock.patch.object(backend, "close", wraps=backend.close) as close_call,
):
backend.open()
backend.connection = mock.MagicMock(spec=SMTP)
self.assertEqual(close_call.call_count, 0)
self.assertEqual(_open_call.call_count, 1)
self.assertEqual(backend.connection.noop.call_count, 0)
self.assertEqual(backend.opened_at, timezone_now())
close_call.reset_mock()
_open_call.reset_mock()
# Old connection is open, but we still reopen a new
# connection because max connection lifetime is 0.
# 1 call to _open is to check whether the connection is
# open and second is to actually open the connection.
initialize_connection(backend)
self.assertEqual(close_call.call_count, 1)
self.assertEqual(_open_call.call_count, 2)
self.assertEqual(backend.opened_at, timezone_now())
close_call.reset_mock()
_open_call.reset_mock()
backend.close()
close_call.reset_mock()
# Old connection is closed, but we still reopen a new
# connection because max connection lifetime is 0.
initialize_connection(backend)
self.assertEqual(close_call.call_count, 0)
self.assertEqual(_open_call.call_count, 1)
self.assertEqual(backend.opened_at, timezone_now())
close_call.reset_mock()
_open_call.reset_mock()
hamlet = self.example_user("hamlet")
with (
self.settings(EMAIL_HOST_USER="test", EMAIL_HOST_PASSWORD=""),
mock.patch.object(backend, "_send", return_value=True),
):
send_email(
"zerver/emails/password_reset",
to_emails=[hamlet.email],
from_name="From Name",
from_address=FromAddress.NOREPLY,
language="en",
connection=backend,
)
# 1 call to close in open() and 1 after sending the message
# because max connection lifetime is 0. 1 call to open in
# open() just after close() and then send_messages calls
# open one time.
self.assertEqual(close_call.call_count, 2)
self.assertEqual(_open_call.call_count, 2)
@time_machine.travel(timezone_now(), tick=False)
@override_settings(EMAIL_MAX_CONNECTION_LIFETIME_IN_MINUTES=None)
def test_max_connection_lifetime_none(self) -> None:
backend = PersistentSMTPEmailBackend()
with (
mock.patch.object(backend, "_open", wraps=backend._open) as _open_call,
mock.patch.object(backend, "close", wraps=backend.close) as close_call,
):
backend.open()
backend.connection = mock.MagicMock(spec=SMTP)
backend.connection.noop.return_value = [250]
self.assertEqual(close_call.call_count, 0)
self.assertEqual(_open_call.call_count, 1)
self.assertEqual(backend.opened_at, timezone_now())
close_call.reset_mock()
_open_call.reset_mock()
# Old connection is open, we will not open a new connection because max connection lifetime is None.
with time_machine.travel(
timezone_now() + timedelta(minutes=10),
tick=False,
):
initialize_connection(backend)
self.assertEqual(close_call.call_count, 0)
# open.call_count is 1 because we are calling connection.open()
# to check whether an open connection already exists. In case of actually opening
# a new connection, the call count will be 2.
self.assertEqual(_open_call.call_count, 1)
self.assertEqual(backend.connection.noop.call_count, 1)
close_call.reset_mock()
_open_call.reset_mock()
backend.connection.noop.reset_mock()
hamlet = self.example_user("hamlet")
with (
self.settings(
EMAIL_HOST_USER="test",
EMAIL_HOST_PASSWORD="",
),
time_machine.travel(
timezone_now() + timedelta(minutes=10),
tick=False,
),
mock.patch.object(backend, "_send", return_value=True),
):
send_email(
"zerver/emails/password_reset",
to_emails=[hamlet.email],
from_name="From Name",
from_address=FromAddress.NOREPLY,
language="en",
connection=backend,
)
# No call to close after sending the message because max
# connection lifetime is None. 1 call to open because
# send_messages calls open one time.
self.assertEqual(close_call.call_count, 0)
self.assertEqual(_open_call.call_count, 1)
self.assertEqual(backend.connection.noop.call_count, 1)
@time_machine.travel(timezone_now(), tick=False)
@override_settings(EMAIL_MAX_CONNECTION_LIFETIME_IN_MINUTES=5)
def test_max_connection_lifetime_5_minutes(self) -> None:
backend = PersistentSMTPEmailBackend()
with (
mock.patch.object(backend, "_open", wraps=backend._open) as _open_call,
mock.patch.object(backend, "close", wraps=backend.close) as close_call,
):
backend.open()
self.assertEqual(close_call.call_count, 0)
self.assertEqual(_open_call.call_count, 1)
self.assertEqual(backend.opened_at, timezone_now())
close_call.reset_mock()
_open_call.reset_mock()
backend.connection = mock.MagicMock(spec=SMTP)
backend.connection.noop.return_value = [250]
# Old connection is open, we will not open a new connection because not enough time has elapsed.
with time_machine.travel(
timezone_now() + timedelta(minutes=3),
tick=False,
):
initialize_connection(backend)
self.assertEqual(close_call.call_count, 0)
# open.call_count is 1 because we are calling connection.open()
# to check whether an open connection already exists. In case of actually opening
# a new connection, the call count will be 2.
self.assertEqual(_open_call.call_count, 1)
self.assertEqual(backend.connection.noop.call_count, 1)
close_call.reset_mock()
_open_call.reset_mock()
backend.connection.noop.reset_mock()
# Old connection is open, we will open a new connection because max time has elapsed.
with (
time_machine.travel(
timezone_now() + timedelta(minutes=6),
tick=False,
),
):
self.assertEqual(backend.opened_at, timezone_now() - timedelta(minutes=6))
initialize_connection(backend)
self.assertEqual(backend.opened_at, timezone_now())
self.assertEqual(close_call.call_count, 1)
self.assertEqual(_open_call.call_count, 2)
def test_send_email_exceptions(self) -> None:
hamlet = self.example_user("hamlet")

2
zproject/computed_settings.py
View File

@ -1226,7 +1226,7 @@ elif not EMAIL_HOST:
WARN_NO_EMAIL = True
EMAIL_BACKEND = "django.core.mail.backends.dummy.EmailBackend"
else:
EMAIL_BACKEND = "django.core.mail.backends.smtp.EmailBackend"
EMAIL_BACKEND = "zproject.email_backends.PersistentSMTPEmailBackend"
EMAIL_TIMEOUT = 15

1
zproject/default_settings.py
View File

@ -39,6 +39,7 @@ TOKENIZED_NOREPLY_EMAIL_ADDRESS = Address(
).addr_spec
PHYSICAL_ADDRESS = ""
FAKE_EMAIL_DOMAIN = EXTERNAL_HOST_WITHOUT_PORT
EMAIL_MAX_CONNECTION_LIFETIME_IN_MINUTES: int | None = 0
# SMTP settings
EMAIL_HOST: str | None = None

45
zproject/email_backends.py
View File

@ -3,12 +3,14 @@ import configparser
import logging
from collections.abc import MutableSequence, Sequence
from email.message import Message
from typing import Any
from django.conf import settings
from django.core.mail import EmailMultiAlternatives
from django.core.mail.backends.smtp import EmailBackend
from django.core.mail.message import EmailMessage
from django.template import loader
from django.utils.timezone import now as timezone_now
from typing_extensions import override
@ -108,3 +110,46 @@ class EmailLogBackEnd(EmailBackend):
email_log_url = settings.ROOT_DOMAIN_URI + "/emails"
logging.info("Emails sent in development are available at %s", email_log_url)
return num_sent
class PersistentSMTPEmailBackend(EmailBackend):
def _open(self, **kwargs: Any) -> bool | None:
is_opened = super().open()
if is_opened:
self.opened_at = timezone_now()
return True
return is_opened
@override
def open(self, **kwargs: Any) -> bool | None:
is_opened = self._open()
if is_opened:
return True
status = None
time_elapsed = (timezone_now() - self.opened_at).seconds / 60
if settings.EMAIL_MAX_CONNECTION_LIFETIME_IN_MINUTES != 0 and (
settings.EMAIL_MAX_CONNECTION_LIFETIME_IN_MINUTES is None
or time_elapsed <= settings.EMAIL_MAX_CONNECTION_LIFETIME_IN_MINUTES
):
# No-op to ensure that we don't return a connection that has been
# closed by the mail server.
try:
assert self.connection is not None
status = self.connection.noop()[0]
except Exception:
pass
# In case of EMAIL_MAX_CONNECTION_LIFETIME_IN_MINUTES = 0, we
# will always return a newly opened connection. The default
# smtp.py implementation of send_messages closes the connection
# after sending the emails if the connection was a new
# connection i.e. is_opened is True.
if status is None or status != 250:
# Close and connect again.
self.close()
is_opened = self._open()
return is_opened

6
zproject/prod_settings_template.py
View File

@ -86,6 +86,12 @@ EXTERNAL_HOST = "zulip.example.com"
# EMAIL_USE_TLS = True
# EMAIL_PORT = 587
# If an SMTP connection has been open longer than this threshold, we will
# close the old connection and reopen a new one. If the value is None, we
# will never close the connection. If the value is 0, we will open and close
# SMTP connections as needed.
# EMAIL_MAX_CONNECTION_LIFETIME_IN_MINUTES = 0
## The noreply address to be used as the sender for certain generated
## emails. Messages sent to this address could contain sensitive user
## data and should not be delivered anywhere. The default is