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

Added new OSC block, removed existing. Updated TUIO block. #1179

Merged
merged 14 commits into from Jan 18, 2016

Conversation

Projects
None yet
7 participants
@ryanbartley
Contributor

ryanbartley commented Nov 4, 2015

No description provided.

@pizthewiz

This comment has been minimized.

Contributor

pizthewiz commented Nov 5, 2015

One other thing that might need some attention is the TUIO block and samples which depended on the old OSC block right?

@ryanbartley

This comment has been minimized.

Contributor

ryanbartley commented Nov 5, 2015

Working on the patch. Should have it up soon.

@pizthewiz

This comment has been minimized.

Contributor

pizthewiz commented Nov 5, 2015

Strong work, especially when I'd have to imagine the TUIO stuff is both pretty gnarly and probably not heavily used.

@ryanbartley

This comment has been minimized.

Contributor

ryanbartley commented Nov 5, 2015

:) gnarly is a perfect word for it!

@richardeakin

This comment has been minimized.

Collaborator

richardeakin commented Nov 8, 2015

Lotsa samples, nice. However I don't see the src/Osc.h and src/Osc.cpp files.

Also I see there are app icons for the samples, can you remove those and point to the ones in samples/data folder? We're planning to do a purge one day and each icon we can avoid reduces the binary package by about half a meg.

@richardeakin

This comment has been minimized.

Collaborator

richardeakin commented Nov 9, 2015

Ah, I see - Osc.h / cpp are at the very end but their diffs don't show up ('maximum number of lines exceeded.')... not a problem though to view them, though.

@richardeakin

This comment has been minimized.

Collaborator

richardeakin commented Nov 14, 2015

Hey Ryan, this looks great, you've accomplished a huge upgrade. While I still haven't run the code but I assume you and @pizthewiz have so impl is correct, a couple nits I wanted to bring up:

  • Should the classes be in the cinder namespace (more specifically, ci::osc like they were before)? One benefit I see of this is that it will be distinguished from code that uses raw oscpack, which is / was in the top-level osc namespace.
  • I think it is a good style to place the body of if() statements on a new line (sorry I can't comment in-line where I saw this because github isn't allowing me to, but for example here). Other than improving legibility (IMO), the main benefit of this is that when you're stepping through code it is easy to tell when you made it into the if block, or if skipped over it. Given that many debuggers like to stutter on things like shared_ptrs or for other odd reasons, landing directly in the body makes it clear what's going on.
  • Can you mark the end of the namespace with a comment (like here). Also there's a stray semicolon here - ok now I admit I'm really nitpicking. :)

Thanks for this, we've needed to move away from oscpack for a long time!

@ryanbartley

This comment has been minimized.

Contributor

ryanbartley commented Dec 5, 2015

Finally updated the Tuio Block, going to add an RFC for this and then I'll fix those changes you requested, Rich. There are also a few minor changes that I'm going to make to Osc after working on this block.

@pizthewiz

This comment has been minimized.

Contributor

pizthewiz commented Dec 20, 2015

I don't have any history with TUIO but I'd be happy to test if that would help push things forward.

@ryanbartley

This comment has been minimized.

Contributor

ryanbartley commented Jan 12, 2016

This is ready to merge. I also tested on Ubuntu and it works great. I'll do some more testing on a couple of embedded devices as soon as it's merged. Thanks for the help guys! Let me know if there are any other outstanding things.

@richardeakin

This comment has been minimized.

Collaborator

richardeakin commented Jan 12, 2016

Thanks for making those changes to the OSC stuff. On TUIO, is there any reason that the statc Object createFromSetMessage() isn't an overload constructor? In general static create methods return a shared or unique pointer.

@ryanbartley

This comment has been minimized.

Contributor

ryanbartley commented Jan 12, 2016

I believe that is from the old Tuio block.

@richardeakin

This comment has been minimized.

Collaborator

richardeakin commented Jan 12, 2016

Ah, I see alot of that is - sorry, hard to tell what is new as I'm not able to use github diffs on this PR (github complains it is too long).

@ryanbartley

This comment has been minimized.

Contributor

ryanbartley commented Jan 12, 2016

Sorry about that. This is definitely a big and unfortunately tethered PR that deals with multiple issues.

@ryanbartley

This comment has been minimized.

Contributor

ryanbartley commented Jan 12, 2016

you can see the actual tuio specific code if you look at my branch. The length is mostly due to how many files were deleted.

@richardeakin richardeakin changed the title from Added new osc block, removed existing to Added new OSC block, removed existing. Updated TUIO block. Jan 12, 2016

andrewfb added a commit that referenced this pull request Jan 18, 2016

Merge pull request #1179 from ryanbartley/OscBlockUpdate
Added new OSC block, removed existing. Updated TUIO block.

@andrewfb andrewfb merged commit 97ffe82 into cinder:master Jan 18, 2016

@paulhoux

This comment has been minimized.

Collaborator

paulhoux commented Jan 19, 2016

Gratz @ryanbartley and thanks for your commitment and hard work.

@ryanbartley ryanbartley deleted the ryanbartley:OscBlockUpdate branch Jan 19, 2016

@ryanbartley

This comment has been minimized.

Contributor

ryanbartley commented Jan 19, 2016

👍

@kitschpatrol

This comment has been minimized.

Contributor

kitschpatrol commented Jan 20, 2016

This is great!

I've been testing it out, and have run into some trouble with failed reconnections after restarting an app running a TCPReceiver. I ended up seeing improvements by setting the reuse_address option to true on the TCPReceiver acceptor like so: kitschpatrol@d739faa

Wondering if anyone else has had similar issues, and if the above fix has any downsides.

@ryanbartley

This comment has been minimized.

Contributor

ryanbartley commented Jan 20, 2016

Yeah, that's a common problem with Tcp Connections, specifically. Basically, your app is finished and closed out and not receiving anything else, but the OS still has the socket open. The socket::close function is more akin to a "wish list" that you'd send to santa. :) If you do a rapid quit and restart, more than likely the OS is still holding on to the socket, waiting for any last second messages because of the OS level timeout for TCP Connections.

Setting the reuse_address is a good way to address the problem. However, it's OS dependent whether the request is actually taken into account, like the close function, Windows is particularly murky in this scenario. It may not be necessary to add the set_option inside of the bind function, as you can just create your acceptor, set the options and then hand it to us here.

@sansumbrella

This comment has been minimized.

Contributor

sansumbrella commented Jan 20, 2016

I was chatting with @pizthewiz about this in a different context and I generally think setting reuse_address is a good idea.

I have had times in the past (untested with the new OSC block) where I wanted to have multiple applications using the same TCP socket on my computer. Usually when testing communication between many instances of the same application. Socket creation would fail if I didn’t allow multiple connections on the socket.

I am not as familiar with all the networking stuff as @ryanbartley, so I'm not sure whether that kind of configuration would work on all platforms. What is the downside to (requesting) allowing address reuse that I'm missing? It seems like a sane default to me.

@ryanbartley

This comment has been minimized.

Contributor

ryanbartley commented Jan 21, 2016

Short answer is that there isn't much of a downside to this change, but that it won't work all of the time, which makes me hesitant to add this to the implementation. Sometimes even when using the SO_REUSEADDR command, the socket may be doing something that won't allow it to be reused, such as a buffer filled waiting to be read or sent. This article explains a lot of the reasons that it happens.

TLDR (which apologies that it may still be TLDR), "for TCP sockets, there can be a relatively long delay between adding data to the send buffer and having the TCP implementation really send that data. As a result, when you close a TCP socket, there may still be pending data in the send buffer, which has not been sent yet but your code considers it as sent, since the send() call succeeded." This causes what is called a TIME_WAIT phase for the socket which is configurable, but most systems have it as long as 2 mins. Down below that in the connect section, he speaks about the EADDRINUSE error received from calling the connect function on a socket. Basically, there are 5 values that differentiate a socket, {, , , , }. If you were to shut down your application and restart it in such a time that the original socket was in a TIME_WAIT, you'd most likely try to rebind your address to the same 5 numbers. "If data arrived for either one of the two connections [socket in TIME_WAIT, or socket just opened], the system could not tell which connection the data belongs to." Hence, the error. Even if you used SO_REUSEADDR, you'd still get an error because you've chosen the exact set of numbers that you just can't choose.

There's another article that echoes this but from a different perspective. "[SO_REUSEADDR] option tells the kernel that even if this port is busy (in the TIME_WAIT state), go ahead and reuse it anyway. If it is busy, but with another state, you will still get an address already in use error. It is useful if your server has been shut down, and then restarted right away while sockets are still active on its port." So, also if the socket is within another state other than TIME_WAIT, you'll get an address already in use error.

I thought about this addition a lot while writing this because it seemed to make the problem better, but realized that it just seemed I had more luck with the specific state of the system more than I fixed the problem. Basically, the socket is doing a ton of work to make sure the connection state is reliable when you actually get connected. When you disconnect, it has to continue that reliability after your gone, either resending lost packets or waiting to receive expected packets or just waiting to hear anything at all from the other side.The real problem is that TCP is complicated and properly shutting down a socket can be complicated. The gist is, call shutdown ( which, now I realize, should probably be a part of the TCP interface ), then read from the socket, to fully empty the buffers or receive any other "errors" that the socket has, and finally close the socket. Even this could create the side effect as the OS won't necessarily tell you when it's TIME_WAIT is up. Sorry about how long this post became :( Hope this all makes sense and I'll look at the addition of a shutdown method soon.

Edit: Actually, I remember choosing not to include shutdown (it's been a while :) as for the means of the "OSC" block, it complicates understanding of the block, which may or may not have been a good idea. This is the reason I made the constructors that take constructed Acceptors/Sockets so that not only could you reuse them in different combinations (a connected socket could become a receiver or a sender, etc.), but also you could control the lifetime of the socket more specifically, i.e. to be able to set options, shut it down, read and close it, etc, outside of the constraints of the OSC block. But I'm definitely willing to revisit the addition of shutdown if people don't think it'll muck with the interface too much.

@kitschpatrol

This comment has been minimized.

Contributor

kitschpatrol commented Jan 21, 2016

Wow thank you for these very helpful responses and links. Clearly this is a bit of an iceberg. Good to know that REUSEADDR improves the odds but doesn't actually deliver any guarantees. Sounds like shutdown is the way to go. Thanks again.

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