Uploaded image for project: 'Puppet'
  1. Puppet
  2. PUP-4089

Refactor and Cleanup Error handling



    • Type: Improvement
    • Status: Accepted
    • Priority: Normal
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
    • Template:
    • Team:
    • Story Points:


      Error handling in Puppet is in a sorry state since specification of how various errors and supporting infrastructure should be used is missing. This has led to a plethora of methods that are very flexible in accepting different types of arguments and perform "helpful" work before errors are logged and raised.

      This leads to an almost impossible situation when trying to get the right message and stack trace through the maze and into the hans of a user.

      PuppetError - has the capability to wrap an original error. Nothing is stated if this should be used to show a tower of exceptions (both backtraces) or, if the intent is to use the backtrace of the original. Also, if the original is a Puppet::Error, it may in turn have an original (which seems to be lost).
      OTOH: creating towers of backtraces is one reason Java has a bad reputation - it is (almost) never the right thing to do.

      ParseError, DevError and ResourceError has mixed in ExternalFile support, and can describe a file, line and pos. These errors are however often used without supplying any location information. Some errors even produce the message already formatted with location information. This leads to difficulties in knowing if an error should be decorated with the Puppet Code last known/current location (in .pp).

      In additional to this, the Logging framework adds extra text if it receives a PuppetError (including ParseError, DevError, and ResourceError) - it then outputs WrappedException - and the message from the wrapped exception.

      The 3.x. AST's safeevaluate method will wrap exceptions in a PuppetError with the original being the cause. When doing this it simply duplicates
      the message. (Somewhere magic occurs and the wrapped exception is stripped off. But not everywhere... And sometimes location information is lost.

      The DevError shows that there was once the intent to differ between errors that occur at runtime that could not possibly have been introduced by the .pp logic being evaluated. (e.g. it is a kind of "InternalError"). This reversed logic; all ruby errors are treated as if they relate to the puppet logic (raising ArgumentError in a function) makes it impossible to differentiate argument errors caused by the user from those caused by "internal errors".

      It is really difficult to fix these problems piece by piece since all error handling is so entangled. When fixing the problems we will have difficulties in changing some of the behaviors now found in ParseError (it is the most used by everyone, in puppet's code base, and in modules).

      Instead, we need to create new and well defined error classes, fix the puppet code base, while still supporting the old error classes. We then deprecate the old classes.

      The first part of this is to write a specification and guidelines for which error class to use when, and how. That work needs to start with an inventory of different use cases (what is done now). We then need to specify what constitutes wanted behavior.

      Things to consider:

      • is this an issue caused by the .pp logic, if so, is it known which location in .pp that should be reported or not
      • is this an "internal error"
      • how should logic that catches an error behave - should it adopt the backtrace or create a backtrace tower
      • is a tower of value to users or to developers of puppet only (i.e. see difference between actual problem, and how the puppet logic arrived at this problem (what if there is a problem in the error handling)


          Issue Links



              henrik.lindberg Henrik Lindberg
              0 Vote for this issue
              1 Start watching this issue



                  Zendesk Support