Transport abstraction, WebSocket support, bug fixes

[djkaty]

This branch abstracts the TCP socket layer from IrcConnection and allows IrcConnection to use pluggable transports instead.

Transports must implement the new IIrcTransportManager. Two transports are provided:

• IrcTcpTransport, providing the same functionality as in the existing implementation
• IrcWebSocketTransport, which allows IRC connections over a WebSocket

The read thread in IrcConnection is moved to IrcTcpTransport. IrcWebSocketTransport does not require a read thread as it is a wrapper around websocket-sharp which is event-driven.

IIrcTransportManager specifies an event-driven receive handler OnMessageReceived which IrcConnection subscribes to. Therefore, the read thread in IrcConnection is replaced by a read queue. The OnMessageReceived handler in IrcConnection (Transport_OnMessageReceived) adds received messages to the read queue.

All transports support AutoRetry and AutoReconnect as these are managed in IrcConnection.

Note that due to the way websocket-sharp is implemented, IrcWebSocketTransport supports AutoRetry but not a multiple server list.

The write buffers, WriteLine etc. are moved into WriteThread which is renamed WriteController to signify it also includes helper methods. This unifies all stream writes to all transports. When WriteController has written a line, it fires a new event OnWriteLine, which IrcConnection subscribes to. The event handler in IrcConnection fires its own OnWriteLine event to end-user applications.

Connect() now supports passing an IIrcTransportManager as the only parameter - pre-populated with connection details - or a transport with a server list or port, plus the original Connect() overloads for backwards compatibility. These will use IrcTcpTransport by default, or whatever the current transport is if it has been changed previously.

The transport in use can be fetched via the new property IrcConnection.Transport. This can be used to modify the connection parameters before calling Connect().

In addition, numerous connection-related bugs are fixed:

• If a new connection is made with a smaller server list, a potential IndexOutOfBoundsException bug is fixed
• AutoRetry would never attempt more than once due to an incorrect comparison
• The number of AutoRetry's is reset when starting a new connection. Before, less than the potential maximum could be attempted (if the comparison operator had been correct).
• AutoRetry recursion has been changed to a do {...} while loop
• The write thread would not reset its queues when reconnecting, meaning that existing queued data would attempt to be written to the new connection incorrectly
• The read thread moved to IrcTcpTransport now cleanly closes the stream when stopping the thread rather than using the evil Thread.Abort().

Other changes:

• ProxyType.cs has been moved to the IrcTransport folder to reflect its proper use.
• The read queue has been updated to use a generic collection Queue instead of a .NET 1.0 non-type-safe collection.
• DCC chat: IrcFeatures.Encoding no longer exists so DccChat.cs wouldn't compile. I have changed it to use the system default encoding (System.Text.Encoding.Default) so that it compiles.

Syntax and testing:

Test.cs has been updated to show all the new possible syntaxes for connecting using standard TCP or WebSockets, using an explicit or implicit transport instance. Please see this for usage details.

I have added a /reconnect command in Test.cs to test reconnection scenarios.

I have tested the library with all of the combinations in Test.cs and can confirm it works as expected. I cannot promise there are no threading bugs whatsoever, especially edge cases relating to disconnects, but I have simulated broken connections or failed connection starts to confirm it works at least superficially.

Breaking changes:

Encoding, UseSsl, ValidateServerCertificate, SslClientCertificate, SocketReceiveTimeout, SocketSendTimeout, ProxyHost, ProxyPort, ProxyType, ProxyUsername and ProxyPassword must now be set on IrcTcpTransport instead of IrcConnection. The property behaviour is otherwise the same (IrcWebSocketTransport uses websocket-sharp's semantics for setting SSL and proxy configurations).

...

Getting this + IRCv3 + Twitch support done is bubbling to the top of things I need to get done for my own project now so I hope we can sort this out quickly. Hope you like the changes!

Katy.

[meebey]

Showing 8 changed files with 1,348 additions and 635 deletions.

Wow, you have been very busy :) That will take a bit to review, I know that changing such fundamental code leads to many moved code. It is just hard to review... One thing I noticed quickly is that your coding style (curly braces mostly) does not match the existing code. I will give some pointers as comments in the changes so you know which cases I mean.

[meebey]

Also the commits should be squashed and rebased on master, github already says it has conflicts that need to be resolved.

[djkaty]

Yeah, it's really hard to modify that kind of code without making it all alot worse before it gets better, and trust me, yesterday it was so utterly broken :) I tried to match the coding style, using names like _Foo for private members etc. (which I don't like btw because if their accessibility is protected they're not CLS-compliant), the curly braces are a result of Visual Studio more or less forcing that in auto-formatting. I'm guessing you're mostly referring to the properties. I did notice :)

I don't really know what squash and rebase means, I know what you're saying tho, crunch it into one commit (the first 2 do nothing, the 2nd reverts the first), I'll try and figure out how to do that in team explorer :) It should be able to be auto-merged.

As for review, I'd recommend you look at the new source files and IrcConnection.cs directly rather than the diff because the diff makes little sense with such sweeping changes.

[meebey]

Oh for Smuxi I actually specified a Coding Style, see here: https://github.com/meebey/smuxi/blob/master/HACKING

It uses the same style, but is a good summary of how things should look like

[meebey]

We can sort out the squashing later, that is not so important, the diff is already optimizing that away anyhow because the 2nd commit undoes what the 1st commit does, leading to 0 delta :)

[djkaty]

You meant I should have "} else {" on the same line right?

Do I need to make a new PR if I modify the coding style or squash?

[meebey]

The changes in total looks very promising, nicely done!

[meebey]

If the SmartIrc4net solution has no code formattings defined yet, I should add that. For Smuxi I had already done that.

[meebey]

Do I need to make a new PR if I modify the coding style or squash?

Yeah just make changes, commit and push, you don't need to care about squashing for now. I don't review commits, I review the delta :)

[djkaty]

Okay, looked through the coding style, I was mostly on track, I'll go through and make the necessary changes. I'm going to start adding CAP REQ support (IRC 3.2) in another branch later, so let me know when you've figured out everything I've ruined so I can fix it :D Glad you liked it so far :)

[djkaty]

Ok, that should have got most of the egregious coding style differences - let me know :)

[djkaty]

Requested changes made, just waiting for an answer on test.cs. Really appreciate you taking the time to go through all this with me :)

[meebey]

I tried to match the coding style, using names like _Foo for private members etc. (which I don't like btw because if their accessibility is protected they're not CLS-compliant),

I no longer use at all in my current CS, see the Smuxi example, only fields are now f prefixed, which is CLS-complaint :)

[meebey]

Finished initial review

[djkaty]

All requested changes made vis a vis my other comments above.

[djkaty]

Just pushed a big commit. Although there may seem like some fluff in there, basically all of the changes to the code-base were driven by the test output. Some things just had to be changed to make everything work.

I forgot to mention in the commit message that I changed the default ping interval and timeout to more sensible defaults.

I also forgot to mention that I added an OnReconnected event to IrcConnection.

Please note that the use of async/await and some TPL features in the test code means it has a minimum requirement of .NET 4.5 Framework. If you need it to run on .NET 4.0, I can make some changes. The use of automatic property initializers in some code pushes the minimum compiler requirement to C# 6 (the initializers can easily be changed if that's a problem too).

If you really want me to walk you through the hows and whys and wheres of all the bug fixes I can do that, but some of it is very esoteric.

There are 14 NUnit tests for connections, disconnections and re-connections. Please see the XML documentation in IrcConnectionTests.cs for details of what each test does. The tests all pass on this commit. They do not affect the user's internet connection.

The 'dirty disconnect' tests will only run on Windows btw, because they use the Win32 API to deliberately sabotage the connection by deleting entries from the TCP connection table. The other 10 tests should run on all platforms.

[djkaty]

I have made my final commit to this branch. Please see the commit description for the details.

The final code base includes 34 tests on a wide variety of connection scenarios which I have worked through thoroughly and I believe this code can be considered stable.

Minimum requirement .NET Framework 4.5. I can cut this down to .NET 4.0 if required but I recommend leaving it so that we can implement async/await in the next round of code.

Further optimizations to socket timeouts, transactional operations, burst sending and write operations will be committed in a separate branch that I will clone from this branch.

[txdv]

You didn't merge this @meebey ?

[djkaty]

https://github.com/djkaty/SmartIrc4net/commits/master

Last year I posted .NET 4.0 support, transport abstraction, WebSockets support, IRCv3 caps support with IDN / Punycode parsing, write queue optimizations and a fairly comprehensive burst sending control, as I had to use SmartIrc4Net in a large distributed bot project. None of the changes were ever merged and I'm working on another project now so I had to clear my Pull requests log. Feel free to look at/use the commits to the repo above if you want these features.

Edit: I still maintain my fork and add things to it as required, but I gave up doing PRs.

[txdv]

Good thing open source lets you fork easily. I thought @meebey would be more thrilled about people working on smartirc4net

[knocte]

I guess meebey didn't merge because he never had time to review a monster-commit. Open source etiquette tells you it's better to do small commits.

[meebey]

@djkaty @txdv @knocte, yeah these changes are still on my todo list. I have spent hours on reviewing already but since there are so many changes it needs further reviewing and testing.