MarcFletcher / NetworkComms.Net

NetworkComms.Net is a high performance cross-platform network library written in C#.
http://networkcomms.net
Apache License 2.0
542 stars 242 forks source link

IExplicitSerializer doesn't work with custom classes #42

Closed Mufanza closed 2 years ago

Mufanza commented 2 years ago

Hi,

Thanks for the hard work & the amazing library! Works great most of the time, though I believe I found a bug in the ExplicitSerialzier

Version: 3.0.4.1 Situation: Trying to send custom classes and deserialize them with ExplicitSerializer closes the connection abruptly. No error message is shown. The 'Deserialize' method of the IExplicitlySerialize interface doesn't seem to get called ('Serialize' seems to pass correctly). Sending primitive types/arrays works correctly.

Example: The following example implements a custom ExplicitModel class. For simplicity's sake, the Serialize and Deserialize methods were left empty (so we'd expect a default instance to pop on the server side, regardless of whatever data the client sends over)

public class ExplicitModel : IExplicitlySerialize
{
    int Id { get; set; }
    string Name { get; set; }

    public ExplicitModel() { }
    public void Serialize(Stream outputStream) { }
    public void Deserialize(Stream inputStream) { }
}

public ServerApp()
{
    var options = new SendReceiveOptions<ExplicitSerializer>();
    NetworkComms.AppendGlobalIncomingPacketHandler<ExplicitModel>("ExplicitModel", ReceiveExplicitModel, options);
    var connectionsTCP = Connection.StartListening
    (
        ConnectionType.TCP,
        new System.Net.IPEndPoint(System.Net.IPAddress.Any, 8754)
    );
    Console.ReadKey();
}

private static void ReceiveExplicitModel(PacketHeader header, Connection connection, ExplicitModel model) =>
    Console.WriteLine($"Received model explicitly");

public ClientApp()
{
    var options = new SendReceiveOptions<ExplicitSerializer>();
    var connection = TCPConnection.GetConnection(new ConnectionInfo("127.0.0.1", 8754));
    var model = new ExplicitModel() { Id = 5, Name = "Foo" }
    connection.SendObject<ExplicitModel>("ExplicitModel", model, options);
    Console.ReadKey();
}
MarcFletcher commented 2 years ago

Thanks for the compliments and great that you're using the library well.

Out of interest this seems potentially similar to https://stackoverflow.com/questions/42751099/networkcomms-custom-object-error-objecttoserialize-must-implement-iexplicitly?

If it's different are there any other logs or error messages available ?

Kind regards.

Mufanza commented 2 years ago

Thanks for the quick reply! It's different - the StackOverflow issue that you linked was regarding the ProtobufSerializer, but the issue that I've encountered is related to ExplicitSerializer

The app indeed does generate a logfile:

Base Exception Type: System.ArgumentException: Provided type NetworkLearning_Unified.ExplicitModel either does not have a parameterless constructor or does not implement IExplicitlySerialize
Parameter name: resultType
   at NetworkCommsDotNet.DPSBase.ExplicitSerializer.DeserialiseDataObjectInt(Stream inputStream, Type resultType, Dictionary`2 options)
   at NetworkCommsDotNet.DPSBase.DataSerializer.DeserialiseGeneralObject[T](MemoryStream receivedObjectStream, List`1 dataProcessors, Dictionary`2 options)
   at NetworkCommsDotNet.DPSBase.DataSerializer.DeserialiseDataObject[T](MemoryStream receivedObjectStream, List`1 dataProcessors, Dictionary`2 options)
   at NetworkCommsDotNet.Tools.PacketTypeHandlerDelegateWrapper`1.DeSerialize(MemoryStream incomingBytes, SendReceiveOptions options)
   at NetworkCommsDotNet.NetworkComms.TriggerAllPacketHandlers(PacketHeader packetHeader, Connection connection, MemoryStream dataStream, SendReceiveOptions options, Boolean ignoreUnknownPacketTypeOverride)
   at NetworkCommsDotNet.NetworkComms.TriggerAllPacketHandlers(PacketHeader packetHeader, Connection connection, MemoryStream dataStream, SendReceiveOptions options)
   at NetworkCommsDotNet.NetworkComms.CompleteIncomingItemTask(Object priorityQueueItemObj)

Stack Trace:    at NetworkCommsDotNet.DPSBase.ExplicitSerializer.DeserialiseDataObjectInt(Stream inputStream, Type resultType, Dictionary`2 options)
   at NetworkCommsDotNet.DPSBase.DataSerializer.DeserialiseGeneralObject[T](MemoryStream receivedObjectStream, List`1 dataProcessors, Dictionary`2 options)
   at NetworkCommsDotNet.DPSBase.DataSerializer.DeserialiseDataObject[T](MemoryStream receivedObjectStream, List`1 dataProcessors, Dictionary`2 options)
   at NetworkCommsDotNet.Tools.PacketTypeHandlerDelegateWrapper`1.DeSerialize(MemoryStream incomingBytes, SendReceiveOptions options)
   at NetworkCommsDotNet.NetworkComms.TriggerAllPacketHandlers(PacketHeader packetHeader, Connection connection, MemoryStream dataStream, SendReceiveOptions options, Boolean ignoreUnknownPacketTypeOverride)
   at NetworkCommsDotNet.NetworkComms.TriggerAllPacketHandlers(PacketHeader packetHeader, Connection connection, MemoryStream dataStream, SendReceiveOptions options)
   at NetworkCommsDotNet.NetworkComms.CompleteIncomingItemTask(Object priorityQueueItemObj)

BaseException claims that the custom class "either does not have a parameterless constructor or does not implement IExplicitlySerialize", but neither is true (as you can see from the example code above)

Mufanza commented 2 years ago

Ah, I've found the root of the issue. The parametelress constructor must be private

This is caused by line #118 of the ExplicitSerializer class (In the 'DeserializeDataObjectInt method), which is asking for a constructor of BindingType.NonPublic

Not really sure if this is intentional or not (though I fail to see a reason for why it would be). Feel free to close this issue if it's by design :) (though maybe a more clear error-message cold come-in handy in that case)

MarcFletcher commented 2 years ago

Many thanks for letting is know. If I'm honest the reason for requiring a private constructor only is lost to time.

I'll close this issue for now but if you'd like to raise a PR to make the error message clearer and/or to also allow a public constructor that would be appreciated.

Kind regards.

Mufanza commented 2 years ago

That's ok! Hmm I don't trust myself in fixing the actual issue (perhaps there really is a good reason for enforcing the 'private' accessibility and changing that would break something). But elaborating on the error message is the least I can do :)

If anybody needs the constructor to be public, they can always implement their own DataSerializer where this isn't a requirement. I tested such approach and it seems to work without any issues.

MarcFletcher commented 2 years ago

Hahaha, fortune favours the bold!

Many thanks for the PR!

Thanks again for your support of our library!