TCP support #37
TCP support #37
Conversation
…rkProtocol to UDP
…it could be > 65535
…rors from server.Serve
… attempting to continue
…e server.ListenAndServe and server.CloseConnection
…ed bundles and args of various types
When Timetag arguments are read from incoming messages, they are parsed as *Timetag, so these references to Timetag needed to be adjusted accordingly.
example output: -- OSC Bundle (2020-01-12 08:42:13.430108205 -0500 EST): -- OSC Message hypebeast#1: /bundle/message/1 ,Tsb true test string 956 blob -- OSC Message hypebeast#2: /bundle/message/3 ,Ni Nil 70030 -- OSC Bundle (2020-01-12 08:42:13.430141439 -0500 EST): -- OSC Message hypebeast#1: /bundle/message/2 ,s test string 353 -- OSC Message hypebeast#2: /bundle/message/3 ,iTb 24665 true blob -- OSC Bundle (2020-01-12 08:42:13.430142487 -0500 EST): -- OSC Message hypebeast#1: /bundle/message/2 ,FN false Nil -- OSC Bundle (2020-01-12 08:42:13.430143456 -0500 EST): -- OSC Bundle (2020-01-12 08:42:13.430144334 -0500 EST): -- OSC Message hypebeast#1: /bundle/message/1 ,si test string 370 18937 -- OSC Message hypebeast#2: /bundle/message/2 ,Ts true test string 275 -- OSC Bundle (2020-01-12 08:42:13.430158528 -0500 EST): -- OSC Message hypebeast#1: /bundle/message/1 ,TiT true 11536 true -- OSC Bundle (2020-01-12 08:42:13.430168011 -0500 EST): -- OSC Message hypebeast#1: /bundle/message/1 ,iF 53026 false -- OSC Message hypebeast#2: /bundle/message/2 ,bi blob 91772 -- OSC Message hypebeast#3: /bundle/message/3 ,N Nil -- OSC Message hypebeast#4: /bundle/message/4 ,T true
This reverts commit b645a08. It turns out that when I was testing with a JavaOSC client, there was a bug in the client causing additional 0 bytes to be added to the end. This is actually incorrect. What we had before was correct.
There was a problem hiding this comment.
Hey @daveyarwood,
Hope you don't mind me jumping in here even though it's not my repository.
Thanks so much for doing this, I didn't even know TCP OSC was a thing, to be honest, but I believe it could be the solution to a few problems I'm going to tackle in the future!
I've had a quick look and left a comment, will take a deeper look tomorrow hopefully.
osc/osc.go
Outdated
| ReadTimeout time.Duration | ||
| networkProtocol NetworkProtocol | ||
| udpConnection net.PacketConn | ||
| tcpListener net.Listener |
There was a problem hiding this comment.
It would be great to abstract udpConnection and tcpListerner out a little so that there is a field on the Server that is populated by an interface that is common to both TCP and UDP. Right off the top of my head I can't quite work it out, but I believe we would be able to do it.
I'm going to take a deeper look at this tomorrow to see what if we can work something out.
There was a problem hiding this comment.
@glynternet I played around with this a bit, but I ran into an issue.
The NewClient method takes 2 parameters, host string, port int. Go doesn't have method overloading, so I can't define NewClient(host string, port int, protocol NetworkProtocol) because there is already a method called NewClient. So I figured that to change the network protocol, you would have to create a client first and then use SetNetworkProtocol to set the protocol.
That happens to work OK with my current implementation on this branch because all the fields are there (i.e. both udpConnection and tcpListener), so it doesn't matter what order you call i.e. SetLocalAddr vs. SetNetworkProtocol.
I experimented with creating a Transport interface with implementations UDPTransport and TCPTransport. The individual Transport implementations then became a place to put the transport-specific properties like udpConnection and tcpListener. SetLocalAddr just called e.g. client.transport.SetLocalAddr. The problem with that approach, though, is that because of the setup above, now it matters which order you call the client and server setter methods. If you called SetLocalAddr(..., ...) followed by SetNetworkProtocol(TCP), the call to SetNetworkProtocol(TCP) would blow away the address and port information that you just set, because it was stored on the old network protocol.
Maybe it would be cleaner to add a second constructor, NewClientTCP, and remove SetNetworkProtocol. There could also be a NewClientUDP and NewClient could delegate to NewClientUDP for backwards compatibility. I'll try that next.
There was a problem hiding this comment.
Yikes, this is ending up being really difficult. I started down the path of doing an insane amount of refactoring. I think I'm inclined to leave this alone, if that's OK. I'm worried that if I tried to "fix" this, it would end up causing a much larger set of changes than I'd intended.
I'd be happy for anyone else to try their hand at refactoring to use an interface, if anyone is interested.
There was a problem hiding this comment.
I've pushed my WIP attempt at this to a branch, for the curious. It's not in a working state, but you can at least see what I tried and the explosion of changes it was beginning to require.
daveyarwood/go-osc@tcp-support...wip-refactor-transport-interface
|
Hey @daveyarwood, Thanks a lot for the PR. I think supporting TCP as a transport protocol is a nice feature and I'm happy to see that you are worked on it. I'll have a look at it the next days and let you what I think. |
|
Hi again @daveyarwood, I had some time on a flight last night to have a go at this. This is what I came up with: daveyarwood/go-osc@tcp-support...glynternet:gh/functional-options Using functional options (kind of Go's solution to method overloading) we can keep the implementations of the server a little more abstracted away from the network protocol. This way we can not have to add an extra ServeTCP function. It would retain backwards compatibility with most uses but would require some people to change their use if they're directly using the Receive method. Perhaps if @hypebeast is interested we could add some semantic versioning to the repo? Functional options: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis I've only done it for the server, not the client, this would be a follow up. |
|
Wow, I really like this idea! I'm new-ish to Go and hadn't heard of functional options. I'm totally sold on this approach. The one thing I would suggest is that perhaps type Option func(*Server)should be type Option func(*Server) errorI can imagine some configuration option that potentially results in an error, and we would want I guess it would be a breaking change to change the return type of |
|
Glad you're into it. I won't be able to have another go at any of this for another couple of weeks, to be honest. |
|
I can find some time to work on it. Thanks for the work you've done so far! |
…tion of the public functions; add doc comments
|
OK, I followed @glynternet's lead and did a similar thing for the client. I like where we've arrived on this. Ready for re-review. |
|
@daveyarwood any chance you could please split this PR up into separate orthogonal components?
I'm hoping that we can be mindful of @hypebeast 's time and efforts. This PR is almost 1000 lines of changes as it is and I feel it would be best if we can get it through in smaller parts. I will, unfortunately, be taking a long-distance flight this weekend <- silver lining being that I should be able to take a look at some/all of this now that we have the TCP mechanism looking how we'd like it. |
|
@glynternet I'm happy to do that, but as a series of PRs that build upon previous ones. Would that be OK? I certainly think it would make reviewing my changes easier, because the diff of each PR would be small and easy to read and discuss. My concern is that if I tried to make a handful of completely separate PRs, it might lead us into merge conflict hell. |
|
OK - I took the time to break this PR apart into several smaller PRs. Review and merge in this order: #39 Test improvements; add server.CloseConnection method As each PR is merged, the diffs of the ones that follow will get smaller, because each one builds on the last (technically, I was able to branch #41 right off of master, so if you want, you can review and merge that one earlier) Closing this PR now, as it's a near duplicate of #42. |
This implements TCP support in the way I described in #34.
Brief summary:
client.SetNetworkProtocol(osc.TCP)orserver.SetNetworkProtocol(osc.TCP).server.CloseConnectionfunction to facilitate testing and give users the ability to forcibly stop a running server.ListenAndServeincludesdefer server.CloseConnection(), ensuring that the connection it creates is closed in the event of an interruption or error.I also made some adjustments and improvements to the tests and examples. I did some thorough testing involving this branch of go-osc and a similar branch of JavaOSC where I've just finished implementing TCP support. I tested all the permutations of TCP vs. UDP, Go client vs. Java client, Go server vs. Java server, and everything is looking great, as far as I can tell.
Feedback welcome, of course. I realize this is a big change set! Let me know if there's anything I can do to make reviewing it easier.
Thanks for all the excellent work on go-osc!