Edgewall Software

Opened 8 years ago

Closed 8 years ago

#12419 closed enhancement (fixed)

speed-up notification tests on Windows — at Version 6

Reported by: Christian Boos Owned by: Christian Boos
Priority: normal Milestone: 1.0.11
Component: notification Version: 1.0dev
Severity: normal Keywords: tests smtplib
Cc: Branch:
Release Notes:

The unit-tests for e-mail notification on Windows could take more than a minute. This is now reduced to a few seconds.

API Changes:
Internal Changes:

Description

I was always annoyed by the slowness of the notification tests, but never got around to address that.

While working on migrating the ticket notification templates, I finally found the issue. Rather, there were 2 issues, both related to the way we invoke smtplib.SMTP in trac.notification.mail.SmtpEmailSender.send:

  • when setting host to 'localhost', the connect will go through the results of getaddrinfo, first attempting '::1', which will timeout after one second, and only then trying '127.0.0.1'; so we pass the appropriate host value directly to avoid this timeout
  • when not setting local_hostname, a name resolution would take place and that can also take time on Windows (gethostbyaddr, see #3481)

The speed-up is quite nice, as this is what I have on trunk:

$ time make test=trac/ticket/tests/notification.py

Python: /c/Dev/Python2710x64/python

  Package        Version
  ----------------------------------------------------------------------------------
  Python       : 2.7.10 (default, May 23 2015, 09:44:00) [MSC v.1500 64 bit (AMD64)]
  Setuptools   : 19.2
  Genshi       : 0.7
  Babel        : 2.1.1
  sqlite3      : 2.6.0 (3.6.21)
  PySqlite     : 2.8.1 (3.7.5)
  MySQLdb      : 1.2.5
  Psycopg2     : 2.6.1 (dt dec pq3 ext lo64)
  SVN bindings : 1.9.3 (r1718519)
  Mercurial    : 3.2-rc+51-af1e3ef18120
  Pygments     : 2.1
  Textile      : not installed
  Pytz         : 2015.7
  Docutils     : 0.12
  Twill        : 0.9
  LXML         : not installed
  coverage     : 4.0.3
  figleaf      : not installed

Variables:
  PATH=C:/Dev/Python2710x64;C:/Dev/Python2710x64/Scripts;C:/Dev/VisualSVNServer35/bin;$PATH
  PYTHONPATH=.;C:/Dev/VisualSVNServer35/PythonPackages;$PYTHONPATH
  TRAC_TEST_DB_URI=
  server-options= -p 9000 -a '*,D:/Trac/envs/htdigest.BCT,BCT' -r -e D:/Trac/envs

External dependencies:
  Git version: git version 1.9.4.msysgit.1

python  setup.py -q test -s trac.ticket.tests.notification.suite
SKIP: validation of XHTML output in functional tests (no lxml installed)
..............................................................
----------------------------------------------------------------------
Ran 62 tests in 143.636s

OK

real    2m26.226s
user    0m0.015s
sys     0m0.000s

And this is with the fix:

$ time make test=trac/ticket/tests/notification.py

[...]

python  setup.py -q test -s trac.ticket.tests.notification.suite
SKIP: validation of XHTML output in functional tests (no lxml installed)
..............................................................
----------------------------------------------------------------------
Ran 62 tests in 1.724s

OK

real    0m4.321s
user    0m0.000s
sys     0m0.015s

Maybe I should backport it to 1.0-stable as well.

Change History (6)

comment:2 by Christian Boos, 8 years ago

The patch makes no noticeable difference on Linux (i.e. it was already fast there).

Last edited 8 years ago by Christian Boos (previous) (diff)

comment:3 by Jun Omae, 8 years ago

After those changes, ticket notification would say HELO localhost. I think the behavior could lead blocking by anti-spam helo/ehlo checking. e.g. HELO_LOCALHOST filter in spamassassin.

I think the local_hostname should be cached or configurable.

in reply to:  3 comment:4 by Christian Boos, 8 years ago

Owner: set to Christian Boos
Status: newassigned

Replying to Jun Omae:

After those changes, ticket notification would say HELO localhost. I think the behavior could lead blocking by anti-spam helo/ehlo checking. e.g. HELO_LOCALHOST filter in spamassassin.

Oh right, didn't think about that.

I think the local_hostname should be cached or configurable.

Ok, what about:

  • trac/notification/mail.py

    diff --git a/trac/notification/mail.py b/trac/notification/mail.py
    index 2bccf87..9606052 100644
    a b EMAIL_LOOKALIKE_PATTERN = (  
    6161
    6262_mime_encoding_re = re.compile(r'=\?[^?]+\?[bq]\?[^?]+\?=', re.IGNORECASE)
    6363
     64local_hostname = None
    6465
    6566
    6667def create_charset(mime_encoding):
    class SmtpEmailSender(Component):  
    433434        """Use SSL/TLS to send notifications over SMTP.""")
    434435
    435436    def send(self, from_addr, recipients, message):
     437        global local_hostname
    436438        # Ensure the message complies with RFC2822: use CRLF line endings
    437439        message = fix_eol(message, CRLF)
    438440
    439441        self.log.info("Sending notification through SMTP at %s:%d to %s",
    440442                      self.smtp_server, self.smtp_port, recipients)
    441443        try:
    442             server = smtplib.SMTP(self.smtp_server, self.smtp_port, 'localhost')
     444            server = smtplib.SMTP(self.smtp_server, self.smtp_port,
     445                                  local_hostname)
     446            local_hostname = server.local_hostname
    443447        except smtplib.socket.error as e:
    444448            raise ConfigurationError(
    445449                tag_("SMTP server connection error (%(error)s). Please "

(caching it on the component doesn't help much for the tests, since the reuse rate is very low)

smtplib.SMTP.local_hostname isn't part of the "public" API, however it looks stable enough to me (present from at least from Python 2.4.4 to current head).

Last edited 8 years ago by Christian Boos (previous) (diff)

comment:5 by Jun Omae, 8 years ago

The patch looks good to me. Also, backport to 1.0-stable is good to me.

comment:6 by anonymous, 8 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Thanks for the review!

Applied on trunk in r14651. Before that, was backported to 1.0-stable in r14650.

Note: See TracTickets for help on using tickets.