kivy / oscpy

An efficient OSC implementation compatible with python2.7 and 3.5+
MIT License
111 stars 27 forks source link

Protect the server against exceptions in callbacks #53

Closed pakal closed 4 years ago

pakal commented 4 years ago

Describe the bug As of now, callbacks are called by the listener thread as "cb(address, *values)", without try...except protection around. So an exception in a callback will stop the server thread.

Expected behavior Maybe do some "catch all" around the callback call, and log potential errors? Or document that callbacks must by themselves prevent any excrption from flowing through?

tshirtman commented 4 years ago

I'm not a fan of eating errors, but maybe we could give a way for the user to handle exceptions in a way they see fit with such a try/except Exception construct, i think i would raise in the absence of such an error handler value, to avoid this being a surprising property of the system, but still allow users to opt into a more resilient mode.

pakal commented 4 years ago

I think most server frameworks (Django, Flask etc.) handle each request as a separate "task", and have a catch-all which log unhandled exceptions (including traceback), or perform other signalling (eg. sending emails to admins).

I guess a catch-all like that, relying on stdlib logging, would be just fine. It could be switchable by an init argument of the server indeed, even though I wonder: are there cases where having the server stop all operation on error is useful (since it's using UDP and handled by a secondary thread, it makes a quite silent error anyway, doesn't it ?)

(I could work on a PR once we see how it should preferably be done)

tshirtman commented 4 years ago

Indeed the fact that it's in a thread makes it less obvious that would be desirable that something went wrong, if you want to work on this, as long as it's possible to register a custom error handler so the calling program can be aware of error and plug whatever report logic it wants, i think that would be fine, and the default behavior could be to log the error, which is indeed better than the current behavior, since it can process further requests. Make sure the error handling interface is described in the server documentation, so it can be easily discovered, and to add tests for the behavior :).

pakal commented 4 years ago

Here is a PR which solves my various needs - resilience regarding callback exceptions, and ordered shutdown: https://github.com/kivy/oscpy/pull/54

tshirtman commented 4 years ago

closed in #54