Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

#12419 closed enhancement (fixed)

speed-up notification tests on Windows

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.

Attachments (0)

Change History (6)

comment:2 by Christian Boos, 5 years ago

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

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

comment:3 by Jun Omae, 5 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, 5 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 5 years ago by Christian Boos (previous) (diff)

comment:5 by Jun Omae, 5 years ago

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

comment:6 by anonymous, 5 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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.