Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Class based namespaces error handling #834

Closed
wants to merge 1 commit into from

Conversation

codyleyhan
Copy link

Adds the ability to have an on_error handler on a class based namespace that will handle any errors within the namespace rather than having to register it separately.

Addresses #832

@miguelgrinberg
Copy link
Owner

There are a couple of issues with your implementation:

  1. If you use on_error for the namespace error handler you make it impossible for an event named error to be used. I would prefer something that does not use the on_ prefix, such as exception_handler.
  2. When you register the namespace error handler you erase any decorator based error handler that may have been set earlier. From that point on, the namespace error handler will be triggered even for events that are implemented as a decorator based handler.

What I would do to address the second problem is to handle namespace errors directly in the namespace class, without having to be registered. When trigger_event() calls into _handle_event(), it can pass a custom error handler that overrides the registered one, for example.

@codyleyhan
Copy link
Author

Hey Miguel thanks for giving feedback so quickly, I will fix the first point!

As for the second point, I don't think that I am understanding. From what I can tell, I think this would this have the similar affect without overriding the decorated handler for the namespace, but the one decorated handler would never be called?

@miguelgrinberg
Copy link
Owner

miguelgrinberg commented Nov 16, 2018

@codyleyhan here is an example that shows the problem:

@socketio.on_error('/test')
def error_handler():
    print('standalone error handler invoked'

@socketio.on('some_event', namespace='/test')
def some_event_handler(data):
    raise RuntimeError("some_event error")

class MyCustomNamespace(Namespace):
    def on_error(self):
        print('namespace error handler invoked')

    def on_another_event(self, data):
        raise RuntimeError('another_event error')

socketio.on_namespace(MyCustomNamespace('/test'))

With this example, if the client emits some_event, the error handler in the namespace will be invoked. The decorated error handler is actually not registered at all and is overriden by the one in the namespace class.

Now, if I were to move things around as follows:

class MyCustomNamespace(Namespace):
    def on_error(self):
        print('namespace error handler invoked')

    def on_another_event(self, data):
        raise RuntimeError('another_event error')

socketio.on_namespace(MyCustomNamespace('/test'))

@socketio.on_error('/test')
def error_handler():
    print('standalone error handler invoked'

@socketio.on('some_event', namespace='/test')
def some_event_handler(data):
    raise RuntimeError("some_event error")

Now the registered event handler is the decorated one, and it will be invoked even for the errors in the namespace handlers. In a large application that uses multiple modules, the order things get imported could determine where the error handler is.

I want a more reliable solution that is deterministic, and independent of the order handlers are defined. What I think makes most sense is that the decorated error handler is used for decorated event handlers, and the namespace error handler is used for namespace event handlers. If the namespace does not have an error handler, then the decorated error handler should also be used for namespace events.

I think a possible solution is to not register the namespace error handler with on_error, and instead just invoke it directly from the trigger_event method when an exception is raised in the invoked handler.

@miguelgrinberg
Copy link
Owner

Closing this due to inactivity. Please reopen or open a new PR with the changes I requested if you'd like to continue with this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants