Issue tracker housecleaning: SVN-1804

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Issue tracker housecleaning: SVN-1804

Nathan Hartman
From the "Closing Old Issues" department:

SVN-1804: "mailer.py doesn't catch SMTPRecipientsRefused in finish()"
* Created and last updated in 2004.
* No comments in more than 15 years.

From my reading of this issue, the impact is that any mail delivery
hiccup may cause this hook script to exit with an unhandled exception,
impeding any further mails that it might have sent.

Looking at mailer.py in trunk@HEAD, SMTPOutput.finish() doesn't handle
any exceptions. In particular:

SMTP.login may raise:
* SMTPHeloError
* SMTPAuthenticationError
* SMTPNotSupportedError
* SMTPException

SMTP.sendmail may raise:
* SMTPRecipientsRefused
* SMTPHeloError
* SMTPSenderRefused
* SMTPDataError
* SMTPNotSupportedError

Any of these exceptions cause the same impact. The same is probably
true of any unhandled exceptions throughout the script, but SMTP errors
are completely outside our control.

So we have a choice to make:

Either:

Option 1: Handle (and ignore) exceptions in SMTPOutput.finish() to let
script execution continue after any SMTP failure. This could be as
simple as wrapping the contents of SMTPOutput.finish() in a "try:" ..
"except:" (catch-all), where the except block does nothing.

Option 2: Just close the issue. No comments on this issue in 15 years.

Thoughts?

Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Yasuhito FUTATSUKI
On 2019/10/20 13:46, Nathan Hartman wrote:

>  From the "Closing Old Issues" department:
>
> SVN-1804: "mailer.py doesn't catch SMTPRecipientsRefused in finish()"
> * Created and last updated in 2004.
> * No comments in more than 15 years.
>
>  From my reading of this issue, the impact is that any mail delivery
> hiccup may cause this hook script to exit with an unhandled exception,
> impeding any further mails that it might have sent.
>
> Looking at mailer.py in trunk@HEAD, SMTPOutput.finish() doesn't handle
> any exceptions. In particular:
>
> SMTP.login may raise:
> * SMTPHeloError
> * SMTPAuthenticationError
> * SMTPNotSupportedError
> * SMTPException
>
> SMTP.sendmail may raise:
> * SMTPRecipientsRefused
> * SMTPHeloError
> * SMTPSenderRefused
> * SMTPDataError
> * SMTPNotSupportedError
>
> Any of these exceptions cause the same impact. The same is probably
> true of any unhandled exceptions throughout the script, but SMTP errors
> are completely outside our control.
>
> So we have a choice to make:
>
> Either:
>
> Option 1: Handle (and ignore) exceptions in SMTPOutput.finish() to let
> script execution continue after any SMTP failure. This could be as
> simple as wrapping the contents of SMTPOutput.finish() in a "try:" ..
> "except:" (catch-all), where the except block does nothing.

With this approach, I think the script should exit with code
other than 0 if an exception raised, to notify error occured to hook
scripts. Python script exits with code 1 if the script is termitated
by unhandled exceptions, so it can notify on current implementation.
Hook scripts can also watch stderr to log what happens on mailer.py,
etc., though our post-*.tmpl don't do it.

Cheers,
--
Yasuhito FUTATSUKI <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Nathan Hartman
On Sun, Oct 20, 2019 at 2:16 AM Yasuhito FUTATSUKI <[hidden email]> wrote:
On 2019/10/20 13:46, Nathan Hartman wrote:
>  From the "Closing Old Issues" department:
>
> SVN-1804: "mailer.py doesn't catch SMTPRecipientsRefused in finish()"
> * Created and last updated in 2004.
> * No comments in more than 15 years.
>
>  From my reading of this issue, the impact is that any mail delivery
> hiccup may cause this hook script to exit with an unhandled exception,
> impeding any further mails that it might have sent.

(snip)
 
> Option 1: Handle (and ignore) exceptions in SMTPOutput.finish() to let
> script execution continue after any SMTP failure. This could be as
> simple as wrapping the contents of SMTPOutput.finish() in a "try:" ..
> "except:" (catch-all), where the except block does nothing.

With this approach, I think the script should exit with code
other than 0 if an exception raised, to notify error occured to hook
scripts. Python script exits with code 1 if the script is termitated
by unhandled exceptions, so it can notify on current implementation.
Hook scripts can also watch stderr to log what happens on mailer.py,
etc., though our post-*.tmpl don't do it.

I think the attached patch accomplishes that. (I'm not putting it inline
because it's 200 lines long! Most of them due to indent changes.)

I'd appreciate a review!

The changes are pretty basic:
* A new exception class is added: MessageSendFailure.
* The contents of SMTPOutput.finish() are wrapped in try..except. We
  catch any smtplib.SMTPException, print an error to stderr, and raise
  MessageSendFailure.
* In all classes' generate(), we wrap the body of the for-loop in a
  try..except. We catch MessageSendFailure and increment a counter;
  generate() returns the value of the counter.
* In main, the return value of messenger.generate() is returned to
  main's caller.
* In the __main__ program, the call to svn.core.run_app() is wrapped
  with sys.exit().


mailer_py_patch.txt (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Daniel Shahaf-2
Nathan Hartman wrote on Wed, Oct 23, 2019 at 11:48:28 -0400:

> On Sun, Oct 20, 2019 at 2:16 AM Yasuhito FUTATSUKI <[hidden email]>
> wrote:
> > With this approach, I think the script should exit with code
> > other than 0 if an exception raised, to notify error occured to hook
> > scripts. Python script exits with code 1 if the script is termitated
> > by unhandled exceptions, so it can notify on current implementation.
> > Hook scripts can also watch stderr to log what happens on mailer.py,
> > etc., though our post-*.tmpl don't do it.
>
>
> I think the attached patch accomplishes that. (I'm not putting it inline
> because it's 200 lines long! Most of them due to indent changes.)

In such cases, it's helpful to attach a secondary diff, generated with «svn
diff -x-wp», for review purposes.

> I'd appreciate a review!
>
> The changes are pretty basic:
> * A new exception class is added: MessageSendFailure.
> * The contents of SMTPOutput.finish() are wrapped in try..except. We
>   catch any smtplib.SMTPException, print an error to stderr, and raise
>   MessageSendFailure.
> * In all classes' generate(), we wrap the body of the for-loop in a
>   try..except. We catch MessageSendFailure and increment a counter;
>   generate() returns the value of the counter.
> * In main, the return value of messenger.generate() is returned to
>   main's caller.
> * In the __main__ program, the call to svn.core.run_app() is wrapped
>   with sys.exit().

What is the functional change?  A log message should describe the functional
change and the the reasons therefor; it shouldn't simply narrate the diff.  Your
last bullet is a good counter-example.

As Yasuhito said, unhandled exceptions already cause a non-zero exit code, so
I'm guessing the point here is that SMTPException during the first delivery no
longer prevent second (and any subsequent) deliveries from being attempted?  In
other words, in what case does the patch have a user-visible effect?

> Index: tools/hook-scripts/mailer/mailer.py
> ===================================================================
> --- tools/hook-scripts/mailer/mailer.py (revision 1868639)

[ I am quoting the -x-wp diff, rather than the one Nathan sent. ]

> @@ -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("SMTP error occurred: %s" % detail);

Error messages should be signed:

   sys.stderr.write("mailer.py: SMTP error occurred: %s" % (detail,))
   #                 ^^^^^^^^^^^

> @@ -1455,8 +1476,8 @@ 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,
> -                   sys.argv[3:3+expected_args])
> +  sys.exit(svn.core.run_app(main, cmd, config_fname, repos_dir,
> +                   sys.argv[3:3+expected_args]))

If run_app returns 256, the exit code will be zero.  I suggest to do

   ret = svn.core.run_app(…)
   sys.exit(1 if ret else 0)

I did not review the core functionality changes due to lack of time, sorry.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Yasuhito FUTATSUKI
In reply to this post by Nathan Hartman
I'm sorry I didn't point out exeptions caused in smtplib, and
too late to reply.

(I already read
<CAJT2EHpZQ7ws6wPFvAUA8rGaExTHGSV=zDzbgt9=AOATES00=[hidden email]>
on Wed, 23 Oct 2019 11:48:28 -0400 from Nathan and
<[hidden email].local2>
on Thu, 24 Oct 2019 02:37:17 +0000 from Daniel,
but my this reply is not related to them immediately, so I reply to
<CAJT2EHrvhVT+QWxCo+X=[hidden email]>
on Sun, 20 Oct 2019 00:46:25 -0400 from Nathan again.)

On 2019/10/20 13:46, Nathan Hartman wrote:

>  From the "Closing Old Issues" department:
>
> SVN-1804: "mailer.py doesn't catch SMTPRecipientsRefused in finish()"
> * Created and last updated in 2004.
> * No comments in more than 15 years.
>
>  From my reading of this issue, the impact is that any mail delivery
> hiccup may cause this hook script to exit with an unhandled exception,
> impeding any further mails that it might have sent.
>
> Looking at mailer.py in trunk@HEAD, SMTPOutput.finish() doesn't handle
> any exceptions. In particular:
>
> SMTP.login may raise:
> * SMTPHeloError
> * SMTPAuthenticationError
> * SMTPNotSupportedError
> * SMTPException
>
> SMTP.sendmail may raise:
> * SMTPRecipientsRefused
> * SMTPHeloError
> * SMTPSenderRefused
> * SMTPDataError
> * SMTPNotSupportedError
>
> Any of these exceptions cause the same impact. The same is probably
> true of any unhandled exceptions throughout the script, but SMTP errors
> are completely outside our control.

Exception on SMTP.login will be repeated unless its response code indicates
temporary error (3XX), because each try uses same parameters. On the other
side, on SMTP.sendmail, with other sender, recipient, or even message data,
try after failure may succeed, of course it depend on kind of exception.
So I think those are not the same impact. Some of them are fatal and
others are not. I guess that is why the reporter mensions only
SMTPRecipientRefused in those exceptions.

As how far we analyze and handle errors honestly is a trade-off
between simplicity and precision, I don't want and don't aim to analyze
too deep.

Cheers,
--
Yasuhito FUTATSUKI <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Yasuhito FUTATSUKI
On 2019/10/25 7:19, Yasuhito FUTATSUKI wrote:
> Exception on SMTP.login will be repeated unless its response code indicates
> temporary error (3XX), because each try uses same parameters. On the other
                    ^^^
Temporary failures are 4XX.

Cheers,
--
Yasuhito FUTATSUKI <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Nathan Hartman
In reply to this post by Yasuhito FUTATSUKI
On Thu, Oct 24, 2019 at 6:20 PM Yasuhito FUTATSUKI <[hidden email]> wrote:

Exception on SMTP.login will be repeated unless its response code indicates
temporary error (3XX), because each try uses same parameters. On the other
side, on SMTP.sendmail, with other sender, recipient, or even message data,
try after failure may succeed, of course it depend on kind of exception.
So I think those are not the same impact. Some of them are fatal and
others are not. I guess that is why the reporter mensions only
SMTPRecipientRefused in those exceptions.

As how far we analyze and handle errors honestly is a trade-off
between simplicity and precision, I don't want and don't aim to analyze
too deep

Thank you to Daniel and Yasuhito for reviewing!

I think we should aim for simplicity, unless you feel strongly that we should handle the login error separately.

Granted, this is not a perfect solution. Also it does not attempt to identify and address any other unhandled exceptions that might occur elsewhere in the program.

But it is an improvement, because right now we have no error handling whatsoever for SMTP failures.

I'll fix the mistakes when I get back to the office and I'll post a corrected secondary diff and proper log message...

Thanks again for reviewing.

Nathan



Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Nathan Hartman
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 <[hidden email]>
           Yasuhito FUTATSUKI <[hidden email]>
]]]

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


mailer-full-patch.txt (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Daniel Shahaf-2
Nathan Hartman wrote on Sun, Oct 27, 2019 at 23:14:55 -0400:
> [[[
> 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.

*lightbulb*

> Found by: rzigweid
> Review by: Daniel Shahaf <[hidden email]>
>            Yasuhito FUTATSUKI <[hidden email]>

Committers may be referred to by their names in COMMITTERS, but non-committers
should be referred to by name, when available.

In this case, having checked <http://subversion.tigris.org/issues/show_bug.cgi?id=1804> to
find rzigweid's name,¹ the conventional form would be:
.
    Found by: Robert M. Zigweid
    Review by: danielsh
               futatuki

Email addresses are optional, in the sense that contribulyzer doesn't require
them.  When specified, we tend to escape @ as {_AT_}.  (This particular
replacement is recognized by tools/dev/contribulyze.py.)

In my review I specifically noted that I hadn't done a complete review.  It would
have been appropriate to add a parenthetical recording that fact.

> +++ tools/hook-scripts/mailer/mailer.py (working copy)
> @@ -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':

Your client inserted a hard line break here.  That's not a deal breaker for a
-x-w patch, of course; just a readability thing.

>        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

Sorry for not catching this in my previous review, but is there any additional
context we provide to the sysadmin who'll read this error message?  For
example, the revision number, repository name, recipients, subject line,
message-id?

The new log message looks good (though the glob pattern did gave me pause), but
I haven't reviewed the logic changes.

Cheers,

Daniel

¹ Aside: When we migrated from tigris to jira, we seem to have lost the "tigris
username to fullname" mapping.  I suspect we don't have any backup of it, save for
what archive.org's spider may have cached.
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Yasuhito FUTATSUKI
In reply to this post by Nathan Hartman
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 <[hidden email]>/<[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Branko Čibej
On 29.10.2019 04:30, Yasuhito FUTATSUKI wrote:

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

+1

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

Yes, but there's nothing we can do about that. We can only, as you
suggested, give the administrator some time to avoid the lockout by
failing early on fatal errors.


-- Brane
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Nathan Hartman
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


mailer-full-patch.txt (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Daniel Shahaf-2
Nathan Hartman wrote on Tue, Oct 29, 2019 at 11:11:58 -0400:
> [[[
>   def finish(self):

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

Would «raise MessageSendFailure from detail» make sense here, and in the next
«except» case as well?

> Daniel, thank you for finding rzigweid's real name. I didn't know how
> to do that. :-)

You're welcome.  For context, Subversion's homepage and issue tracker were both
at https://subversion.tigris.org/ for the first decade of the project's
existence.  When we moved to ASF (between 1.6.0 and 1.7.0), we also migrated to
an ASF-hosted issue tracker to avoid the tigris.org bus factor risk.  The old
issue tracker was then made read-only.

This is also related to the transition from svn.collab.net to svn.apache.org,
documented in ^/subversion/README (sic).

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

Thanks.  HACKING does document this explicitly, too.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Nathan Hartman
On Tue, Oct 29, 2019 at 11:36 AM Daniel Shahaf <[hidden email]> wrote:
Nathan Hartman wrote on Tue, Oct 29, 2019 at 11:11:58 -0400:
> [[[
>   def finish(self):

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

Would «raise MessageSendFailure from detail» make sense here, and in the next
«except» case as well?

Yes. Good catch! I'll fix it.
 
> 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.

Thanks.  HACKING does document this explicitly, too.

It sure does.

I think some of the earlier points you brought up are not yet mentioned in
HACKING. Making a note to look at that more carefully later and fix if
necessary...

Thanks,
Nathan

Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Nathan Hartman
> SVN-1804: "mailer.py doesn't catch SMTPRecipientsRefused in finish()"

The fix for SVN-1804 is committed in r1869194.

Thanks to all for your feedback (and patience!)

Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Daniel Shahaf-2
Nathan Hartman wrote on Thu, 31 Oct 2019 03:24 +00:00:
> > SVN-1804: "mailer.py doesn't catch SMTPRecipientsRefused in finish()"
>
> The fix for SVN-1804 is committed in r1869194.
>
> Thanks to all for your feedback (and patience!)

You're welcome, and here's my +0 for backport to 1.13.x.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Yasuhito FUTATSUKI
In reply to this post by Nathan Hartman
It's too late, but it is a bit pity....

On 2019/10/30 0:11, Nathan Hartman wrote:

<snip>  
> After incorporating all the preceding feedback, the function is
> considerably longer, but we handle and report errors:
> > [[[>    def finish(self):

<snip>

>      finally:
>        server.quit()
> ]]]

As I mentioned before (but I couldn't tell what I want, sorry),
smtplib.SMTP.quit() can raise exception. If server.quit() in finally
block has raised an exception and it isn't caught, it will overwrite
the exception raised in try block or re-raised in exception block.

The exception on server.quit() itself doesn't affect the result that
we could send message or not, and there is nothing we can do more
about this SMTP session. So we can catch it, then safely ignore it or
at most log its cause as warning, and continue the transaction.
Even if the cause of exception is fatal, It doesn't directly affect next
attempt to send message.

Cheers,
--
Yasuhito FUTATSUKI <[hidden email]>/<[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Nathan Hartman
On Sat, Nov 2, 2019 at 4:00 AM Yasuhito FUTATSUKI <[hidden email]> wrote:
It's too late, but it is a bit pity....

On 2019/10/30 0:11, Nathan Hartman wrote:

<snip> 
> After incorporating all the preceding feedback, the function is
> considerably longer, but we handle and report errors:
> > [[[>    def finish(self):

<snip>

>      finally:
>        server.quit()
> ]]]

As I mentioned before (but I couldn't tell what I want, sorry),
smtplib.SMTP.quit() can raise exception. If server.quit() in finally
block has raised an exception and it isn't caught, it will overwrite
the exception raised in try block or re-raised in exception block.

Yes, you did say that before, but I forgot. :-(

I'll fix it soon (when I'll be at my computer).

The exception on server.quit() itself doesn't affect the result that
we could send message or not, and there is nothing we can do more
about this SMTP session. So we can catch it, then safely ignore it or
at most log its cause as warning, and continue the transaction.
Even if the cause of exception is fatal, It doesn't directly affect next
attempt to send message.

Ok. Will do.

Thank you for catching that!!

Nathan 

Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Nathan Hartman
On Sun, Nov 3, 2019 at 12:56 AM Nathan Hartman <[hidden email]> wrote:
On Sat, Nov 2, 2019 at 4:00 AM Yasuhito FUTATSUKI <[hidden email]> wrote:
<snip>

>      finally:
>        server.quit()
> ]]]

As I mentioned before (but I couldn't tell what I want, sorry),
smtplib.SMTP.quit() can raise exception. If server.quit() in finally
block has raised an exception and it isn't caught, it will overwrite
the exception raised in try block or re-raised in exception block.

It seems there's another problem.

Apparently the 'raise .. from' construct is Python 3+. Which means
this breaks the mailer for Python 2.7. I found this when I tested on
Python 2.7 earlier and the script terminated with a syntax error at
'raise MessageSendFailure from detail'.

Which do you think is better:

Remove the 'from detail' and lose the original cause of
MessageSendFailure. MessageSendFailure is meant to be handled and
ignored (possibly only used for outputting additional logging),
allowing the mailer to continue mailing, so this might be acceptable.

Or, alternately:

Check which version of Python is running, use 'from detail' only when
Python 3+.

Thanks,
Nathan

[[[
Index: tools/hook-scripts/mailer/mailer.py
===================================================================
--- tools/hook-scripts/mailer/mailer.py (revision 1869339)
+++ tools/hook-scripts/mailer/mailer.py (working copy)
@@ -325,12 +325,12 @@
     except smtplib.SMTPRecipientsRefused as detail:
       sys.stderr.write("mailer.py: SMTP recipient(s) refused: %s: %s\n"
                            % (self.to_addrs, detail,))
-      raise MessageSendFailure from detail
+      raise MessageSendFailure # from detail <-- breaks on Python 2.7 !!!!
 
     except smtplib.SMTPSenderRefused as detail:
       sys.stderr.write("mailer.py: SMTP sender refused: %s: %s\n"
                            % (self.from_addr, detail,))
-      raise MessageSendFailure from detail
+      raise MessageSendFailure # from detail <-- breaks on Python 2.7 !!!!
 
     except smtplib.SMTPException as detail:
       # All other errors are fatal; this includes:
@@ -339,7 +339,11 @@
       raise
 
     finally:
-      server.quit()
+      try:
+        server.quit()
+      except smtplib.SMTPException as detail:
+        sys.stderr.write("mailer.py: Error occurred during SMTP session cleanup: %s\n"
+                             % (detail,))
 
 
 class StandardOutput(OutputBase):
]]]


Reply | Threaded
Open this post in threaded view
|

Re: Issue tracker housecleaning: SVN-1804

Yasuhito FUTATSUKI
On 2019/11/04 8:16, Nathan Hartman wrote:
> On Sun, Nov 3, 2019 at 12:56 AM Nathan Hartman <[hidden email]>
> wrote:

> It seems there's another problem.
>
> Apparently the 'raise .. from' construct is Python 3+. Which means
> this breaks the mailer for Python 2.7. I found this when I tested on
> Python 2.7 earlier and the script terminated with a syntax error at
> 'raise MessageSendFailure from detail'.
>
> Which do you think is better:
>
> Remove the 'from detail' and lose the original cause of
> MessageSendFailure. MessageSendFailure is meant to be handled and
> ignored (possibly only used for outputting additional logging),
> allowing the mailer to continue mailing, so this might be acceptable.
>
> Or, alternately:
>
> Check which version of Python is running, use 'from detail' only when
> Python 3+.

The latter can't accomplish with only single script, because syntax
error occurs on parse on loading, not on execution. To hide
'from detail' from Python 2 interpreter, we should get it out of
the script.

(The hunk arround server.quit() looks good to me.)

Cheers,
--
Yasuhito FUTATSUKI <[hidden email]>/<[hidden email]>
12