Catching exceptions raised from callbacks attached to GLFW - improvement idea

[swistakm]

Currently there is no way to catch errors from callbacks attached to glfw using glfw.set_*_callback.

Consider following example:

import glfw

def error_callback(err, description):
    raise RuntimeError("GLFW error (%s): %s" % (err, description))

def main():
    # this will fail because glfw was not initialized yet
    glfw.set_error_callback(error_callback)
    try:
        window = glfw.create_window(640, 480, "Hello World", None, None)
    except Exception as err:
        print("Exception raised: %s" % err)
        exit(1)

    if not window:
        print("Exception not raised but window creation failed")
        glfw.terminate()
        return

if __name__ == "__main__":
    main()

When executed it gives following output:

$ python glfwerrs.py 
Traceback (most recent call last):
  File "_ctypes/callbacks.c", line 314, in 'calling callback function'
  File "glfwerrs.py", line 6, in error_callback
    raise RuntimeError("GLFW error (%s): %s" % (err, description))
RuntimeError: GLFW error (65537): The GLFW library is not initialized
Exception not raised but window creation failed

This means that exception is raised but the only effect is that traceback is printed on standard output. Program simply continues its flow. This can be problematic in some situations. For instance if we have keyboard handler that fails at some point and it is also responsible for catching input that should close application (ESC key) then user will find difficult to exit such program. Unexpected exceptions should be propagated in stack so would terminate application if not handled properly. For this case it is not such big issue because glfw.create_window informs about error with output value but (as stated by GLFW docs) not all functions does that and the only way to reliably handle errors is through error callbacks.

Proper propagation of exceptions could be achieved if we provide default error callback that stores latest error state (e.g. in some global/module-level variable) and utilise ctypes' errcheckprotocol by attaching custom errcheck function to every function in glfw._glfw library instance.

Here is simple proof of concept of how it could work:

import glfw
_errors = [None]

def _errcheck(result, func, args):
    if _errors[0] is not None:
        _errors[0], error = None, _errors[0]
        raise error
    return result

for function in glfw._glfw.__dict__.values():
    if hasattr(function, 'errcheck'):
        function.errcheck = _errcheck

def error_callback(err, description):
    _errors[0] = RuntimeError("GLFW error (%s): %s" % (err, description))

def main():
    glfw.set_error_callback(error_callback)
    window = glfw.create_window(640, 480, "Hello World", None, None)

    if not window:
        print("Exception not raised but window creation failed")
        glfw.terminate()
        return

if __name__ == "__main__":
    main()

This approach also gives possibility to wrap all the errors with some custom base exception class e.g. GlfwException or even differentiate different kind of errors with different error classe by inspecting error codes.

Above example is of course not complete because:

• Adding some custom error callback will break backwards compatibility and it is highly possible that this package users made their own workarounds or are simply happy with this particular behaviour.
• This is completely not thread safe. If thread safety is a concern (I suppose it is) then errors should be stored in per-thread (maybe using dict, proper locking and thread.get_ident() as state key)

I can happily provide pull request for that change but I would like first to know what do you think about that and how would you prefer to solve some issues:

1. should be the behaviour opt-in or opt-out? If opt-in then how it should be triggered? I see few options:
   1. additional function in glfw API like enable_exception_propagation() that will initialize error state structure, set default handler and attach errcheck to each glfw._glfw (DLL) function
   2. add custom errcheck to every library function but internally verify if error propagation was enabled (again new API function).
2. Is thread safety a concern and if so do you have any special requirements about how it should be achieved?
3. All additional API elements that would be created are something additional that is not found in GLFW. It would be great to somehow inform the package user that they are not a part of GLFW function mapping but something specific only for this single python wrapper. Is information in README enough or maybe we should use specific naming convention or even some submodule?

I think also that it could be very useful to expose some more internals of above behaviour so user could easily add their own error callbacks that will still properly propagate exceptions. Maybe some decorator?

As a side note about defining opt-in mechanism: I would not recommend using environment variables for defining expected behaviour because the end-user could accidentally completely corrupt application flow if it would heavily rely on exception handling.

EDIT: I have realised that I had mixed up two different things. One thing is propagating GLFW errors as exceptions and the other thing is propagating exceptions from python input handlers (keyboard, mouse, joystick etc.). Still I think that we can solve two issues using similar approach and propagating GLFW function errors as exceptions is a good start.

[FlorianRhiem]

Thank you for your suggestion!

Let's start with the exception forwarding:

Do you know a case of when forwarding callback exceptions to the main exception context would not be a good idea? Aside of the error callback, all other callbacks are only called from within glfwPollEvents and glfwWaitEvents, so if a user wishes to ignore callback exceptions, I think:

try:
    glfw.poll_events()
except:
    pass

would be the pythonic way to do it in user code, and it would already show the user that ignoring these exceptions might not be a good idea.

Thread-safety is no issue here, as the relevant GLFW functions are not thread-safe anyway.

I'd suggest the following behavior: If an uncaught exception causes a callback to exit, the exception is re-raised from the current PyGLFW function. The stack trace is not printed. If the callback was called from inside glfw.poll_events or glfw.wait_events, execution of other callbacks is disabled for this call of the function (and of course re-enabled for future calls of the function).

This leads to the typical behavior of executions: prevent code execution and jump to the next except or quit the program. This would also mean that there can only be one "buffered" exception at a time. I think this would cause the least surprise to a Python developer.

What are your thoughts about this suggested behavior?

---

For optional features like error checking, I think the approach of using module level constants as done in PyOpenGL is quite good.

import glfw
glfw.ERROR_REPORTING = False

This code would opt-out of the automatic propagation of GLFW errors to Python exceptions. I guess this would be the default error callback and setting the error callback to None would actually re-install this error-to-exception callback?

Alternatively, an explicit glfw.set_error_callback(None) could disable the default?

[swistakm]

I agree with all suggestions. Especially the fact about threading support in these parts of GLFW is a good point. Unfortunately I won't be able to submit any PR in next two weeks but will do that eventually if no one will take care of that in the meantime. I have already did some tests in one of my projects and it seems like there shouldn't be any problems in implementing this as proposed.

[FlorianRhiem]

Please take a look at commit 37fbb8c and let me know if you have any suggestions on how to enhace this mechanism. The way I replace the callback function types looks a bit hack-ish, but that way there's less of a chance to forget the callback decorator when adding support for new callbacks.

[swistakm]

Everything looks great. Exactly how I imagined it :)

[FlorianRhiem]

That's good to hear. I merged the branch into master and uploaded the new version to PyPI. Thank you for your help!