Opened 19 months ago

Closed 18 months ago

Last modified 18 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.

Change History (11)

comment:1 Changed 19 months ago by edevil

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

comment:2 Changed 19 months ago by claudep

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

comment:3 Changed 19 months ago by susan

This looks like an interesting ticket! The PR is currently missing tests. So, I'll work on writing up tests in a new PR.

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?

Version 1, edited 19 months ago by susan (previous) (next) (diff)

comment:4 Changed 19 months ago by susan

  • Cc susan.tan.fleckerl@… added

comment:5 Changed 19 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 18 months ago by edevil

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

comment:7 Changed 18 months ago by timo

  • Patch needs improvement set

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

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

In 3afde36d03e2b3b5ff5a265af39d8fb27afa8959:

Undelete the login() call inadvertantly removed in 4e0a2fe59c

Refs #21271.

comment:9 Changed 18 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 18 months ago by anonymous

Any chance of this fix landing in 1.6?

comment:11 Changed 18 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.

Note: See TracTickets for help on using tickets.
Back to Top