From 87751375466e23c3e2ed17567ab5200c82044f08 Mon Sep 17 00:00:00 2001 From: Veiko Aasa Date: Sun, 13 Dec 2020 20:54:28 +0200 Subject: [PATCH] diagnostics: Improve exception handling in app diagnostics Fixes the problem when the app throws an error when running diagnostics, the app's diagnostics page shows an html 500 error and on the diagnostics app page, only an excpetion message is shown without other apps diagnostics. (I encountered this when the network app diagnostics threw an exception because of a particular network configuration.) Tests performed: - Checked that an app diagnostics page and the diagnostics app shows all info correctly. - Modified the networks app diagnostics code so that it throws an exception. Checked that the app diagnostics page shows an exception message and the diagnostics module shows all the apps and the networks app section shows an exception message. Signed-off-by: Veiko Aasa [sunil: Re-add removed relevant comment] [sunil: Remove ability to dismiss the exception message, not much gain for user] [sunil: Remove unnecessary redundant if condition in template] Signed-off-by: Sunil Mohan Adapa Reviewed-by: Sunil Mohan Adapa --- plinth/modules/diagnostics/__init__.py | 37 +++++++++---------- .../diagnostics/templates/diagnostics.html | 35 ++++++++---------- .../templates/diagnostics_app.html | 4 ++ plinth/modules/diagnostics/views.py | 25 ++++++++++--- 4 files changed, 58 insertions(+), 43 deletions(-) diff --git a/plinth/modules/diagnostics/__init__.py b/plinth/modules/diagnostics/__init__.py index 02d4814fb..7bbf7d15b 100644 --- a/plinth/modules/diagnostics/__init__.py +++ b/plinth/modules/diagnostics/__init__.py @@ -78,22 +78,10 @@ def start_task(): if running_task: raise Exception('Task already running') - running_task = threading.Thread(target=_run_on_all_enabled_modules_wrapper) + running_task = threading.Thread(target=run_on_all_enabled_modules) running_task.start() -def _run_on_all_enabled_modules_wrapper(): - """Wrapper over actual task to catch exceptions.""" - try: - run_on_all_enabled_modules() - except Exception as exception: - logger.exception('Error running diagnostics - %s', exception) - current_results['error'] = str(exception) - - global running_task - running_task = None - - def run_on_all_enabled_modules(): """Run diagnostics on all the enabled modules and store the result.""" global current_results @@ -111,7 +99,6 @@ def run_on_all_enabled_modules(): apps = [] for app in app_module.App.list(): - # XXX: Implement more cleanly. # Don't run diagnostics on apps have not been setup yet. # However, run on apps that need an upgrade. module = importlib.import_module(app.__class__.__module__) @@ -125,18 +112,30 @@ def run_on_all_enabled_modules(): continue apps.append((app.app_id, app)) - current_results['results'][app.app_id] = None + app_name = app.info.name or _('None') + current_results['results'][app.app_id] = {'name': app_name} current_results['apps'] = apps for current_index, (app_id, app) in enumerate(apps): - app_name = app.info.name or _('None') - current_results['results'][app_id] = { - 'name': app_name, - 'results': app.diagnose() + app_results = { + 'diagnosis': None, + 'exception': None, } + + try: + app_results['diagnosis'] = app.diagnose() + except Exception as exception: + logger.exception('Error running %s diagnostics - %s', app_id, + exception) + app_results['exception'] = str(exception) + + current_results['results'][app_id].update(app_results) current_results['progress_percentage'] = \ int((current_index + 1) * 100 / len(apps)) + global running_task + running_task = None + def _get_memory_info_from_cgroups(): """Return information about RAM usage from cgroups.""" diff --git a/plinth/modules/diagnostics/templates/diagnostics.html b/plinth/modules/diagnostics/templates/diagnostics.html index 6e8ac9422..1e726504d 100644 --- a/plinth/modules/diagnostics/templates/diagnostics.html +++ b/plinth/modules/diagnostics/templates/diagnostics.html @@ -31,26 +31,23 @@ {% if results %}

{% trans "Results" %}

- {% if results.error %} -
- × - {{ results.error }} -
- {% else %} - {% for app_id, app_data in results.results.items %} -

- {% blocktrans with app_name=app_data.name %} - App: {{app_name}} - {% endblocktrans %} -

+ {% for app_id, app_data in results.results.items %} +

+ {% blocktrans with app_name=app_data.name %} + App: {{app_name}} + {% endblocktrans %} +

- {% if app_data.results %} - {% include "diagnostics_results.html" with results=app_data.results %} - {% else %} -

- {% endif %} - {% endfor %} - {% endif %} + {% if app_data.diagnosis %} + {% include "diagnostics_results.html" with results=app_data.diagnosis %} + {% elif app_data.exception %} + + {% else %} +

+ {% endif %} + {% endfor %} {% endif %} {% endblock %} diff --git a/plinth/modules/diagnostics/templates/diagnostics_app.html b/plinth/modules/diagnostics/templates/diagnostics_app.html index 43b593b51..6d74d2f15 100644 --- a/plinth/modules/diagnostics/templates/diagnostics_app.html +++ b/plinth/modules/diagnostics/templates/diagnostics_app.html @@ -13,6 +13,10 @@ {% if results %} {% include "diagnostics_results.html" with results=results %} + {% elif exception %} + {% else %}

{% trans "This app does not support diagnostics" %}

{% endif %} diff --git a/plinth/modules/diagnostics/views.py b/plinth/modules/diagnostics/views.py index 0c81f7440..097e9fc25 100644 --- a/plinth/modules/diagnostics/views.py +++ b/plinth/modules/diagnostics/views.py @@ -3,6 +3,8 @@ FreedomBox app for running diagnostics. """ +import logging + from django.http import Http404 from django.template.response import TemplateResponse from django.utils.translation import ugettext_lazy as _ @@ -11,6 +13,8 @@ from django.views.decorators.http import require_POST from plinth.app import App from plinth.modules import diagnostics +logger = logging.getLogger(__name__) + def index(request): """Serve the index page""" @@ -35,8 +39,19 @@ def diagnose_app(request, app_id): except KeyError: raise Http404('App does not exist') - return TemplateResponse(request, 'diagnostics_app.html', { - 'title': _('Diagnostic Test'), - 'app_id': app_id, - 'results': app.diagnose() - }) + diagnosis = None + diagnosis_exception = None + try: + diagnosis = app.diagnose() + except Exception as exception: + logger.exception('Error running %s diagnostics - %s', app_id, + exception) + diagnosis_exception = str(exception) + + return TemplateResponse( + request, 'diagnostics_app.html', { + 'title': _('Diagnostic Test'), + 'app_id': app_id, + 'results': diagnosis, + 'exception': diagnosis_exception, + })