Eliminate use of BinaryFormatter in TCP protocol.

[CharliePoole]

It's considered unsafe and is expected to go away at some point. See https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md

[CharliePoole]

General Approach

Simplify

Using BinaryFormatter was a quick fix in the initial development of our TCP communication protocol because it allowed use of arbitrary data types in messages. This was useful because it permitted rapid development and modification of the protocol. However, we actually only use a very limited set of types and it's possible to reduce that set even further. That's what I'll do as the first step.

Add Unit Tests

The TCP communication pipeline is not well tested. In particular, we need tests of the messages themselves. This step will be carried out in parallel with the simplification step.

Self-Encoding and Decoding

Messages should be responsible for their own encoding and decoding. Each message type should have an Encode method, which converts that messages content to a byte array. A static Decode method will create the appropriate message instance from a received byte array. Sending the bytes along with a length prefix and receiving messages from a socket will remain the responsibility of the BinarySerializationProtocol class.

[CharliePoole]

Problems Encountered

A number of problems had to be resolved in implementing this change. I'm listing them here, primarily for the benefit of anyone working on the corresponding NUnit issue nunit/nunit-console#1354. The first few listed were envisioned ahead of time but the rest came to light as I worked. Naturally, I was only able to address each problem as I found it but it may turn out to be more effective to address them in some other order. :-) I'll continue update this list as I find other problems.

1. Command arguments in TCP messages may be of any arbitrary type. Resolved by serializing them as strings.
2. Return values in TCP arguments may be of any arbitrary type. Also resolved by serializing them as strings.
3. There is no existing method for serializing a TestPackage as a string. I created a class to do this using XML but that created an additional problem (4).
4. Setting values in a TestPackage may be of any arbitrary type. The initial solution is to encode them as strings but it looks as if we need a way to encode at least those types we actually use. I'm looking at JSON for this.
5. AsyncTestEngineResult is marshalled across processes. This is not necessary. Resolved by having the async result returned within the calling process, with asynchronicity handled independently within the called process.
6. Not really a problem, but a change of plan. I originally thought that the message class should handle it's own serialization but I ended up keeping everything in the BinarySerializationProtocol class because that allowed me to eliminate one extra copy operation involving potentially large data arguments, such as test results. Further improvements may be possible.

...More to come...

[CharliePoole]

🎉 This issue has been resolved in version 2.0.0-beta4 🎉

The release is available on:

• GitHub Release
• NuGet Package