From 2601472bebbad4e9497638570be5c58effc5202d Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Tue, 4 Jun 2024 08:26:57 +0000 Subject: [PATCH] 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. --- zerver/lib/send_email.py | 29 +--- zerver/tests/test_send_email.py | 247 ++++++++++++++++++++++++++--- zproject/computed_settings.py | 2 +- zproject/default_settings.py | 1 + zproject/email_backends.py | 45 ++++++ zproject/prod_settings_template.py | 6 + 6 files changed, 275 insertions(+), 55 deletions(-) diff --git a/zerver/lib/send_email.py b/zerver/lib/send_email.py index e1f512aa58..3c06c1d1a6 100644 --- a/zerver/lib/send_email.py +++ b/zerver/lib/send_email.py @@ -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 diff --git a/zerver/tests/test_send_email.py b/zerver/tests/test_send_email.py index dc0ac5b6e4..8cfebc496f 100644 --- a/zerver/tests/test_send_email.py +++ b/zerver/tests/test_send_email.py @@ -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") diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index 04b65e9e80..f4a8032d86 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -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 diff --git a/zproject/default_settings.py b/zproject/default_settings.py index dba8557c1d..8699758b81 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -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 diff --git a/zproject/email_backends.py b/zproject/email_backends.py index 9651708242..b88cbcf4a4 100644 --- a/zproject/email_backends.py +++ b/zproject/email_backends.py @@ -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 diff --git a/zproject/prod_settings_template.py b/zproject/prod_settings_template.py index aff324dbce..a1ee7fd685 100644 --- a/zproject/prod_settings_template.py +++ b/zproject/prod_settings_template.py @@ -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