#1023 closed defect (fixed)
mail_admins called even on SystemExit
Reported by: | hugo | Owned by: | Adrian Holovaty |
---|---|---|---|
Component: | Core (Other) | Version: | |
Severity: | normal | Keywords: | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
If DEBUG=False, Django mails about exceptions to the admins. That's generally great. But it even mails them on SystemExit exceptions - which are only triggered if the programmer deliberately called sys.exit with some exit code. I think SystemExit should be ignored for mailing, as the coder would surely have plugged his own mailing if that would have been needed. Especially if the exit code is 0 ...
Something along the lines:
Index: core/handlers/base.py =================================================================== --- core/handlers/base.py (revision 1565) +++ core/handlers/base.py (working copy) @@ -104,6 +104,8 @@ return self.get_friendly_error_response(request, resolver) except exceptions.PermissionDenied: return httpwrappers.HttpResponseForbidden('<h1>Permission denied</h1>') + except SystemExit: + pass except: # Handle everything else, including SuspiciousOperation, etc. if DEBUG: return self.get_technical_error_response(request)
Why this arises in my system: I have a job control app running that triggers background processes - and since they are triggered in the view functions, they have the full stack of the base handler active. The background process just ends with sys.exit(0) if everything is ok, but due to the SystemExit messaging I get error messages in my mailbox.
Change History (9)
comment:1 by , 19 years ago
Status: | new → assigned |
---|
comment:2 by , 19 years ago
This is a SytemExit - there isn't anything to return, it's the end of the process. Where would you return to? ;-)
Oh, and it's DEBUG=False, so get_friendly_error_response is out, anyway, isn't it? I thought that one is for DEBUG=True? But anyway, as it is SystemExit, which is _only_ triggered by sys.exit(), it shouldn't try to do anything else but just, well, exit.
comment:3 by , 19 years ago
Oh, maybe you are right on that "pass" isn't the right thing - it might be that "raise" would be better, to reraise the SystemExit. That's why I said "along the lines" - I just catched a cold and am dumbed down to the level of PHP programmer, so take patches from me carefully ;-)
comment:4 by , 19 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
I guess I don't understand why a view would want to call SystemExit
. But even if you want to do that (which is fine -- it's your code), I don't think the base handler should have to know about it. It seems like too much of an edge case. Maybe you can change your app to raise another error, or something?
comment:5 by , 19 years ago
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
Uhm. It is _not_ a view. The problem is, it is a forked process from a view. The code itself has _nothing_ to do with view functionality. And no, I can't change my code, because the only way to end my forked process is to call sys.exit(0) - and regardless of what Django does, I am absolutely positively sure it should never send a "coding error" mail to the admins because a forked process ended successfully. :-)
My problem will happen with all code that uses fork to start off something in the background, because when you run your code, you are in a view function and so the forked process will get the current environment and so the base handler is active. It's a rather common problem of multiprocess programming - if you do it, and you have global exception handlers, you will have to handle SystemExit specially. The base handler _must_ know about it, because that's where the global exception handler sits - and catches SystemExit, and does create a mail on it ...
I can't raise any other error, as all of them will produce some mailing when Django runs with DEBUG=False. And I don't raise SystemExit myself - I just use the standard official way to terminate a process successfully by calling sys.exit(0).
comment:6 by , 19 years ago
To make it a bit more clear what is happening:
import os import sys try: pid = os.fork() if pid: print os.getpid(), "parent" else: print os.getpid(), "child" sys.exit(0) except Exception, e: print os.getpid(), "error", e print os.getpid(), "only parent should go here"
That's the outline of what is happening with Django. The parent is the view funciton. The exception handling takes place in the base handler. The forked process needs to sys.exit(), because otherwise it would execute code that only the parent should execute. But due to the exception-handling in the base handler, the sys.exit(0) is catched, mailed, and ignored. So two things happen: the child process produces a "coding error" mail - which is wrong, as it just should die silently - and the parent code is still executed. This effectively breaks multiprocessing from view functions.
Correct it would be this way:
import os import sys try: pid = os.fork() if pid: print os.getpid(), "parent" else: print os.getpid(), "child" sys.exit(0) except SystemExit: raise except Exception, e: print os.getpid(), "error", e print os.getpid(), "only parent should go here"
Here the base handler catches SystemExit and just reraises it. It is just needed because SystemExit is an Exception that explicitely denotes _no_ error - it just denotes a system exit with given return code. You can't avoid raising it if you use sys.exit, it's the runtime that does that.
So, yes, actually the "pass" was rather stupid to do - it only catched the problem of the mailing, not the problem of the parent code.
comment:7 by , 19 years ago
Just for the records: there is a - rather ugly - temporary fix for this problem:
import signal import os try: pid = os.fork() if pid: print "parent", os.getpid() else: print "child", os.getpid() os.kill(os.getpid(), signal.SIGTERM) except: print 'error', os.getpid() print "only parent should reach this", os.getpid()
So you actually have to kill yourself to stop the running process - sending SIGTERM won't produce an exception, but won't run any standard exit handlers - only those that are hooked up with the sigterm signal are executed. But at least it allows to solve this for now.
This does have problems:
- won't be fully portable
- might not work on systems with enabled capabilities (as user processes not necessarily have the capability to send signals to themselves)
- it really is butt-ugly
comment:8 by , 19 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
comment:9 by , 18 years ago
You may also be interested in #4701 if you're reading over this old bug report.
Instead of
pass
ing, shouldn't itreturn self.get_friendly_error_response(request, resolver)
?