Re: Issue tracker housecleaning: SVN-1804

On 2019-10-28 12:14, Nathan Hartman wrote:
> 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.

I worry about lock out caused by repeated helo/ehlo error or authentication
failure when the user uses external SMTP service, watched by inspection
tools (e.g. fail2ban, etc). So I think it is better that
SMTPAuthenticationError and SMTPHeloError are treated as fatal.

Even if only one error caused per run, successive trigger to run the script
can alse cause it, though.

> 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

I'm sorry I also couldn't catch all to point out.

If server.login() or server.sendmail() raise an exception, server.quit()
will be skipped, so the SMTP connections may be left until Python's GC can
find them. To avoid this, it is better to make finally clause and
if server is valid SMTP object, then do explicitly sever.quit() in it.

Note that smtplib.SMTP_SSL() or smtplib.SMTP() also may raise exception,
`server' is not set and thus server.quit() is not need in such case.

server.quit() itself may raise exception even if server is valid
smtplib.SMTP object, so it may also need another try: ... exception: block.

(To understand what happens when errors occur, SMTP server side log may
also be helpful).

> 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
>
> ]]]

I prefer to propagate error by rasing exception rather than return value,
however I could't find this sort of coding convensions here, so it is
just my impression.

Cheers,

-- 
Yasuhito FUTATSUKI <futatuki_at_yf.bsdclub.org>/<futatuki_at_poem.co.jp>