Code

Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#21271 closed Cleanup/optimization (fixed)

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

Reported by: edevil Owned by: nobody
Component: Core (Mail) Version: 1.5
Severity: Normal Keywords: smtp timeout
Cc: susan.tan.fleckerl@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

When EmailBackend initializes SMTP objects it does not provide a timeout, and the default timeout is object() (no timeout). It would be sensible to provide some configurable timeout.

I've got bitten by this when using the AdminEmailHandler. My database went down, a lot of exceptions were generated and Django was trying to send emails. The SMTP server started not responding and the requests started blocking, until all my workers were used up. By then Django stopped serving requests. Basically, I was DoSed by my own SMTP server.

Attachments (0)

Change History (11)

comment:1 Changed 6 months ago by edevil

  • Easy pickings set
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 months ago by claudep

  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/optimization

comment:3 Changed 6 months ago by susan

This looks like an interesting ticket! The PR is currently missing a test. I've merged @edevil's PR into a new one here + a test: https://github.com/django/django/pull/1758

In the current PR, the timeout is set to 60 seconds; it's hard-coded in. On the email backend side, should any action happen (such as closing the smtp email-backend connection) when that time limit of 60 seconds is reached?

Last edited 6 months ago by susan (previous) (diff)

comment:4 Changed 6 months ago by susan

  • Cc susan.tan.fleckerl@… added

comment:5 Changed 6 months ago by edevil

Can't you use the global_setting in assertEqual(), so that the value isn't hardcoded?

As for the backend, currently send_messages() already skips close() when _send() throws an exception. But in the case of timeouts, smtplib automatically closes the underlying connection.

comment:6 Changed 6 months ago by edevil

Any news on this? There is a patch available with tests.

comment:7 Changed 6 months ago by timo

  • Patch needs improvement set

The patch needs improvement per the comments on the pull request.

comment:8 Changed 6 months ago by Claude Paroz <claude@…>

In 3afde36d03e2b3b5ff5a265af39d8fb27afa8959:

Undelete the login() call inadvertantly removed in 4e0a2fe59c

Refs #21271.

comment:9 Changed 6 months ago by timo

  • Resolution set to fixed
  • Status changed from new to closed

In 4e0a2fe59c8b9c32c2f3111474354356474128a8:

Fixed #21271 -- Added timeout parameter to SMTP EmailBackend.

Thanks Tobias McNulty and Tim Graham for discussions and code review.
Thanks Andre Cruz the suggestion and initial patch.

comment:10 Changed 6 months ago by anonymous

Any chance of this fix landing in 1.6?

comment:11 Changed 6 months ago by timo

No, now that 1.6 has been released, it'll only receive critical bug fixes. See https://docs.djangoproject.com/en/dev/internals/release-process/#supported-versions for details.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.