Browse Source

[core] Fix 500 handler

Make 500 handler more resilient by checking for exception type and value
before showing debugger. Also, if debugger cannot be shown, show real
500 error instead.
Abraham Elmahrek 12 years ago
parent
commit
3f366a11f2
2 changed files with 19 additions and 14 deletions
  1. 5 7
      desktop/core/src/desktop/tests.py
  2. 14 7
      desktop/core/src/desktop/views.py

+ 5 - 7
desktop/core/src/desktop/tests.py

@@ -27,7 +27,7 @@ import desktop.views as views
 import proxy.conf
 import proxy.conf
 
 
 from nose.plugins.attrib import attr
 from nose.plugins.attrib import attr
-from nose.tools import assert_true, assert_equal, assert_not_equal
+from nose.tools import assert_true, assert_equal, assert_not_equal, assert_raises
 from django.conf.urls.defaults import patterns, url
 from django.conf.urls.defaults import patterns, url
 from django.core.urlresolvers import reverse
 from django.core.urlresolvers import reverse
 from django.http import HttpResponse
 from django.http import HttpResponse
@@ -322,7 +322,7 @@ def test_error_handling():
 def test_error_handling_failure():
 def test_error_handling_failure():
   # Change rewrite_user to call has_hue_permission
   # Change rewrite_user to call has_hue_permission
   # Try to get filebrowser page
   # Try to get filebrowser page
-  # test for werkzeug debugger
+  # test for default 500 page
   # Restore rewrite_user
   # Restore rewrite_user
   import desktop.auth.backend
   import desktop.auth.backend
 
 
@@ -338,15 +338,13 @@ def test_error_handling_failure():
     delattr(user, 'has_hue_permission')
     delattr(user, 'has_hue_permission')
     return user
     return user
 
 
-  def store_exc_info(*args, **kwargs): pass
-  c.store_exc_info = store_exc_info
-
   original_rewrite_user = desktop.auth.backend.rewrite_user
   original_rewrite_user = desktop.auth.backend.rewrite_user
   desktop.auth.backend.rewrite_user = rewrite_user
   desktop.auth.backend.rewrite_user = rewrite_user
 
 
   try:
   try:
-    response = c.get('/dump_config')
-    assert_true('AttributeError at /dump_config' in response.content, response)
+    # Make sure we are showing default 500.html page.
+    # See django.test.client#L246
+    assert_raises(AttributeError, c.get, '/dump_config')
   finally:
   finally:
     # Restore the world
     # Restore the world
     restore_django_debug()
     restore_django_debug()

+ 14 - 7
desktop/core/src/desktop/views.py

@@ -239,18 +239,25 @@ def serve_404_error(request, *args, **kwargs):
 
 
 def serve_500_error(request, *args, **kwargs):
 def serve_500_error(request, *args, **kwargs):
   """Registered handler for 500. We use the debug view to make debugging easier."""
   """Registered handler for 500. We use the debug view to make debugging easier."""
-  exc_info = sys.exc_info()
-  if desktop.conf.HTTP_500_DEBUG_MODE.get():
-    return django.views.debug.technical_500_response(request, *exc_info)
   try:
   try:
-    return render("500.mako", request, {'traceback': traceback.extract_tb(exc_info[2])})
-  except:
-    # Fallback to technical 500 response if ours fails
+    exc_info = sys.exc_info()
+    if exc_info:
+      if desktop.conf.HTTP_500_DEBUG_MODE.get() and exc_info[0] and exc_info[1]:
+        # If (None, None, None), default server error describing why this failed.
+        return django.views.debug.technical_500_response(request, *exc_info)
+      else:
+        # Could have an empty traceback
+        return render("500.mako", request, {'traceback': traceback.extract_tb(exc_info[2])})
+    else:
+      # exc_info could be empty
+      return render("500.mako", request, {})
+  finally:
+    # Fallback to default 500 response if ours fails
     # Will end up here:
     # Will end up here:
     #   - Middleware or authentication backends problems
     #   - Middleware or authentication backends problems
     #   - Certain missing imports
     #   - Certain missing imports
     #   - Packaging and install issues
     #   - Packaging and install issues
-    return django.views.debug.technical_500_response(request, *exc_info)
+    pass
 
 
 _LOG_LEVELS = {
 _LOG_LEVELS = {
   "critical": logging.CRITICAL,
   "critical": logging.CRITICAL,