Add Global Error Handler

[ocelotl]

Description

Add a global error handler. This object allows users to pass exceptions to pre-registered error handlers.

Fixes #1079

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added unit tests.

Checklist:

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[ffe4]

After taking a closer look, I think the examples I had in mind during the SIG can probably all be solved by either directly raising or re-raising the exception somewhere. E.g.:

try:
    do_stuff(arg)
except InvalidArg:
    with GlobalErrorHandler():
        raise InvalidArg
    do_something_else()

However, maybe having a .raise(Error) method on the handler would be more intuitive?

[ocelotl]

After taking a closer look, I think the examples I had in mind during the SIG can probably all be solved by either directly raising or re-raising the exception somewhere. E.g.:

try:
    do_stuff(arg)
except InvalidArg:
    with GlobalErrorHandler():
        raise InvalidArg
    do_something_else()

However, maybe having a .raise(Error) method on the handler would be more intuitive?

🤔 What is the purpose of do_something_else? Can that code be added to the handler for InvalidArg?

[ffe4]

🤔 What is the purpose of do_something_else? Can that code be added to the handler for InvalidArg?

do_something_else() is supposed to be error handling done by the SDK, so we want the error to be handled by the default handler. Let's just say do_stuff() had a return value that we wanted and now we have to get it another way.

As a general question, since I am not quite sure what the intention behind the spec was, could you maybe share your understanding of what kinds of errors a user should be able to register handlers for? The spec only mentions relevant errors, so I'm not quite sure where we would make use of these error handlers in the first place.

[ocelotl]

thinking What is the purpose of do_something_else? Can that code be added to the handler for InvalidArg?

do_something_else() is supposed to be error handling done by the SDK, so we want the error to be handled by the default handler. Let's just say do_stuff() had a return value that we wanted and now we have to get it another way.

Just for the record, here is more context related to relevant errors. I can think about a way of getting the return value of error handlers using the context manager, let me write something down...

[ffe4]

I can think about a way of getting the return value of error handlers using the context manager, let me write something down...

The example is supposed to be SDK internal. If we put all error handling code for relevant internal errors into error handlers, our code will become more fragmented. IMO the above example is better than one where we have to jump to another class to see what happens if we encounter an error. I also don't know if we want to give users the ability, or the obligation, to supply return values that get used in case of an error. At least the spec only talks about enabling strict error handling, so I think we should keep it simple.

[ocelotl]

I can think about a way of getting the return value of error handlers using the context manager, let me write something down...

The example is supposed to be SDK internal. If we put all error handling code for relevant internal errors into error handlers, our code will become more fragmented. IMO the above example is better than one where we have to jump to another class to see what happens if we encounter an error. I also don't know if we want to give users the ability, or the obligation, to supply return values that get used in case of an error. At least the spec only talks about enabling strict error handling, so I think we should keep it simple.

I can think about a way of getting the return value of error handlers using the context manager, let me write something down...

The example is supposed to be SDK internal. If we put all error handling code for relevant internal errors into error handlers, our code will become more fragmented. IMO the above example is better than one where we have to jump to another class to see what happens if we encounter an error. I also don't know if we want to give users the ability, or the obligation, to supply return values that get used in case of an error. At least the spec only talks about enabling strict error handling, so I think we should keep it simple.

The spec requires to be possible for users to register their own error handlers, that's basically what is being implemented here. I agree that giving the users the return value of an error handler is outside of the scope of the requirement and probably self defeating also, since it would require the application code to act on the registered handlers themselves, breaking the separation between application code and error handlers. Because of this, I have removed the return of any value in the error handlers.

[codeboten]

Tested this by walking through the doc, this looks great, just a few minor updates requested.

[codeboten]

Thanks for including my suggestions, this looks good to me.

[lzchen]

What is the reason for needing to inherit from an Exception type class? Is there a necessity that the behaviour of handle must rely on the type of the exception?

[ocelotl]

Please read comment below.

[lzchen]

I don't think I can see anything that enforces a specific handler to handle only certain types of errors (inheriting for an Error doesn't really enforce this). With that being said, this logic will run through all the handlers regardless of what kind of errors they are handling. Does this make sense if there are multiple handlers handling the same type of error already?

[ocelotl]

I imagine most implementations of the handlers having to deal with checking the exception type if this is not implemented somewhere else. It seems natural to me that handlers are made for specific exceptions, and they will not be interested in any other exception types, so I preferred to put this check in the global error handler. I am open to other design ideas, this is not something I consider definitive for this particular feature.

[lzchen]

That's fine then. I think we can leave it up to the user's discretion to check exception type and handle them appropriately (if they have multiple handlers executing on the same error type then that is up to them). Is the order of execution of the handlers known somehow? Or does that depend on which iter point is loaded first?

[ocelotl]

That would depend on the iter_points order. So, you prefer that the global error handler does not check exception type and all error handlers are called?

[lzchen]

I think the current implementation is fine.

[lzchen]

Just curious, are we going to wrap our entire sdk in an global error handler context to catch unhandled exceptions?

[ocelotl]

Just curious, are we going to wrap our entire sdk in an global error handler context to catch unhandled exceptions?

I was not thinking about that when I was writing this error handler, I am also not sure how "doable" that is (I mean, how certainly can we guarantee that we can catch any SDK-related exception).

[lzchen]

@ocelotl
Have a handler that just wraps the code in a giant try/catch and catch for generic exception. I'm just thinking of ideas, might not be feasible.