Add TCP protocol support to Socket #59

pekiZG · 2016-04-13T23:40:15Z

Added TCP protocol parameter switch.
Name refactoring.

Added TCP protocol parameter switch. Name refactoring.

DarrellMozingo · 2016-04-16T23:35:35Z

Thanks for the PR Petar! A few concerns:

The project .NET version being upgraded is a breaking change we weren't ready to make yet. What forced you to do it?
No smoke/acceptance tests for the TCP functionality itself
I don't see the TCP connection being opened anywhere? According to the docs for SendTo(), it should be throwing an exception:

If you are using a connection-oriented protocol, you must first establish a remote host connection by calling the Connect method or accept an incoming connection request using the Accept method. If you do not establish or accept a remote host connection, SendTo will throw a SocketException.

The management of a TCP connection is why I was hesitant to add this functionality in the first place. I can't see a full handshake being done before each send request since it's all blocking and would kill the calling app. It's all fully possible of course, just more than I wanted to add for something I still don't see a huge amount of benefit from :)

pekiZG · 2016-04-18T16:49:51Z

Hi Darrell,
This is my first PR :)

I would like to address all your concerns.

Break was not intentional. Visual Studio 2015 on Windows 10 wouldn't open it.
Later I found how to enable .net 2.0+ and will migrate it without breaking.
Still looking why tests won't run. It's probably connected to bullet number 1.
I tried it and it didn't throw an exception. SendTo() should be used on line 107 in StatsdUDP.cs
I'm not happy how I did it.

This reverts commit 88abbb0.

pekiZG · 2016-04-18T20:20:51Z

@DarrellMozingo

I have successfully run all the tests after poking around and finding out that NUnit 2.6 is being used.
Will have TCP connection test soon.

KR
Petar

Changed to implementation towards the Interface. Added StatsdTCP concrete class. Reverted StatsdUDP to original state.

pekiZG · 2016-04-19T21:50:34Z

After finding out that there is App.confing in Tests I have created Hyper-V and configured InfluxDB with Chronograph and Telegraph to have valid Smoke tests. (See picture below)

TCP Smoke test is passing.

I have changed to the implementation towards the interface over concrete type.
You can see it in the last commit (src/StatsdClient/Metrics.cs)

Also, I'm not done yet.

Thinking about doing it with async methods. @DarrellMozingo that should address your concern on blocking the calling app. Right?

DarrellMozingo · 2016-04-21T17:28:53Z

Thanks for fixing the .NET version and splitting out the TCP/UDP classes - nice touch.

I see you're doing the connect/disconnect now. I think that should be in a try/finally so ensure the connection is closed if something goes wrong in the send (I'll add an in-line comment). Also a base class (or a separate class) to handle the IP address resolution as it's a bit convoluted?

I've personally always been against doing the TCP for socket management and the speed hit of doing a sync call. If it's an absolute requirement, I feel it's better to split that responsibility out to another app (ie have a statsd relay running on each server that you'd send UDP stats to, and it would relay them in TCP). Do you still think the feature is needed if someone does this?

Thanks again for taking the time to contribute, I do appreciate it! :)

DarrellMozingo · 2016-04-21T17:29:28Z

src/StatsdClient/StatsdTCP.cs

+        {
+            _clientSocket.Connect(IPEndpoint.Address, _port);
+            _clientSocket.SendTo(encodedCommand, encodedCommand.Length, SocketFlags.None, IPEndpoint);
+            _clientSocket.Disconnect(false);


Put this in a try {} finally {} so we're not leaving hanging TCP connections?

pekiZG · 2016-04-21T17:58:44Z

@DarrellMozingo no problem I really want to contribute.
Also, I will do a little How to setup development environment. Recently had to do a clean install and it took me time to setup.

My position is that you should have a choice between UDP and TCP.
If someone has control over the network and can place another app. that sounds like a better option.

Today I will work on this part from documentation.
https://msdn.microsoft.com/en-us/library/system.net.sockets.socket(v=vs.80).aspx

To process communications using separate threads during execution, use the following methods, which are designed for asynchronous operation mode

Using BeginSend and EndSend would be much better.
I will try to make some kind of load test. Have no idea how (actually I have).
Have to think about what I want to measure.

On the topic of IP address resolution, that should definitely be pulled out (separation of concern).

I really appreciate you taking time to look at my messy code :)
Pair programming 👍

Pulled out shared interface into separate file. Pulled out address resolution.

DarrellMozingo · 2016-04-22T00:35:35Z

src/StatsdClient/StatsdTCP.cs

+            {
+                _clientSocket.Connect(IPEndpoint);
+                _clientSocket.SendTo(encodedCommand, encodedCommand.Length, SocketFlags.None, IPEndpoint);
+                _clientSocket.Disconnect(false);


The Disconnect() should be in the finally block, no?

Yes,

still working on this part to make it Async. :)

DarrellMozingo · 2016-04-22T00:38:14Z

I'm still hesitant to add it, but I suppose the TCP code will be insulated in a separate class and not defaulted or anything, so we can allow it with perhaps appropriate warnings (ie to use UDP or a TCP relay app first).

Thanks for the fixes so far! I'll be away for a few days, so if you get anything else done I'll take a look next week.

pekiZG · 2016-04-25T06:09:04Z

I haven't had time over the weekend.
I will have it finished tonight \o/

It's being used only inside Address class

pekiZG · 2016-04-25T21:27:46Z

src/StatsdClient/StatsdTCP.cs

            }
            finally
            {
-                _clientSocket.Disconnect(false);


Removed Disconnect() in favor of Shutdown() and Close()

pekiZG · 2016-04-25T21:33:19Z

@DarrellMozingo I think I'm done. \o/
Need second opinion.

DarrellMozingo · 2016-04-27T22:09:10Z

Nice work!

I know IDisposable was already on the UDP version, but seeing it do nothing in the TCP one and not really being called anywhere in the UDP version makes me wonder if it's even needed?

Also can you rename the TCP smoke test to match the class/other UDP one?

pekiZG · 2016-04-27T22:36:01Z

Hi @DarrellMozingo

I have implemented IDisposable interface, and renamed filenames to reflect classnames.
I didn't catch what you mean to rename smoketests?

Is there some convention I'm not being aware of?

also, I'm logging to console?
Do you think that's a smart thing to do? Or just to "swallow" the error?

DarrellMozingo · 2016-04-28T11:21:43Z

Yea, saw you implmented IDisposable. I was wondering if it could be removed all together? I don't see Dispose() being called on the UDP version. Maybe it's best to just close the UDP socket inside the Send() method as well...

Ignore my smoke test naming comment. I misread something and thought the UDP test was named differently. It's fine as is.

Swallowing seems wrong. I think something like TraceSource (built into .NET) or LibLog would be best, but we're not doing either now and there hasn't been a need yet. The console log seems good enough for now anyway :)

DarrellMozingo · 2016-04-28T11:27:04Z

The build for this branch is broken. Not sure why the PR didn't update (I just enabled it, maybe I did it wrong!).

Looks like the project file still has StatsdTCP.cs instead of StatsdTCPClient.cs (same for UDP as well)?

pekiZG · 2016-04-28T11:29:05Z

well, thinking is that if you close the outside connection (finally block), you shouldn't worry about dispose.

Or if you are doing something like this:
https://msdn.microsoft.com/en-us/library/yh598w02(v=vs.140).aspx

using(mySocket)
{
// do work
}

Dispose will be properly called.

Yes, I changed the names; Didn't know what you were referencing on SmokeTest

I can change it back?

pekiZG · 2016-04-28T11:32:02Z

Yes, you are right... I will fix it now :)

DarrellMozingo · 2016-04-28T11:35:41Z

Regarding disposable, you're absolutely correct. No body is doing a using() with it though, and you're disposing the socket right away in the TCP class. Perhaps we should do the same with the UDP one then remove the IDisposable interface entirely? It would not be doing anything then.

I changed the filenames, but forgot to push changed .csproj

pekiZG · 2016-04-28T11:36:28Z

Here is what happened.
Didn't push the changed StatsdClient.csproj

Please check if it's passing now (It should).

https://ci.appveyor.com/project/DarrellMozingo/statsd-csharp-client

EDIT:
I think smoke test will fail

pekiZG · 2016-04-28T11:43:49Z

Yes, TCP smoke test will fail.
UDP will not because it's not waiting on connection.

I should change the smoke test to use TcpListener

pekiZG · 2016-04-28T11:52:22Z

@DarrellMozingo

https://ci.appveyor.com/project/DarrellMozingo/statsd-csharp-client/build/tests

What should we do with TCP SmokeTest?

DarrellMozingo · 2016-04-28T11:56:23Z

I like the idea of the smoke test spinning up a simple TcpListener to receive the connection. As a bonus it can read the data and make sure it's correct :)

pekiZG · 2016-04-28T11:58:19Z

I was under Impression that Smoke test should be done on 'real' thing :)

That's why I did all this

EDIT:
Will fix it now so the test passes :)

DarrellMozingo · 2016-04-28T12:00:22Z

You're correct, it should be hitting the real system. From the point of view of the client though I can see just making sure it's sending a properly formed TCP packet, rather than a full end-to-end test that makes sure it shows up in the back-end correctly. So just spinning up a TCP socket to send to should work well enough for our purposes.

It should now pass the CI build on AppVeyor

pekiZG · 2016-04-28T12:14:00Z

@DarrellMozingo 1.2.40 built successfully

https://ci.appveyor.com/project/DarrellMozingo/statsd-csharp-client/build/1.2.40

We should probably update the readme.md 👍

DarrellMozingo · 2016-04-28T12:19:35Z

src/Tests/TCPSmokeTests.cs

+			System.Net.IPAddress localAddr = System.Net.IPAddress.Parse(_serverName);
+
+			// TcpListener server = new TcpListener(port);
+			server = new TcpListener(localAddr, port);


Can you wrap this in a using{} to make sure it's always stopped?

TcpListener class doesn't inherit/implement IDisposable Interface.
For that reason It's can't be wrapped in using()

If you try you get Type used in a using statement must be implicitly convertible to 'System.IDisposable'

Quite right :) I was thinking of TcpClient, sorry. Maybe just put the Stop() in a finally block, or probably a test teardown fixture would be better. Just want to make sure if the test fails that listening socket gets closed.

I went with setup/teardown fixture :)

DarrellMozingo · 2016-04-28T12:20:19Z

Looks good, thanks! Want to take a stab at updating the README?

pekiZG · 2016-04-28T12:21:48Z

Sure,

I would love to :)

pekiZG · 2016-04-28T13:57:52Z

@DarrellMozingo readme is updated :)

The changelog is left up to you 👍

DarrellMozingo · 2016-04-28T14:22:30Z

Great, thanks a lot for the contribution Petar! I'll get it pushed out today or tomorrow.

pekiZG · 2016-04-28T14:25:06Z

No hurry :)

I'm here if additional tweaks are needed.

Add TCP protocol support to Socket

3e1651b

Added TCP protocol parameter switch. Name refactoring.

pekiZG mentioned this pull request Apr 13, 2016

Add TCP support #51

Closed

pekiZG and others added 2 commits April 14, 2016 08:43

Remove C# 6.0 Auto-Property Initializor

dd3a06d

Change SocketType to Stream for Tcp protocol

e298aae

pekiZG added 3 commits April 18, 2016 21:15

Revert "Update CHANGELOG.md"

00b3bb9

This reverts commit 88abbb0.

Revert Reverted "Update CHANGELOG.md"

99015a6

Returning Tools and Framework version to original state

d0790d5

pekiZG added 2 commits April 19, 2016 01:40

Added TCPSmokeTests

ab31047

Refactoring and reorganizing

335aa6a

Changed to implementation towards the Interface. Added StatsdTCP concrete class. Reverted StatsdUDP to original state.

DarrellMozingo reviewed Apr 21, 2016
View reviewed changes

Unclutered Statsd TCP and UDP classes

fff49d6

Pulled out shared interface into separate file. Pulled out address resolution.

DarrellMozingo reviewed Apr 22, 2016
View reviewed changes

DarrellMozingo added enhancement work in progress and removed enhancement labels Apr 22, 2016

pekiZG added 4 commits April 25, 2016 22:45

Catching possible exceptions

75f92c0

Removed unnecessary usings

0bbf171

Turned method into private

0321df9

It's being used only inside Address class

Switched logging and removed disconnect

7d05fd8

pekiZG reviewed Apr 25, 2016
View reviewed changes

pekiZG added 2 commits April 28, 2016 00:30

Implemented IDisposable interface

4969402

Renamed filenames to reflect class names

308fdd2

Fix .csproj

5e58ea3

I changed the filenames, but forgot to push changed .csproj

Fixing TCP smoke test

18446da

It should now pass the CI build on AppVeyor

DarrellMozingo reviewed Apr 28, 2016
View reviewed changes

Updated readme and smoke test with setup/teardown

9e71bfe

DarrellMozingo merged commit f87a98c into Pereingo:master Apr 28, 2016

DarrellMozingo removed the work in progress label Apr 28, 2016

Add TCP protocol support to Socket #59

Add TCP protocol support to Socket #59

Conversation

pekiZG commented Apr 13, 2016

DarrellMozingo commented Apr 16, 2016

pekiZG commented Apr 18, 2016 • edited

pekiZG commented Apr 18, 2016

pekiZG commented Apr 19, 2016

DarrellMozingo commented Apr 21, 2016

DarrellMozingo Apr 21, 2016

Choose a reason for hiding this comment

pekiZG commented Apr 21, 2016 • edited

DarrellMozingo Apr 22, 2016

Choose a reason for hiding this comment

pekiZG Apr 22, 2016 • edited

Choose a reason for hiding this comment

DarrellMozingo commented Apr 22, 2016

pekiZG commented Apr 25, 2016

pekiZG Apr 25, 2016

Choose a reason for hiding this comment

pekiZG commented Apr 25, 2016

DarrellMozingo commented Apr 27, 2016

pekiZG commented Apr 27, 2016

DarrellMozingo commented Apr 28, 2016

DarrellMozingo commented Apr 28, 2016

pekiZG commented Apr 28, 2016 • edited

pekiZG commented Apr 28, 2016 • edited

DarrellMozingo commented Apr 28, 2016

pekiZG commented Apr 28, 2016 • edited

pekiZG commented Apr 28, 2016

pekiZG commented Apr 28, 2016

DarrellMozingo commented Apr 28, 2016

pekiZG commented Apr 28, 2016 • edited

DarrellMozingo commented Apr 28, 2016

pekiZG commented Apr 28, 2016 • edited

DarrellMozingo Apr 28, 2016

Choose a reason for hiding this comment

pekiZG Apr 28, 2016

Choose a reason for hiding this comment

DarrellMozingo Apr 28, 2016

Choose a reason for hiding this comment

pekiZG Apr 28, 2016

Choose a reason for hiding this comment

DarrellMozingo commented Apr 28, 2016

pekiZG commented Apr 28, 2016

pekiZG commented Apr 28, 2016

DarrellMozingo commented Apr 28, 2016

pekiZG commented Apr 28, 2016

pekiZG commented Apr 18, 2016 •

edited

pekiZG commented Apr 21, 2016 •

edited

pekiZG Apr 22, 2016 •

edited

pekiZG commented Apr 28, 2016 •

edited

pekiZG commented Apr 28, 2016 •

edited

pekiZG commented Apr 28, 2016 •

edited

pekiZG commented Apr 28, 2016 •

edited

pekiZG commented Apr 28, 2016 •

edited