Establish NET 3.5 compatibility #103
Conversation
|
I was under the impression that recent versions of .NET 3.5 could handle optional arguments just fine ? |
|
See @JamesWHurst the great thing about having this workflow is you can keep commiting to your personal repo/branch and the changes reflect on the pull request :) |
|
You're right - I'm liking this now. But most of all - I feel appreciation for the help of others such as yourself. |
|
According to this thread on SO: http://stackoverflow.com/questions/1210679/can-you-use-optional-parameters-in-code-targeting-net-3-5, it "is" possible to have optional parameters in c# 3.5, but it's more a tooling thing. VS versions below vs2010 will return an error and command line build tools for 3.5 will also return an error. How ever, that seems to be solvable by using the msbuild from version 4 of the framework. All in all optional parameters are supposed to be a feature of C# 1.0, for interop with other languages, though, they are officially supported by the IDE with version 4.0. So, it's probably a good idea to go with overloads. |
|
That'd explain it. Everyone at PSG uses Visual studio 2013. |
|
That's interesting! I'd love to be able to use optional, named parameters esp., for code that needs to be x-framework compatible. Upon reflection, it does seem as though these features should be capable of being implemented by the build system, as opposed to the underlying .Net Framework. Of course one primary motivation for this 3.5 work was to enable use within Mono and Unity3D. But thanks for informing me on that - good to know. |
|
I'm thinking it would be nice if that "Close & Comment" button had a hint that it's purpose is to "close the pull request"! Sometimes I wonder whether it's the fact that I started in CS so long ago, that my mind sometimes takes things so literally. My nieces seem to be able to master apps so much faster than I. |
|
That's what I was thinking at first too - but it seemed that there actually isn't anything you can mutate on an empty NetMQFrame once you have created it. I created a unit-test to get the Empty, change it, get another Empty, and verify that the more recent one is still actually empty. But no properties were apparent that I could change on it. Perhaps I should've dwelled upon that a bit longer. What are your thoughts? |
|
The Buffer property, which is the byte[] buffer can be changed just fine, and wouldn't be empty anymore. |
|
update: oh wait .. Its a buffer with length 0 .. you can't set anything to it, good point! |
|
@JamesWHurst Is this ready to be merge? |
| protected void StartClient(int id, int waitBeforeSending) | ||
| { | ||
| #if !PRE_4 | ||
| Task.Factory.StartNew(() => |
There was a problem hiding this comment.
You have code duplication here (the body of the anonymous task/thread functions). Maybe factor this code out into a function.
A general note: I see why you would want to use conditional compilation, but I would strongly suggest to minimize it as much as possible (ideally, not use it at all).
I would suggest you abstract away all code that needs conditional compilation (into a small number of classes that handle these cases) and move conditional compilation there (just minimize the scope of conditional compilation).
Ideally, though, you should just consider using .NET 3.5 constructs.
|
Yes - it is ready to be merged. Regarding the formatter: I simply have Visual Studio set it its' default. I did notice that in a few files the format was really hairy (line-indents were crazy) so I simply did format-selection command. Urbas: yes - I tried to avoid that amap (code duplication and the use of conditional-compilation). I was trying hard not to change what others have chosen for how to implement certain things - esp. where it involves their choice of how to implement concurrency. And sometimes the older (.Net 3.5) way are so much less succinct than the current implementation, that a conditional-compilation symbol was the only way to go. For the most part you'll notice that I did abstract those into a small number of classes in order to minimize the changes. |
|
As master now supports dotnet3.5 i'm closing the pull request |
Summary of my changes to NetMQ for branch Net35CompatFix:
I created a new Visual Studio 2008 solution, named NetMQ_Vs2008.sln
I went with this naming because that makes it immediately clear which solution is for Visual Studio 2008. I created this within a VMware virtual machine (VM) that does NOT have any other version of Visual Studio, and no other version of the .NET Framework. The latter is important, since that guarrantees I am not accidentally allowing a dependency on another .NET version to creep in. If it builds on this VM, it is .NET 3.5 compatible.
While on the topic of platforms, I also tested it on VMs with Windows XP SP3, Win 7, and Win 8, and Windows Server 2003 and 2008. And on both x64 and x86, to check for any unexpected fragility. It builds, and the unit-tests run successfully on all platforms - with the exception of the pgm tests which I did not run, and the NetMQScheduler which I left out as that particular class may not make sense for 3.5
.NET 3.5 does not have default parameter-values, so I separated those out in some cases into multiple methods, generally having the one call the other.
I added comments mostly only for that code that I myself edited; I don't want to add anything there unless I understand with confidence what it's doing and can compose a decently-informative comment.
Once the comments are fairly complete for this project, I'd like to suggest we use a tool such as SandCastle to automatically produce a decent help file for it and include that within the binary-downloads.
I use ReSharper (plz let me know if you need a copy) and used it for some code-analysis and some of its optimization facilities. For a few of the files, I removed unused using pragmas, plus remove unused references from the project NetMQ_Vs2008.sln
In some cases, I did not want to regress the existing code, so - as in Devices\ThreadedDeviceRunner.cs, I used conditional-compilation and provided two alternative bits of code. I tried to keep this to a minimum, as it does add code-noise to our project, which I am loath to do. In the case of ThreadedDeviceRunner, I left the code that uses Tasks, and added an alternative bit of code that uses Threads instead - as 3.5 did not have Tasks (TPL). TPL simply uses threads itself regardless, but I like its simpler syntax. In this case, I used the ThreadPool to save the execution-time of spinning up a new thread. It is important when using the ThreadPool to know when that is advantageous, and when it's better to just create a new thread.
To implement the conditional-compilation, I used the symbol PRE_4, which means only features that are provided in pre-.NET 4.0 Frameworks. I have found this scheme to be much simpler than trying to use a symbol that means a particular version is intended. If in the future we use C# 5.0/.NET 4.5 features, I recommend we use the symbol PRE_5 to provide compatibility for that platform. For every file in which I use any pragma (in this case, PRE_4 is the only one) I inserted a note at the very top of the file.
Since 3.5 also does not have CancellationTokens, I went with an alternative mechanism for cancelling background threads: Interlock, which is the most efficient mechanism available and maps to the Intel CPU hardware directly. You can see this in use within Monitoring\NetMQMonitor.cs. Here, I created a new method, RequestCancellation, and a property, IsCancellationRequested, to make the actual code clear and simple (with no conditional-compilation), and isolated the actual conditional-compilation sections to within those members. I replicated this pattern several times, identically, where needed. It makes the code even easier to read than it was before.
In file Sockets\SubscriberSocket.cs, I took the liberty of adding the method SubscribeToAll(). I think that is a bit clearer than requiring the user to Subscribe(""). Feel free to remove that of course if it doesn't suit you.
Within the folder Native, with VS2008 I found that some naming conflicts were occurring - as, for example, Poller (we have multiple types with this same name). I guess VS2008 is a bit more particular: I resolved this by appending ".Native" to the namespace of the files within this folder - which is standard C# practice anyway.
Some of the most extensive changes are within file Utils.cs, which I used as a sort of closet to place misc bits of code.
For example, several classes did not have Dispose methods in 3.5. So I added those here, using conditional-compilation as usual such that this code will simply not be included in normal builds (thus no code-bloat). I hate adding this syntactic sugar to the file though; I included comments to make it clear why these needed to exist.
I added a property here, Is64BitProcess, to provide a platform-neutral way to check this. Again, this is so that (where this is used) the messy conditional-compilation can be confined to inside of this property, instead of scattered throughout our code.
Finally, in Utils.cs I added a simplistic substitute for class DnsEndPoint, since that too is unavailable in 3.5
In IOutgoingSocket, I had to break apart that one method, which had 2 parameters with defaults, into two methods. Theoretically one would have to provide more, to cover every possible way of calling it, but I think that having method-overloads with one, and another with two, parameters of the same type, makes it too easy to accidentally specify the wrong parameter, especially since 3.5 does not provide for named-position arguments.
For NetMQFrame, I provided a very-slight optimization in that the Empty property returns a pre-constructed instance, instead of creating a new one with every call.
Another very-small optimization that I did where I saw this: for member variables that are initialized at their point of declaration to their default values, that initialization is redundant and can be removed. For example, in NetMQSocket, m_isClosed is a boolean and will be false by default: there is no need to set it to false explicitly (which results in additional IL-code).
In Poller.cs, at the beginning of method Start, I added a test for CurrentThread.Name being null before setting the Name. If you write a unit-test that uses this and you give your thread a name, that code would crash as this property does not like to be set twice, so - this protects against that.
In Tests\Devices\QueueDeviceTests.cs, and also StreamerDeviceTests.cs, you'll notice that I call StartClient with two arguments instead of one. That's because I removed the parameter-defaults from those methods. Here, I think it is less likely to cause confusion to just require the developer to put two parameters, rather than risk thinking he/she specified one intead of the other (they're both of the same type).
In Tests\zmq\ZMQPollTests.cs, I hate to bloat the code here - but decided to conditionally-compile since the two sections of code are right next to one another, and fairly small, so they can be easily compared visually. With test code, it's more important that it be explicitly transparent what code is executing, as opposed to trying to abstract things.
In places such as MonitorPollTests.cs, it is quite practical to just use the 3.5 version of the code instead of breaking it up via conditional-compilation. However, I specifically wanted to not disturb existing code AMAP; perhaps the author had a strong preference for using Tasks - but if you prefer to simplify that I don't see any harm in reducing this to just use Threads for both versions and eliminating the conditional-compilation here.
Please do note that I did do one edit to the NetMQ.Tests project (file NetMQ.Tests\NetMQ.Tests.csproj) : removed unused references.
I added the Timeout attribute to a few tests, such as PollerTests, as when these fail they can linger forever. This can be important when we set up a continuous-build/continuous-test platform, as in that case you don't want failing tests to hang up your system. Not vital. Just a bit of a convenience.
A rather important note (if you consider solid unit-testing to be as vital as I do): certain of the tests within PollerTests do have a dependency upon the test box having sufficient processing-resources to pass. I tested this on a VM with only one CPU with one core, and that newly-created thread never got to execute in time for these tests. The net result would be intermittent test failures. But not always. TaskManager showed the CPU going 100%. I added a core to that VM, and the tests stopped failing. So, these tests do not necessarily indicate failed functionality if they fail. I added notes to that effect.
Even on a not-so-heavily-loaded VM, test TwoTimers would sometimes fail. The cause was a dependency on the threads executing with too much precision (that is, start up immediately and execute simultaneously). In practice, sometimes a new thread does not fire up instantly. So I adjusted the limit very slightly here - as you'll see by comparing the two files: count2 can be 5 or 6.
I did not test this with Mono - yet. I'll wait for this to be merged in first, before continuing on to that platform.
jh