Fix EmailHandler to not fall into infinite recursion

Detect indirect recursion caused by sendmail, which raises exceptions.

Lines 185, 189, 213 refer to Unicode literals for email subject cr - https://codereview.appspot.com/13629046/

[Thomas.J.Waldmann, 2013-09-21 00:52:40]

https://codereview.appspot.com/13274046/diff/1/MoinMoin/log.py
File MoinMoin/log.py (right):

https://codereview.appspot.com/13274046/diff/1/MoinMoin/log.py#newcode202
MoinMoin/log.py:202: global in_email_handler
globals are usually a recipe for problems.
consider multithreaded execution...

[ana.balica, 2013-09-22 21:17:18]

https://codereview.appspot.com/13274046/diff/1/MoinMoin/log.py
File MoinMoin/log.py (right):

https://codereview.appspot.com/13274046/diff/1/MoinMoin/log.py#newcode202
MoinMoin/log.py:202: global in_email_handler
On 2013/09/21 00:52:40, Thomas.J.Waldmann wrote:
> globals are usually a recipe for problems.
> consider multithreaded execution...

Created 2 loggers: one for stderr and one for email handling, and set
propagation to False. The recursion problem still persists. Even if email logger
tries to send mail and sendmail raises an exception, it is caught by the signal
and is sent to logging again, and again the email logger tries sending mail...

[Thomas.J.Waldmann, 2013-09-23 10:52:20]

https://codereview.appspot.com/13274046/diff/1/MoinMoin/log.py
File MoinMoin/log.py (right):

https://codereview.appspot.com/13274046/diff/1/MoinMoin/log.py#newcode216
MoinMoin/log.py:216: if app.cfg.email_tracebacks:
so, if that is False, you are detecting it here and then nothing is done?

maybe the check should be much earlier in this method.

also, considering this is just a normal logging handler, the configuration
value's name looks like a misnomer, as not only tracebacks could be emailed,
but, depending on the logging configuration, also other stuff.

also, do we need this setting at all? if logging to email can be configured in
the logging configuration (and thus, get used OR NOT), do we need the
cfg.email_tracebacks setting additionally?

[Thomas.J.Waldmann, 2013-09-23 11:00:03]

https://codereview.appspot.com/13274046/diff/1/MoinMoin/log.py
File MoinMoin/log.py (right):

https://codereview.appspot.com/13274046/diff/1/MoinMoin/log.py#newcode208
MoinMoin/log.py:208: cfg = app.cfg
this line is a bit superfluous btw if you use cfg only once.

even for only 2 usages, it might be better to just use app.cfg.

[ana.balica, 2013-09-23 14:16:16]

https://codereview.appspot.com/13274046/diff/1/MoinMoin/log.py
File MoinMoin/log.py (right):

https://codereview.appspot.com/13274046/diff/1/MoinMoin/log.py#newcode216
MoinMoin/log.py:216: if app.cfg.email_tracebacks:
On 2013/09/23 10:52:20, Thomas.J.Waldmann wrote:
> also, do we need this setting at all? if logging to email can be configured in
> the logging configuration (and thus, get used OR NOT), do we need the
> cfg.email_tracebacks setting additionally?

The cfg.email_tracebacks is superfluous, same can be said of cfg.admin_emails,
if one really wants, he can list them in the logging config. But how do we set
up the emailing for the default config file from MoinMoin/log.py? Or should we
not care about that?

[Thomas.J.Waldmann, 2013-10-25 13:20:21]

https://codereview.appspot.com/13274046/diff/10001/MoinMoin/log.py
File MoinMoin/log.py (right):

https://codereview.appspot.com/13274046/diff/10001/MoinMoin/log.py#newcode195
MoinMoin/log.py:195: self.in_email_handler = True
why did you change the initial value from False to True?

this somehow looks like an issue and that it can't ever work like that.

https://codereview.appspot.com/13274046/diff/10001/MoinMoin/log.py#newcode209
MoinMoin/log.py:209: return
as if it is True initially, it can't never execute the code below this line.

https://codereview.appspot.com/13274046/diff/10001/MoinMoin/log.py#newcode217
MoinMoin/log.py:217: self.in_email_handler = False
also, I am thinking about whether a try/finally is needed here because if it is
not there, it'll never reach the "= False" line, which does not release this
lock.

if not releasing it ever again is intended, there should be a comment about it.
If it is intended, there should be try/finally.

[Thomas.J.Waldmann, 2013-11-18 14:55:41]

https://codereview.appspot.com/13274046/diff/50001/MoinMoin/log.py
File MoinMoin/log.py (right):

https://codereview.appspot.com/13274046/diff/50001/MoinMoin/log.py#newcode210
MoinMoin/log.py:210: self.in_email_handler = True
with try/finally, you are making sure that the "lock" that does get set here, is
unlocked again in case something blows up after setting the lock.

so put the "try" into a line just below here?

[Thomas.J.Waldmann, 2013-11-18 15:06:47]

https://codereview.appspot.com/13274046/diff/70001/MoinMoin/log.py
File MoinMoin/log.py (right):

https://codereview.appspot.com/13274046/diff/70001/MoinMoin/log.py#newcode211
MoinMoin/log.py:211: self.in_email_handler = True
well, almost :)

it doesn't make a real difference in this case, but just so you know the general
pattern:

DO_X
try:
    ...
finally:
    UNDO_X

(there is no point in undoing an it some cases it might even fail trying to do
so in case it blows up in the "do" operation itself)

to support such stuff even better, python has so-called context managers, see
the "with" statement, that make such stuff easier. not needed here, though.