hoijui / JavaOSC

OSC content format/"protocol" library for JVM languages
http://www.illposed.com/software/javaosc.html
BSD 3-Clause "New" or "Revised" License
156 stars 43 forks source link

Don't stop listening if an invalid OSC address is received #28

Closed nlowe closed 5 years ago

nlowe commented 7 years ago

The default templates for andrOSC uses some invalid OSC addresses. Sending a message with these addresses throws an unhandled exception:

Exception in thread "Thread-4" java.lang.IllegalArgumentException: Not a valid OSC address: pad
    at com.illposed.osc.OSCMessage.checkAddress(OSCMessage.java:116)
    at com.illposed.osc.OSCMessage.<init>(OSCMessage.java:63)
    at com.illposed.osc.OSCParser.convertMessage(OSCParser.java:186)
    at com.illposed.osc.OSCParser.convert(OSCParser.java:100)
    at com.illposed.osc.transport.channel.OSCDatagramChannel.read(OSCDatagramChannel.java:111)
    at com.illposed.osc.transport.udp.OSCPortIn.run(OSCPortIn.java:150)
    at java.lang.Thread.run(Thread.java:745)

This seems to kill the listener thread. I don't see any way to listen for this to manually restart the listener. Ideally, I'd like to be able to log this and keep listening.

daveyarwood commented 5 years ago

I happened to notice that there is a patch for this on the develop branch: https://github.com/hoijui/JavaOSC/blob/cd36c67/modules/core/src/main/java/com/illposed/osc/transport/udp/OSCPortIn.java#L274-L283

Not sure if it's been released yet.

I would argue that the default behavior upon receiving a bad packet should be to log it and continue listening. To have the OSC server crash anytime someone sends it a bad message feels really unintuitive, and like it would be cumbersome for testing. If nothing else, maybe resilient should default to true?

hoijui commented 5 years ago

the commit is here: fde9bc2ac9e79aa5a274052b1d3ff4e5eb41f1d9 The master branch contains what is released. This fix is not released yet.

hoijui commented 5 years ago

Hmm.. I guess my rationale back then (for resilient defaulting to false) was, that once bad data is received, it woudl be unlikely that the state of OSCPortIn wouldl line-up again with the incomming stream of data. As in: after noticing a bad address, OSCPortIn might reset itsself and await a new packet to come, but the incomming steam still contains the remainder of the packet, and ther will be more errors, and there might never be a recovery. Maybe I was too pesmimistic back then... not even looking at the code. Either way, the correct thing to do would probably be to write a unit-test.

daveyarwood commented 5 years ago

Many programs (DAWs, etc.) have OSC APIs that let users control them from other programs, controllers, etc. I think these programs would need to be able to recover from garbage messages coming from end users. As an end user trying to integrate with one of these programs by writing my own program that sends OSC messages, I would want the OSC receiver to simply ignore any messages I send that aren't valid, as opposed to crashing.

daveyarwood commented 5 years ago

Now that I'm thinking about it more: what if it were required to implement a method for dealing with invalid messages? That way the library user has to specify what to do. I feel like that might be better than the two options we have now: silently ignoring bad messages, or crashing.

hoijui commented 5 years ago

hmm... I don't think so. Bad data either means the sender uses invalid code/sends invalid data, or this library has a bug/is not up to the current standard. either way, there is library code to be changed on the sending or receiving side. Facilitating the user to make a hacky workaround seems not like the right way to go. If the user really feels that is the right way, he may as well hack the library code directly .. but the first message.. yes!