Re: Issue tracker housecleaning: SVN-1804

Thanks to everyone for reviewing and providing feedback!

I've incorporated it and I hope that the following is good enough to
be committed as a fix for SVN-1804...

Since last time, only SMTPOutput.finish changes. I think it will be
easier to just see the function in full rather than a diff...

This is the original function before I began:

[[[
  def finish(self):
    if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl ==
'yes':
      server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
    else:
      server = smtplib.SMTP(self.cfg.general.smtp_hostname)
    if self.cfg.is_set('general.smtp_username'):
      server.login(self.cfg.general.smtp_username,
                   self.cfg.general.smtp_password)
    server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())
    server.quit()
]]]

After incorporating all the preceding feedback, the function is
considerably longer, but we handle and report errors:

[[[
  def finish(self):
    """
    Send email via SMTP or SMTP_SSL, logging in if username is
    specified.

    Errors such as invalid recipient, which affect a particular email,
    are reported to stderr and raise MessageSendFailure. If the caller
    has other emails to send, it can continue doing so.

    Errors caused by bad configuration, such as login failures, for
    which multiple retries could lead to SMTP server lockout, are
    reported to stderr and re-raised. These should be considered fatal.
    """

    try:
      if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl
== 'yes':
        server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
      else:
        server = smtplib.SMTP(self.cfg.general.smtp_hostname)
    except Exception as detail:
      sys.stderr.write("mailer.py: Failed to instantiate SMTP object: %s\n"
% (detail,))
      # Any error to instantiate is fatal
      raise

    try:
      if self.cfg.is_set('general.smtp_username'):
        try:
          server.login(self.cfg.general.smtp_username,
                       self.cfg.general.smtp_password)
        except smtplib.SMTPException as detail:
          sys.stderr.write("mailer.py: SMTP login failed with username %s
and/or password: %s\n"
                           % (self.cfg.general.smtp_username, detail,))
          # Any error at login is fatal
          raise

      server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())

    except smtplib.SMTPRecipientsRefused as detail:
      sys.stderr.write("mailer.py: SMTP recipient(s) refused: %s: %s\n"
                           % (self.to_addrs, detail,))
      raise MessageSendFailure

    except smtplib.SMTPSenderRefused as detail:
      sys.stderr.write("mailer.py: SMTP sender refused: %s: %s\n"
                           % (self.from_addr, detail,))
      raise MessageSendFailure

    except smtplib.SMTPException as detail:
      # All other errors are fatal; this includes:
      # SMTPHeloError, SMTPDataError, SMTPNotSupportedError
      sys.stderr.write("mailer.py: SMTP error occurred: %s\n" % (detail,))
      raise

    finally:
      server.quit()
]]]

Proposed log message:

[[[
Fix issue #1804: mailer.py terminates abnormally on any SMTP error

Per SVN-1804, any SMTP error terminates mailer.py with an unhandled
exception. The impact is that emails which would have been sent after
the exception are silently lost.

To fix this issue, we handle SMTP exceptions. When an exception only
affects a particular email, such as invalid recipient, execution
continues to avoid losing any later emails. Fatal SMTP errors, such
as login with bad credentials, terminate execution as before but with
some additional reporting to stderr.

The script's exit code is zero if all messages sent successfully,
nonzero if any SMTP error occurs.

* tools/hook-scripts/mailer/mailer.py
  (main): Propagate return value of messenger.generate() to caller.
  (MessageSendFailure): New exception class, to allow for future
    handling of such failures with other (non-SMTP) delivery methods.
  (SMTPOutput.finish): Reimplement with exception handling and
    reporting to stderr.
  (Commit.generate, PropChange.generate, Lock.generate): Wrap contents
    of for-loop in try .. except block; return nonzero if SMTP
    error(s) occurred.
  (__main__): Exit nonzero if SMTP error(s) occurred.

Found by: Robert M. Zigweid
Review by: danielsh (partial review)
           futatuki
]]]

Secondary diff, just for fun. More below...

[[[

Index: tools/hook-scripts/mailer/mailer.py
===================================================================
--- tools/hook-scripts/mailer/mailer.py (revision 1869113)
+++ tools/hook-scripts/mailer/mailer.py (working copy)
@@ -123,7 +123,7 @@ def main(pool, cmd, config_fname, repos_dir, cmd_a
   else:
     raise UnknownSubcommand(cmd)

- messenger.generate()
+ return messenger.generate()

 def remove_leading_slashes(path):
@@ -285,14 +285,59 @@ class SMTPOutput(MailedOutput):
     self.write(self.mail_headers(group, params))

   def finish(self):
+ """
+ Send email via SMTP or SMTP_SSL, logging in if username is
+ specified.
+
+ Errors such as invalid recipient, which affect a particular email,
+ are reported to stderr and raise MessageSendFailure. If the caller
+ has other emails to send, it can continue doing so.
+
+ Errors caused by bad configuration, such as login failures, for
+ which multiple retries could lead to SMTP server lockout, are
+ reported to stderr and re-raised. These should be considered fatal.
+ """
+
+ try:
     if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl
== 'yes':
       server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
     else:
       server = smtplib.SMTP(self.cfg.general.smtp_hostname)
+ except Exception as detail:
+ sys.stderr.write("mailer.py: Failed to instantiate SMTP object:
%s\n" % (detail,))
+ # Any error to instantiate is fatal
+ raise
+
+ try:
     if self.cfg.is_set('general.smtp_username'):
+ try:
       server.login(self.cfg.general.smtp_username,
                    self.cfg.general.smtp_password)
+ except smtplib.SMTPException as detail:
+ sys.stderr.write("mailer.py: SMTP login failed with username %s
and/or password: %s\n"
+ % (self.cfg.general.smtp_username, detail,))
+ # Any error at login is fatal
+ raise
+
     server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())
+
+ except smtplib.SMTPRecipientsRefused as detail:
+ sys.stderr.write("mailer.py: SMTP recipient(s) refused: %s: %s\n"
+ % (self.to_addrs, detail,))
+ raise MessageSendFailure
+
+ except smtplib.SMTPSenderRefused as detail:
+ sys.stderr.write("mailer.py: SMTP sender refused: %s: %s\n"
+ % (self.from_addr, detail,))
+ raise MessageSendFailure
+
+ except smtplib.SMTPException as detail:
+ # All other errors are fatal; this includes:
+ # SMTPHeloError, SMTPDataError, SMTPNotSupportedError
+ sys.stderr.write("mailer.py: SMTP error occurred: %s\n" % (detail,))
+ raise
+
+ finally:
     server.quit()

@@ -421,11 +466,13 @@ class Commit(Messenger):
     ### rather than rebuilding it each time.

     subpool = svn.core.svn_pool_create(self.pool)
+ ret = 0

     # build a renderer, tied to our output stream
     renderer = TextCommitRenderer(self.output)

     for (group, param_tuple), (params, paths) in self.groups.items():
+ try:
       self.output.start(group, params)

       # generate the content for this group and set of params
@@ -433,9 +480,12 @@ class Commit(Messenger):
                        group, params, paths, subpool)

       self.output.finish()
+ except MessageSendFailure:
+ ret = 1
       svn.core.svn_pool_clear(subpool)

     svn.core.svn_pool_destroy(subpool)
+ return ret

 class PropChange(Messenger):
@@ -456,7 +506,9 @@ class PropChange(Messenger):

   def generate(self):
     actions = { 'A': 'added', 'M': 'modified', 'D': 'deleted' }
+ ret = 0
     for (group, param_tuple), params in self.groups.items():
+ try:
       self.output.start(group, params)
       self.output.write('Author: %s\n'
                         'Revision: %s\n'
@@ -485,6 +537,9 @@ class PropChange(Messenger):
           'to' : tempfile2.name,
           }))
       self.output.finish()
+ except MessageSendFailure:
+ ret = 1
+ return ret

 def get_commondir(dirlist):
@@ -564,7 +619,9 @@ class Lock(Messenger):
                                        self.dirlist[0], self.pool)

   def generate(self):
+ ret = 0
     for (group, param_tuple), (params, paths) in self.groups.items():
+ try:
       self.output.start(group, params)

       self.output.write('Author: %s\n'
@@ -579,6 +636,9 @@ class Lock(Messenger):
         self.output.write('Comment:\n%s\n' % (self.lock.comment or ''))

       self.output.finish()
+ except MessageSendFailure:
+ ret = 1
+ return ret

 class DiffSelections:
@@ -1394,6 +1454,8 @@ class UnknownMappingSpec(Exception):
   pass
 class UnknownSubcommand(Exception):
   pass
+class MessageSendFailure(Exception):
+ pass

 if __name__ == '__main__':
@@ -1455,8 +1517,9 @@ if the property was added, modified or deleted, re
   if not os.path.exists(config_fname):
     raise MissingConfig(config_fname)

- svn.core.run_app(main, cmd, config_fname, repos_dir,
+ ret = svn.core.run_app(main, cmd, config_fname, repos_dir,
                    sys.argv[3:3+expected_args])
+ sys.exit(1 if ret else 0)

 # ------------------------------------------------------------------------
 # TODO

]]]

Daniel, thank you for finding rzigweid's real name. I didn't know how
to do that. :-) I agree that the glob pattern in the log message is a
bad idea as it reduces searchability. I've changed this to list each
affected function explicitly.

Yasuhito, thank you for reviewing. I hope the code above addresses
all the very good points you made.

Please let me know if this looks acceptable to be committed...

Thanks again to everyone!
Nathan