#21271 Django's usage of smtplib.SMTP should have a timeout #1758

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

ArcTanSusan commented Oct 16, 2013

https://code.djangoproject.com/ticket/21271

Tests pass. I added a short doc to 1.7 release notes and to https://docs.djangoproject.com/en/dev/topics/email/#smtp-backend. Do feel free to review.

Owner

timgraham commented Oct 16, 2013

We're actually trying to remove settings, not add more. :-)

For example, see #1614

An alternate approach we've been working toward is to allow users to subclass to override attributes. It looks like we could do that with EmailBackend here.

@timgraham timgraham and 1 other commented on an outdated diff Oct 17, 2013

django/core/mail/backends/smtp.py
super(EmailBackend, self).__init__(fail_silently=fail_silently)
self.host = host or settings.EMAIL_HOST
self.port = port or settings.EMAIL_PORT
+ self.timeout = 60
@timgraham

timgraham Oct 17, 2013

Owner

The idea would be to make this a class attribute so that it can be overridden by overriding EmailBackend, for example

class MyEmailBackend(EmailBackend):
    timeout = 120

settings.EMAIL_BACKEND = 'path.to.MyEmailBackend'

You can add documentation in this section https://docs.djangoproject.com/en/dev/topics/email/#smtp-backend
A mention in the release notes is also needed.

@ArcTanSusan

ArcTanSusan Oct 17, 2013

Contributor

Thanks for clarifying the use case of this class attribute self.timeout=60. Is there anything I need to fix up in the code itself? I still need to add docs to that section and in release notes.

@timgraham

timgraham Oct 17, 2013

Owner

Instead of putting timeout in the __init__ method, make it a class attribute. Here's an example change where we use this same technique: 8b00148

@ArcTanSusan

ArcTanSusan Oct 17, 2013

Contributor

Made the change. Is the test appropriate for the change?

@timgraham

timgraham Oct 17, 2013

Owner

I think the test fails as it is as you're no longer passing self.timeout to smtplib.SMTP_SSL.

I don't think Django should specify a default of 60. Instead it should be None and only passed to the SMTP connection if the user specifies it.

Here's a quick sketch of what I have in mind (plus some cleanup):

    # If local_hostname is not specified, socket.getfqdn() gets used.
    # For performance, we use the cached FQDN for local_hostname.
    connection_class = smtplib.SMTP_SSL if self.use_ssl else smtplib.SMTP
    connection_params = {
        'local_hostname': DNS_NAME.get_fqdn(),
    }
    if self.timeout is not None:
        connection_params['timeout'] = self.timeout

    self.connection = connection_class(self.host, self.port, **connection_params)

    # TLS/SSL are mutually exclusive, so only attempt TLS over
    # non-secure connections.
    if not self.use_ssl and self.use_tls:
        self.connection.ehlo()
        self.connection.starttls()
        self.connection.ehlo()

For the test you'd subclass EmailBackend and verify the connection has the timeout use specified.

@timgraham

timgraham Oct 18, 2013

Owner

Susan, your latest PR doeesn't seem to take the above suggestion into account.

@timgraham

timgraham Oct 19, 2013

Owner

You should set self.timeout to timeout, not 60.

Owner

timgraham commented Oct 17, 2013

re: "As for the decision to remove the global settings, was it made in order to avoid dependency on global variables?"
Yes, that's my understanding.

@tobiasmcnulty tobiasmcnulty commented on an outdated diff Oct 17, 2013

django/core/mail/backends/smtp.py
def __init__(self, host=None, port=None, username=None, password=None,
- use_tls=None, fail_silently=False, use_ssl=None, **kwargs):
+ use_tls=None, fail_silently=False, use_ssl=None,
+ **kwargs):
@tobiasmcnulty

tobiasmcnulty Oct 17, 2013

Member

Where is kwargs used?

@tobiasmcnulty tobiasmcnulty and 2 others commented on an outdated diff Oct 17, 2013

django/core/mail/backends/smtp.py
class EmailBackend(BaseEmailBackend):
"""
A wrapper that manages the SMTP network connection.
"""
+
+ # These are class-level attribute, so that they can be easily overriden.
+ timeout = 60
@tobiasmcnulty

tobiasmcnulty Oct 17, 2013

Member

You probably don't want this to be a class-level attribute, because they are exactly that (class-level, not object-level). The better approach is to accept an optional timeout argument to __init__ and store that on self to later be passed in to smtplib.SMTP.

@timgraham

timgraham Oct 17, 2013

Owner

The idea is to subclass EmailBackend, for example

class MyEmailBackend(EmailBackend):
    timeout = 120

settings.EMAIL_BACKEND = 'path.to.MyEmailBackend'

do you see a problem with this approach? I'm not sure how the _init__ approach would work exactly?

@tobiasmcnulty

tobiasmcnulty Oct 17, 2013

Member

Since integers are immutable it's probably not actually an issue in this case, but for future reference the problem is this:

http://stackoverflow.com/questions/207000/python-difference-between-class-and-instance-attributes

Nonetheless, I'd probably do it like this just to keep all the configuration done the same way throughout the EmailBackend class:

class EmailBackend(BaseEmailBackend):
   def __init__(self, host=None, port=None, username=None, password=None,
                 use_tls=None, fail_silently=False, use_ssl=None, timeout=None):
      self.timeout = timeout
      # ...

and then:

class MyEmailBackend(EmailBackend):
    def __init__(*args, **kwargs):
        kwargs['timeout'] = 60
        super(MyEmailBackend, self).__init__(*args, **kwargs)

I think the better, long term solution is probably to collapse all the EMAIL_* settings into a single dictionary (a la DATABASES) that allows passing arbitrary arguments to __init__, and this way of doing it would keep the door open for that change without changing the API.

@timgraham

timgraham Oct 17, 2013

Owner

Ok, I'm +1 on your approach. Thank-you very much for the review and feedback.

@tobiasmcnulty

tobiasmcnulty Oct 17, 2013

Member

Thank you for the contribution & taking on the ticket! 😺

@tobiasmcnulty

tobiasmcnulty Oct 17, 2013

Member

Whoops, just realized the original author was someone else! Sorry about that. Nonetheless, thanks for discussing!

@ArcTanSusan

ArcTanSusan Oct 17, 2013

Contributor

I've been following the discussion. Being a junior dev, I am not entirely familiar with all the trade-offs of either approach. Since the consensus seems to use a __init__ approach, I will go implement that. I will also attempt the better, long-term solution that @tobiasmcnulty suggested and will ask more questions here if I get stuck.

@timgraham

timgraham Oct 17, 2013

Owner

Sounds good, Susan, although I would caution that the second item may not be straightforward and should obviously be a separate ticket. @ramiro tried to covert the COOKIE settings into a dictionary and ran into some issues. see https://code.djangoproject.com/ticket/21051 for details.

@ArcTanSusan

ArcTanSusan Oct 18, 2013

Contributor

See updated PR. I've added 2 more tests; feel free to give me feedback.

@timgraham timgraham commented on an outdated diff Oct 18, 2013

tests/mail/tests.py
@@ -278,6 +278,22 @@ def test_dummy_backend(self):
email = EmailMessage('Subject', 'Content', 'bounce@example.com', ['to@example.com'], headers={'From': 'from@example.com'})
self.assertEqual(connection.send_messages([email, email, email]), 3)
+ def test_connection_timeout_value(self):
+ """ Test that the connection's hard-coded timeout value is 60 seconds."""
+ connection = mail.get_connection('django.core.mail.backends.smtp.EmailBackend')
+ self.assertEqual(connection.timeout, 60)
+
+ # Test two sample use cases. Timeout value can get overwritten in 2 ways: class attribute and __init__ attribute.
+ class MyEmailBackend1(smtp.EmailBackend):
+ timeout = 61
+ self.assertEqual(MyEmailBackend1.timeout, 61)
+
+ class MyEmailBackend2(smtp.EmailBackend):
+ def __init__(self):
+ self.timeout = 42
@timgraham

timgraham Oct 18, 2013

Owner

You need to call super() in __init__() as noted in Tobias' comment.

@timgraham

timgraham Oct 19, 2013

Owner

As suggested by Tobias, it should be:

class MyEmailBackend(EmailBackend):
    def __init__(*args, **kwargs):
        kwargs['timeout'] = 60
        super(MyEmailBackend, self).__init__(*args, **kwargs)

@timgraham timgraham commented on an outdated diff Oct 18, 2013

tests/mail/tests.py
@@ -278,6 +278,22 @@ def test_dummy_backend(self):
email = EmailMessage('Subject', 'Content', 'bounce@example.com', ['to@example.com'], headers={'From': 'from@example.com'})
self.assertEqual(connection.send_messages([email, email, email]), 3)
+ def test_connection_timeout_value(self):
+ """ Test that the connection's hard-coded timeout value is 60 seconds."""
+ connection = mail.get_connection('django.core.mail.backends.smtp.EmailBackend')
+ self.assertEqual(connection.timeout, 60)
+
+ # Test two sample use cases. Timeout value can get overwritten in 2 ways: class attribute and __init__ attribute.
+ class MyEmailBackend1(smtp.EmailBackend):
@timgraham

timgraham Oct 18, 2013

Owner

I don't think we should suggest doing it this way, hence I'd suggest removing this test.

@timgraham timgraham commented on an outdated diff Oct 18, 2013

django/core/mail/backends/smtp.py
@@ -9,16 +9,17 @@
from django.core.mail.message import sanitize_address
from django.utils.encoding import force_bytes
-
@timgraham

timgraham Oct 18, 2013

Owner

2 spaces above class names is correct -- shouldn't remove this newline.

@timgraham timgraham commented on an outdated diff Oct 19, 2013

tests/mail/tests.py
@@ -278,6 +278,19 @@ def test_dummy_backend(self):
email = EmailMessage('Subject', 'Content', 'bounce@example.com', ['to@example.com'], headers={'From': 'from@example.com'})
self.assertEqual(connection.send_messages([email, email, email]), 3)
+ def test_connection_timeout_value(self):
+ """ Test that the connection's hard-coded timeout value is 60 seconds."""
+ connection = mail.get_connection('django.core.mail.backends.smtp.EmailBackend')
+ self.assertEqual(connection.timeout, 60)
+
+ # Test a sample use case. Timeout value can get overwritten by using the __init__ attribute.
+ class MyEmailBackend(smtp.EmailBackend):
+ def __init__(*args, **kwargs):
@timgraham

timgraham Oct 19, 2013

Owner

this line should be: def __init__(self, *args, **kwargs):

@timgraham timgraham and 1 other commented on an outdated diff Oct 21, 2013

django/core/mail/backends/smtp.py
@@ -35,6 +37,11 @@ def open(self):
Ensures we have a connection to the email server. Returns whether or
not a new connection was required (True or False).
"""
+ connection_params = {
+ 'local_hostname': DNS_NAME.get_fqdn(),
+ }
+ if self.timeout is not None:
+ connection_params['timeout'] = self.timeout
@timgraham

timgraham Oct 21, 2013

Owner

I see you incorporated some of my suggestion as to how to refactor this, but as it is now the connection_params variable isn't used.

@ArcTanSusan

ArcTanSusan Oct 24, 2013

Contributor

Should connection_params be passed as an input to self.connection, which is assigned to smtplib.SMTP_SSL?

@timgraham

timgraham Oct 24, 2013

Owner

Right, here's a repeat of my comment earlier:

    # If local_hostname is not specified, socket.getfqdn() gets used.
    # For performance, we use the cached FQDN for local_hostname.
    connection_class = smtplib.SMTP_SSL if self.use_ssl else smtplib.SMTP
    connection_params = {
        'local_hostname': DNS_NAME.get_fqdn(),
    }
    if self.timeout is not None:
        connection_params['timeout'] = self.timeout

    self.connection = connection_class(self.host, self.port, **connection_params)

    # TLS/SSL are mutually exclusive, so only attempt TLS over
    # non-secure connections.
    if not self.use_ssl and self.use_tls:
        self.connection.ehlo()
        self.connection.starttls()
        self.connection.ehlo()

@timgraham timgraham commented on an outdated diff Oct 21, 2013

docs/releases/1.7.txt
@@ -239,6 +239,7 @@ Email
* :func:`~django.core.mail.send_mail` now accepts an ``html_message``
parameter for sending a multipart ``text/plain`` and ``text/html`` email.
+* :func:`~django.core.mail.backends.smtp.EmailBackend` now accepts a configurable ``timeout`` parameter.
@timgraham

timgraham Oct 21, 2013

Owner

wrap text at 80 characters

@timgraham timgraham commented on an outdated diff Oct 21, 2013

docs/topics/email.txt
@@ -428,7 +428,8 @@ This is the default backend. Email will be sent through a SMTP server.
The server address and authentication credentials are set in the
:setting:`EMAIL_HOST`, :setting:`EMAIL_PORT`, :setting:`EMAIL_HOST_USER`,
:setting:`EMAIL_HOST_PASSWORD`, :setting:`EMAIL_USE_TLS` and
-:setting:`EMAIL_USE_SSL` settings in your settings file.
+:setting:`EMAIL_USE_SSL` settings in your settings file. This backend contains a configurable `timeout`
@timgraham

timgraham Oct 21, 2013

Owner

wrap text at 80 characters. Also, a usage example (as in the test) would be helpful here.

@timgraham timgraham and 1 other commented on an outdated diff Oct 24, 2013

django/core/mail/backends/smtp.py
@@ -43,10 +63,12 @@ def open(self):
# For performance, we use the cached FQDN for local_hostname.
@timgraham

timgraham Oct 24, 2013

Owner

the code above in green should replace the code here -- it's a refactor of it (you'll see the duplicate comments).

@ArcTanSusan

ArcTanSusan Oct 24, 2013

Contributor

Ah, that is indeed illuminating! I should have read this more carefully. Cool refactor.

@timgraham timgraham commented on an outdated diff Oct 24, 2013

docs/topics/email.txt
@@ -430,6 +430,14 @@ The server address and authentication credentials are set in the
:setting:`EMAIL_HOST_PASSWORD`, :setting:`EMAIL_USE_TLS` and
:setting:`EMAIL_USE_SSL` settings in your settings file.
+This backend contains a configurable `timeout` parameter, which can be overwritten
@timgraham

timgraham Oct 24, 2013

Owner

missing ..versionadded:: 1.7 to identify the new functionality

@timgraham timgraham commented on an outdated diff Oct 24, 2013

docs/topics/email.txt
@@ -430,6 +430,14 @@ The server address and authentication credentials are set in the
:setting:`EMAIL_HOST_PASSWORD`, :setting:`EMAIL_USE_TLS` and
:setting:`EMAIL_USE_SSL` settings in your settings file.
+This backend contains a configurable `timeout` parameter, which can be overwritten
+with the following sample code::
+
+ class MyEmailBackend(smtp.EmailBackend):
+ def __init__(self,*args, **kwargs):
+ kwargs['timeout'] = 42
+ super(MyEmailBackend, self).__init__(self,*args, **kwargs)
@timgraham

timgraham Oct 24, 2013

Owner

missing comma after self, (same thing two lines above)

@timgraham timgraham commented on an outdated diff Oct 24, 2013

docs/topics/email.txt
@@ -430,6 +430,14 @@ The server address and authentication credentials are set in the
:setting:`EMAIL_HOST_PASSWORD`, :setting:`EMAIL_USE_TLS` and
:setting:`EMAIL_USE_SSL` settings in your settings file.
+This backend contains a configurable `timeout` parameter, which can be overwritten
+with the following sample code::
+
+ class MyEmailBackend(smtp.EmailBackend):
+ def __init__(self,*args, **kwargs):
+ kwargs['timeout'] = 42
@timgraham

timgraham Oct 24, 2013

Owner

missing indentation for this method

@timgraham

timgraham Oct 24, 2013

Owner

I'm actually going to suggest modifying this to: kwargs.setdefault('timeout', 42). That way if someone uses MyEmailBackend and wants to customize the timeout, they will be able to. You'll need to update line 291 in the test not to pass timeout=None since that would now override 42.

@timgraham timgraham commented on an outdated diff Oct 24, 2013

tests/mail/tests.py
@@ -278,6 +278,19 @@ def test_dummy_backend(self):
email = EmailMessage('Subject', 'Content', 'bounce@example.com', ['to@example.com'], headers={'From': 'from@example.com'})
self.assertEqual(connection.send_messages([email, email, email]), 3)
+ def test_connection_timeout_value(self):
+ """ Test that the connection's timeout value is initially None. Test that timeout can be overwritten."""
+ connection = mail.get_connection('django.core.mail.backends.smtp.EmailBackend')
+ self.assertEqual(connection.timeout, None)
+
+ # Test a sample use case. Timeout value can get overwritten by using the __init__ attribute.
+ class MyEmailBackend(smtp.EmailBackend):
+ def __init__(self,*args, **kwargs):
@timgraham

timgraham Oct 24, 2013

Owner

missing space after self, as described above (and two lines below)

@timgraham timgraham commented on an outdated diff Oct 24, 2013

tests/mail/tests.py
@@ -278,6 +278,19 @@ def test_dummy_backend(self):
email = EmailMessage('Subject', 'Content', 'bounce@example.com', ['to@example.com'], headers={'From': 'from@example.com'})
self.assertEqual(connection.send_messages([email, email, email]), 3)
+ def test_connection_timeout_value(self):
+ """ Test that the connection's timeout value is initially None. Test that timeout can be overwritten."""
+ connection = mail.get_connection('django.core.mail.backends.smtp.EmailBackend')
+ self.assertEqual(connection.timeout, None)
+
+ # Test a sample use case. Timeout value can get overwritten by using the __init__ attribute.
+ class MyEmailBackend(smtp.EmailBackend):
+ def __init__(self,*args, **kwargs):
+ kwargs['timeout'] = 42
+ super(MyEmailBackend, self).__init__(self,*args, **kwargs)
@timgraham

timgraham Oct 24, 2013

Owner

please add a newline after this

@timgraham timgraham commented on an outdated diff Oct 24, 2013

docs/releases/1.7.txt
@@ -239,6 +239,8 @@ Email
* :func:`~django.core.mail.send_mail` now accepts an ``html_message``
parameter for sending a multipart ``text/plain`` and ``text/html`` email.
+* :func:`~django.core.mail.backends.smtp.EmailBackend` now accepts a
+configurable ``timeout`` parameter.
@timgraham

timgraham Oct 24, 2013

Owner

need to indent this as is done 2 lines above in order for sphinx to build the docs properly.

@timgraham timgraham commented on an outdated diff Oct 24, 2013

django/core/mail/backends/smtp.py
+ connection_class = smtplib.SMTP_SSL if self.use_ssl else smtplib.SMTP
+ connection_params = {
+ 'local_hostname': DNS_NAME.get_fqdn(),
+ }
+ if self.timeout is not None:
+ connection_params['timeout'] = self.timeout
+
+ self.connection = connection_class(self.host, self.port, **connection_params)
+
+ # TLS/SSL are mutually exclusive, so only attempt TLS over
+ # non-secure connections.
+ if not self.use_ssl and self.use_tls:
+ self.connection.ehlo()
+ self.connection.starttls()
+ self.connection.ehlo()
+
if self.connection:
@timgraham

timgraham Oct 24, 2013

Owner

this should appear above the code in green as it does in the original file.

@timgraham timgraham commented on an outdated diff Oct 24, 2013

django/core/mail/backends/smtp.py
@@ -35,30 +37,27 @@ def open(self):
Ensures we have a connection to the email server. Returns whether or
not a new connection was required (True or False).
"""
+ # If local_hostname is not specified, socket.getfqdn() gets used.
@timgraham

timgraham Oct 24, 2013

Owner

deindent comment

@timgraham timgraham commented on an outdated diff Oct 24, 2013

django/core/mail/backends/smtp.py
if self.connection:
# Nothing to do if the connection is already open.
return False
- try:
@timgraham

timgraham Oct 24, 2013

Owner

the try/except needs to be added back

@timgraham timgraham commented on the diff Oct 24, 2013

docs/topics/email.txt
@@ -430,6 +430,15 @@ The server address and authentication credentials are set in the
:setting:`EMAIL_HOST_PASSWORD`, :setting:`EMAIL_USE_TLS` and
:setting:`EMAIL_USE_SSL` settings in your settings file.
+.. versionadded:: 1.7
@timgraham

timgraham Oct 24, 2013

Owner

newline after this (I believe you'd see a sphinx error if you tried building docs)

@timgraham timgraham commented on an outdated diff Oct 24, 2013

docs/topics/email.txt
@@ -430,6 +430,15 @@ The server address and authentication credentials are set in the
:setting:`EMAIL_HOST_PASSWORD`, :setting:`EMAIL_USE_TLS` and
:setting:`EMAIL_USE_SSL` settings in your settings file.
+.. versionadded:: 1.7
+This backend contains a configurable `timeout` parameter, which can be overwritten
+with the following sample code::
+
+ class MyEmailBackend(smtp.EmailBackend):
+ def __init__(self,*args, **kwargs):
@timgraham

timgraham Oct 24, 2013

Owner

still missing spaces after "self," and the method contents aren't properly indented

@timgraham timgraham commented on an outdated diff Oct 24, 2013

tests/mail/tests.py
@@ -278,6 +278,20 @@ def test_dummy_backend(self):
email = EmailMessage('Subject', 'Content', 'bounce@example.com', ['to@example.com'], headers={'From': 'from@example.com'})
self.assertEqual(connection.send_messages([email, email, email]), 3)
+ def test_connection_timeout_value(self):
+ """ Test that the connection's timeout value is initially None. Test that timeout can be overwritten."""
+ connection = mail.get_connection('django.core.mail.backends.smtp.EmailBackend')
+ self.assertEqual(connection.timeout, None)
+
+ # Test a sample use case. Timeout value can get overwritten by using the __init__ attribute.
+ class MyEmailBackend(smtp.EmailBackend):
+ def __init__(self, *args, **kwargs):
+ kwargs['timeout'] = 42
@timgraham

timgraham Oct 24, 2013

Owner

might as well use setdefault in the test as well

Owner

timgraham commented Oct 24, 2013

the commit message is missing dashes, i.e. "Fixed #21271 --"

@ArcTanSusan ArcTanSusan Fixed #21271 -- Django's usage of smtplib.SMTP should have a timeout.
Also contains test and doc addition to 1.7 release note and /topics/email/#smtp-backend.
Thanks Andre Cruz and Tim Graham for discussions and code review. Thanks edevil for bug report
and initial PR.
7ad0c5a
Owner

timgraham commented Oct 25, 2013

Thanks for your work on this Susan. I made some docs enhancements as well as other little improvements I noticed when doing a final check of this and merged in 4e0a2fe.

timgraham closed this Oct 25, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment