Safely shut down ganga to prevent bad things #794

rob-c · 2016-09-16T08:19:34Z

Fixes #793

This replaces the erroneous template error and raises a new GangaException when a plugin fails to launch.

This correctly triggers a stop to the Ganga startup process which hopefully should allow the user a chance to fix whatever in their system/configuration is causing the problems. (or to report a bug).

milliams · 2016-09-16T10:40:35Z

python/Ganga/Utility/Runtime.py

@@ -257,12 +257,14 @@ def loadPlugins(environment):
            logger.error('problems with loading Named Templates for %s', n)
            logger.error('Reason: %s' % str(err))

-    for r in allRuntimes.values():
+    for n, r in zip(allRuntimes.keys(), allRuntimes.values()):


Why not:

for n, r in allRuntimes.items():

Following coding style above so I'd rather stick to what's already used for sake of readability.

Now defunct since latest commit but github is no longer showing this.

milliams · 2016-09-16T10:41:56Z

python/Ganga/Utility/Runtime.py

            logger.error('Reason: %s' % str(err))
+            raise GangaException("Failed to load plugin: %s. Ganga will now shutdown to prevent job corruption." % n)


We should probably avoid raising bare GangaExceptions. Instead a new exception type (GangaPluginError or similar) should be used so that when it's caught you know what happened.

GangaPluginError it is then.

Which doesn't exist and pulling in Core code here is against the point of splitting Core and Utlity... I'll create a new Exception type for the sake of this then.

This just makes a mess of things frankly. I don't want to global import a file from Core here as that is clearly bad and I can't create a class deriving from GangaException here which we want to avoid throwing an un-caught exception to the user. (Which will arrive from some plugins!) I'm opting to keep the import here.

I'm confused - I think @milliams was just saying it would be good to have a new exception class for this type of exception and import that from the exceptions in Core instead of GangaException, i.e. just create a new Exception class in Core.exceptions and import+raise that instead.

Why can't we 'global import a file from Core here'? This file should be able to depend on any Core module just fine.

I'm reluctant to start going around making new exceptions for all the different situations, we just end up with a dictionary of possible classes in the exception which doesn't really shed much light imho.

Fine, I'll try a global import again, but once again I strongly feel this is the wrong thing to be doing here as we shouldn't really have Core classes being imported into Utility imo as well as the amount of extra stuff pulled in out of a sane order or safe way when we do this.

I would personally argue that this is a big enough collection of errors (i.e. any failure to load a plugin) that it's worth it's own exception. However I agree that in general we would want to avoid a proliferation of these.

But we need to do the Core import at some point to use the exception at all and therefore it's just a question of where it goes. The import itself won't pull anything in outside of the exceptions looking at the code and we (or at least me) have been trying to make these imports as safe as possible so there is an absolute minimum of stuff done at import time for just this reason.

I've fixed this to what is requested as per the latest commit (Edit: now uploaded) but this whole file (in fact a lot of files) need to have the import situation sorted out.
Fixing 1 or 2 methods ad-hoc just makes the files messy imo and instead of each method importing what it needs where it needs it (yes a bad thing) we now have a horrible mix of global and local imports which should probably be 90%+ global imports due to a handful of circular dependencies.

I totally agree with this sentiment and I believe this should be achievable outside the Logger and the Config systems. In fact I have a (very old) branch that was doing exactly this (converting all local imports to global and making sure everything was OK) but I didn't get particularly far with it as it was low priority. I'll try to revisit this in the future.

milliams · 2016-09-16T10:42:24Z

python/Ganga/Utility/Runtime.py

        try:
            r.loadPlugins()
        except Exception as err:
-            logger.error('problems with loading Named Templates for %s', n)
+            from Ganga.Core.exceptions import GangaException


This import should be at the top of the file if possible.

No, you pull in too much accidental stuff by going near Ganga.Core

I may be missing something but looking at the code you don't pull in anything from the __init__ of Ganga.Core and in most other places in the code base I've seen the exception imports are at the top of the code. Is there a circular import problem?

rob-c · 2016-09-19T11:32:31Z

What is the status of this? I'm looking to merge as we need this for #755 to raise correctly.

drmarkwslater · 2016-09-19T11:40:27Z

python/Ganga/Utility/Runtime.py

            logger.error('Reason: %s' % str(err))
+            raise GangaException("Failed to load plugin: %s. Ganga will now shutdown to prevent job corruption." % n)


I'm confused - I think @milliams was just saying it would be good to have a new exception class for this type of exception and import that from the exceptions in Core instead of GangaException, i.e. just create a new Exception class in Core.exceptions and import+raise that instead.

drmarkwslater · 2016-09-19T11:44:42Z

python/Ganga/Utility/Runtime.py

        try:
            r.loadPlugins()
        except Exception as err:
-            logger.error('problems with loading Named Templates for %s', n)
+            from Ganga.Core.exceptions import GangaException


I may be missing something but looking at the code you don't pull in anything from the __init__ of Ganga.Core and in most other places in the code base I've seen the exception imports are at the top of the code. Is there a circular import problem?

…ds fixing

rob-c · 2016-09-19T12:08:19Z

Changes made, this should be ready for merging I think

milliams · 2016-09-19T12:18:27Z

python/Ganga/Core/exceptions.py

+    def __str__(self):
+        """ Return a string explainng our message, just like GangaException """
+        return "PluginError: %s" % self.msg
+


This whole class could be as simple as:

class PluginError(GangaException): pass

and inheritance will take care of the rest.

This would also make it fit in nicely with #752 actually. I should really look in to getting that merged...

rob-c · 2016-09-19T12:23:44Z

And now?

Safely shut down ganga to prevent bad things

2122e2c

rob-c added bug Core labels Sep 16, 2016

rob-c added this to the 6.2.1 milestone Sep 16, 2016

rob-c self-assigned this Sep 16, 2016

milliams suggested changes Sep 16, 2016

View reviewed changes

Minor change

3e7d21f

rob-c mentioned this pull request Sep 16, 2016

work to stabilize GangaDirac API #788

Merged

Merge branch 'develop' into exitOnCrash

47a4e0f

drmarkwslater requested changes Sep 19, 2016

View reviewed changes

Adding PluginError, used once. Making import global, rest of file nee…

2ec3d9a

…ds fixing

milliams suggested changes Sep 19, 2016

View reviewed changes

Update exceptions.py

625562e

milliams approved these changes Sep 19, 2016

View reviewed changes

drmarkwslater approved these changes Sep 19, 2016

View reviewed changes

rob-c merged commit 08a339a into develop Sep 19, 2016

rob-c deleted the exitOnCrash branch September 19, 2016 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Safely shut down ganga to prevent bad things #794

Safely shut down ganga to prevent bad things #794

rob-c commented Sep 16, 2016

milliams Sep 16, 2016

rob-c Sep 16, 2016

rob-c Sep 16, 2016

milliams Sep 16, 2016

rob-c Sep 16, 2016

rob-c Sep 16, 2016

rob-c Sep 16, 2016

drmarkwslater Sep 19, 2016 •

edited

milliams Sep 19, 2016

rob-c Sep 19, 2016

drmarkwslater Sep 19, 2016

rob-c Sep 19, 2016 •

edited

drmarkwslater Sep 19, 2016

milliams Sep 16, 2016

rob-c Sep 16, 2016

drmarkwslater Sep 19, 2016

rob-c commented Sep 19, 2016

drmarkwslater Sep 19, 2016 •

edited

drmarkwslater Sep 19, 2016

rob-c commented Sep 19, 2016

milliams Sep 19, 2016 •

edited

drmarkwslater Sep 19, 2016

rob-c commented Sep 19, 2016

		logger.error('Reason: %s' % str(err))
		raise GangaException("Failed to load plugin: %s. Ganga will now shutdown to prevent job corruption." % n)

Safely shut down ganga to prevent bad things #794

Safely shut down ganga to prevent bad things #794

Conversation

rob-c commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drmarkwslater Sep 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob-c Sep 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob-c commented Sep 19, 2016

drmarkwslater Sep 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob-c commented Sep 19, 2016

milliams Sep 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob-c commented Sep 19, 2016

drmarkwslater Sep 19, 2016 •

edited

rob-c Sep 19, 2016 •

edited

drmarkwslater Sep 19, 2016 •

edited

milliams Sep 19, 2016 •

edited