Re: Issue tracker housecleaning: SVN-1804

Finally got back around to this...

With these changes, I verified that:
* sending with SMTP works and mailer.py exits with 0 on success
* using a deliberately wrong configuration to trigger a failure, the SMTP
  exception is caught, the message prints to stderr, and mailer.py exits
  with 1 to indicate failure(s) occurred.

Since I didn't send a secondary diff or proper log message before, here
it is, with the corrections suggested by Daniel.

Currently we don't give different treatment to errors in SMTP.login and
SMTP.sendmail, although as Yasuhito points out, this may cause repeated
errors. Let me know if you feel this should be addressed.

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 and attempt to continue
execution to avoid losing any later emails. If SMTP error occurs, we
report it to stderr and the script's exit code is nonzero.

* 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): Wrap logic in try .. except block; report
    SMTP exception to stderr.
  (*.generate): Wrap for loop contents in try .. except block; return
    nonzero if SMTP error(s) occurred.
  (__main__): Exit nonzero if SMTP error(s) occurred.

Found by: rzigweid
Review by: Daniel Shahaf <d.s_at_daniel.shahaf.name>
           Yasuhito FUTATSUKI <futatuki_at_poem.co.jp>
]]]

Secondary diff (svn diff -x-wp):

[[[

Index: tools/hook-scripts/mailer/mailer.py
===================================================================
--- tools/hook-scripts/mailer/mailer.py (revision 1869058)
+++ 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,6 +285,7 @@ class SMTPOutput(MailedOutput):
     self.write(self.mail_headers(group, params))

   def finish(self):
+ 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:
@@ -294,6 +295,9 @@ class SMTPOutput(MailedOutput):
                    self.cfg.general.smtp_password)
     server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())
     server.quit()
+ except smtplib.SMTPException as detail:
+ sys.stderr.write("mailer.py: SMTP error occurred: %s\n" % (detail,))
+ raise MessageSendFailure

 class StandardOutput(OutputBase):
@@ -421,11 +425,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 +439,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 +465,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 +496,9 @@ class PropChange(Messenger):
           'to' : tempfile2.name,
           }))
       self.output.finish()
+ except MessageSendFailure:
+ ret = 1
+ return ret

 def get_commondir(dirlist):
@@ -564,7 +578,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 +595,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 +1413,8 @@ class UnknownMappingSpec(Exception):
   pass
 class UnknownSubcommand(Exception):
   pass
+class MessageSendFailure(Exception):
+ pass

 if __name__ == '__main__':
@@ -1455,8 +1476,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

]]]

Thanks for the help!

Nathan