search

kivy / oscpy

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

Server callback error handling #60

Closed ChihayaK closed 3 years ago

ChihayaK commented 3 years ago

Is your feature request related to a problem? Please describe. I noticed that when error happens in the OSCThreadServer callback, the thread will crash. For example, if I bind a function to the server which has two arguments, but some evil client decided to send a message with three arguments. It will causes the server to crash.

Describe the solution you'd like Error handling in OSCThreadServer._listen function. And it is already available in commit e33716986b4a3a68d898ec9fb6ee5f95210e021c .

Describe alternatives you've considered Currently I am overriding the _listen function to implement my own error handling when cb happened, but it will be nice to have a release that supports this kind of feature.

tshirtman commented 3 years ago

Hello,

Indeed sounds like a fair requirement, (I’d argue you could add *args to all your callback signatures to work around it, but I understand you might not want that) I’m afraid I’m not going to work on it myself any time soon, but I would certainly accept a PR to achieve that if you want to do it.

Adding a new argument to the OSCThreadServer’s __init__ is probably the most natural option, but i’m not sure what should be the spec for cases where both intercept_errors and that new argument would be passed, or if they should be exclusive, then it might be better to reuse the argument, to allow passing a callback instead of a boolean, and if the argument is callable, call it instead of logging? But I’m not sure about such shenanigans, it might bite later.

ChihayaK commented 3 years ago

Hi tshirtman,

Thanks for the quick reply. I think I wrote something confusing above. My goal is to prevent server from crashing when something unexpected happened during the callback. And there is already a commit(e337169) on the master branch that fixed this issue. In other words, I am asking for a new release that includes that commit if possible. AFAIK the if there is an arg count mismatch, it will be caught as something similar as below:

TypeError: FunctionName() missing 1 required positional argument: 'argumentName'

tshirtman commented 3 years ago

Oh, sorry, i though you wanted additional features to what is in there, if releasing the current codebase is enough, yeah, we can certainly do that by pushing a tag, i don’t think there is any outstanding bug preventing that.

edit: and you are right, my proposed workaround would only work if there are too many arguments, not not enough.

ChihayaK commented 3 years ago

Thank you! I am very happy to hear that.