openframeworks / openFrameworks

openFrameworks is a community-developed cross platform toolkit for creative coding in C++.
http://openframeworks.cc
Other
9.97k stars 2.56k forks source link

ofxOsc needs broadcast + multiple clients on single port #722

Closed kylemcdonald closed 12 years ago

kylemcdonald commented 13 years ago

i believe the code posted here is patched for both those requirements:

https://github.com/kylemcdonald/DohaInstallation/tree/master/addons/ofxOsc/libs/oscpack/src

i have access to the oscpack repo and need to submit the changes + test across systems.

also, some discussion here: http://forum.openframeworks.cc/index.php/topic,1625.0/topicseen.html

bilderbuchi commented 13 years ago

great, I agree, those would be very useful (especially multiple listeners on single port)! while you're at it, could you maybe spare a look here: https://github.com/openframeworks/openFrameworks/issues/701 ?

bilderbuchi commented 13 years ago

@kylemcdonald: anything I can do to help here (testing etc)? Did you make any progress with integrating your changes into oscpack?

kylemcdonald commented 13 years ago

@bilderbuchi i haven't made any progress. the main issue is that it needs to be tested across platforms. if it's possible for you to check these changes, i can be responsible for committing back to oscpack.

bilderbuchi commented 13 years ago

I can offer checking on ubuntu 11.10 32 and 64bit, and probably on win7 64bit (but I haven't touched that in a long while). Could you open an openframeworks branch with your changes, which we can hack on together? Or how should we do this?

kylemcdonald commented 12 years ago

so i can't commit back to oscpack anymore because of some complex things with google changing how accounts work.

but i just added the minor necessary changes to the repo you made, christoph https://github.com/bilderbuchi/oscpack/

the next step would be compiling the lib for each of the platforms, and testing them that they haven't broken. which is the part that takes a bit. but hopefully next time we need to compile oscpack we can do this.

bilderbuchi commented 12 years ago

Thank you!

When we have tested oscpack, we can just send a diff back to Ross I think. regarding the code: 1) Shouldn't that be sizeof(reuse) instead of sizeof reuse? 2) Why do you cast the int to char* in the first setsockopt, but not in the second? 3) The original oscpack has unit tests iirc. Should we/can we add tests for multicast and multi-listeners? 4) Also, reading the OF thread again, one time you have socket_, and one time socket, is that correct?

kylemcdonald commented 12 years ago

oh no, i should have read the code closer!

there were definitely some weird formatting things that happened during the migration from the old forum, so there could be some characters missing. i was just copying directly from the forum post.

i just checked against a modded working version of ofxOsc, and i have written:

// arturo
int on = 1;
setsockopt(socket_, SOL_SOCKET, SO_BROADCAST, (char*) &on, sizeof(on));

// allow socket reuse!
int reuse = 1;
setsockopt(socket_, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof reuse);

the not having parens around sizeof thing is kind of weird, but i see examples of it here http://beej.us/guide/bgnet/output/html/multipage/setsockoptman.html

i just normalized the calls and re-pushed.

bilderbuchi commented 12 years ago

No problem. :-)

In the link you just posted, it says

Warning: on some systems (notably Sun and Windows), the option can be a char instead of an int, and is set to, for example, a character value of '1' instead of an int value of 1. Again, check your own man pages for more info with "man setsockopt" and "man 7 socket"!

so that's probably where the (char*) comes from. This still is wrong now in the win32 version, I believe.

sizeof seems to work with and without parentheses, but if I grasp this correctly, it's safer to use parentheses.

kylemcdonald commented 12 years ago

ah, ok -- so even the version posted in the forum was probably wrong, because it was still using an int but casting the pointer to a char. i'm not sure that would work.

i just pushed a change that should handle that. if you have the opportunity to make a windows test at some point, that would be great!

ofTheo commented 12 years ago

what would make this way easier to implement is if we included oscpack non-compiled. right now adding the features requires us to rebuild oscpack for a ton of platforms and its not a huge library

bilderbuchi commented 12 years ago

Sounds like a good idea I think. As soon as we have tested the changes, we can include oscpack as source, and send the relevant changes upstream.

ofTheo commented 12 years ago

as far as I can tell SO_REUSEADDR doesn't allow multiple binds on the same local port. when I try this I get a bind error on the second bind.

it seems to be merely a solution to give you a better chance to bind to a port that is no longer in use.

from: http://stackoverflow.com/questions/2384562/double-udp-socket-binding-in-linux

"SO_REUSEADDR doesn't let you bind to an endpoint twice, though. Its purpose is to override the TIME_WAIT state after you close a TCP socket. Normally the OS keeps a TCP socket in TIME_WAIT for a few minutes to pick up any "late" packets that haven't arrived yet. If you try to open a new socket you get EADDRINUSE unless you specify SO_REUSEADDR which kills the TIME_WAIT socket. – John Kugelman Mar 5 '10 at 5:25"

also from: http://beej.us/guide/bgnet/output/html/multipage/setsockoptman.html

"SO_REUSEADDR: Allows other sockets to bind() to this port, unless there is an active listening socket bound to the port already. This enables you to get around those "Address already in use" error messages when you try to restart your server after a crash."

anyway good to know as I assumed with this fix, we could bind to the same port at the same time.

kylemcdonald commented 12 years ago

hmm, i'm confused. when i made this modification to oscpack initially, it was precisely because i needed to bind to the same port multiple times. i had two apps running on one machine (really, the same app, just two instances) both listening to osc from a master computer, and both listening on the same port. this was all with linux.

maybe there is some kind of difference between trying to bind to the same port within one instance of an app vs binding to the same port from different instances? i wish i understood the details, all i know is that this must work on some level...

ofTheo commented 12 years ago

yes - that is what I thought it could do as well :)

I spent the last 3 hours trying to make it work. but no luck. all the docs say that the SO_REUSEADDR is to skip the time wait that normally occurs and binding on the same port ( for incoming data ) isn't possible. I'm curious what you did.

if you want to test it do the following in oscReceiverExample. also check you don't have oscpack.a in your project settings, the project generator is still adding that in for me.

ofxOscReceiver receiver2;

//--------------------------------------------------------------
void testApp::setup(){
    // listen on the given port
    cout << "listening for osc messages on port " << PORT << "\n";
    receiver.setup(PORT);
    receiver2.setup(PORT);

you'll get:

attempting to bind to port: 12345 attempting to bind to port: 12345 terminate called after throwing an instance of 'std::runtime_error' what(): unable to bind udp socket

edit: PS I also was doing this from multiple instances of the same app and I got the same error.

kylemcdonald commented 12 years ago

i just tried the multiple-instances thing and had the same problem as you. argh.

here is the installation i did https://github.com/kylemcdonald/DohaInstallation

this was the commit that fixed it for me:

https://github.com/kylemcdonald/DohaInstallation/commit/a8d294bb99deffa59b7c3d7b03ddc3688376aeff#diff-5

and here's an example of using it (just like you expect):

https://github.com/kylemcdonald/DohaInstallation/blob/master/MultiscreenDemo/src/testApp.cpp

i'm really not sure what's wrong... :/ could it be a linux-only thing?

ofTheo commented 12 years ago

huh - yeah was wondering if it was different on linux. I wish there was a way around it as it makes testing multiple networked apps on one machine quite tricky.

bilderbuchi commented 12 years ago

I just tested your code above on linux, Theo, and got no complaints.. only output was listening for osc messages on port 12345 from the cout.

ofTheo commented 12 years ago

Crazy. I guess its an osx only issue. Thanks for testing.

I guess we can remove the 0071 milestone. Lets keep it open in case we find a fix for mac at some point.

ofTheo commented 12 years ago

whoa - found a working fix. from: http://lists.apple.com/archives/java-dev/2004/Dec/msg00489.html

"Normally this is controlled by setsockopt with SO_REUSEADDR and (on some BSD-based systems, OSX included) SO_REUSEPORT."

setting line 117 of ip/posix/UdpSocket.cpp to use SO_REUSEPORT instead of SO_REUSEADDR allows multiple binds on OS X.

it seems linux is currently working with SO_REUSEADDR, and windows implementation is in the win32/UdpSocket.cpp so we just need a #ifdef for OS X and maybe iOS in ip/posix/UdpSocket.cpp

so happy it works!! :)

bilderbuchi commented 12 years ago

Yeah it is really strange, I think there's potential for bugs here. See this SO answer (and especially the comment on it). I'm not totally convinced of anything here, UDP acts differently from TCP, and also the answers on SO differ in opinion/conclusion if you browse a bit. Can you confirm that both listeners get all your messages if you don't use a multi/broadcast message to send them? I got to admit I'm out of my depth here, this is too far into the guts of OSes. I'm not sure we should do this if we're not sure it's working, may bite us further down the line (mysteriously vanishing OSC messages, or whatever)

ofTheo commented 12 years ago

It only works for broadcast/multicast ( as the SO thread suggest ) when doing broadcast/multicast all listeners get the data though, which is great.

That SO post is really good to see though as it suggests adding the SO_REUSEPORT for apple rather than ifdef-ing it with SO_REUSEADDR

Either way I think it might be good to look at not making 'binding to a port in use' a hard exit but instead return false to the call of ofxOscReciever::setup()

That way you give people some options rather than just bringing the app down hard!

adding @damiannz because he is the guy behind ofxOsc

damian0815 commented 12 years ago

@bilderbuchi UDP doesn't guarantee delivery of messages so mysteriously vanishing OSC messages isn't actually an error condition .. perhaps we should make this more clear, that OSC packets are not guaranteed to reach their destination and whatever protocol is in use shouldn't depend on being able to receive everything. (by 'whatever protocol is in use' i mean the way the receiver handles sets of OSC messages, not OSC itself or UDP).

damian0815 commented 12 years ago

in any case bringing the app down hard is not ideal i guess :-)

damian0815 commented 12 years ago

... however as i understand it the problem only occurs when you want to make multiple ofxOscListeners within a single app listening on the same port, right? why would you want to do that? (i'm not sceptical, just curious)?

ofTheo commented 12 years ago

actually multiple apps on the same port. for example I am making a multi app project which needs to communicate over osc via multicast. its super helpful to be able to run them on the same machine ( for testing )

damian0815 commented 12 years ago

ohh right, yeah that makes a lot of sense. honestly i'm also out of my depth. UDP can be weird, even without broadcast/multicast; precise behaviour seems to depend on OS(es), drivers and router(s) between you and the target.

bilderbuchi commented 12 years ago

Yeah I know that UDP is fire-and-forget. Still, the way I understood this is, even when re-using the port/address works, if you don't use multi/broadcast, and have two listeners on a perfect connection, both will only get 50% of the messages (or one will get all, the other nothing?) - that's a bit harsh even for UDP, innit?

I think we should mention this in documentation or something, and maybe even an additional ofLogVerbose/Notice message warning about this possibility.

And yes, I agree with Theo that testing would be one application. I also imagine using this to listen in on OSC traffic between other applications (for debugging), or maybe use OSC traffic coming to a DAW or video program to control other aspects of the show (think audio-visual act/performance).

damian0815 commented 12 years ago

i think one will get all, the other nothing. yeah this is harsh! it probably needs to be EXTREMELY well documented. is there a way of determining of someone else also has this port open? i think something like this could be good:

if ( portIsAlreadyOpen(portNum) && bSharePort ) {
  ofLogWarning( "ofxOscReceiver", "another application or ofxOscListener instance is already "
    "listening on port %i, behaviour may be unreliable/unpredictable, you have been warned!", 
    portNum );
  ... open the port
}