Cysharp / MagicOnion

Unified Realtime/API framework for .NET platform and Unity.
MIT License
3.68k stars 417 forks source link

Incorrect deserialize on using Unity client and MemoryPack #689

Closed AwaharaFujihiro closed 8 months ago

AwaharaFujihiro commented 8 months ago

Hi, I am reporting that I have encountered the following problem when communicating using MemoryPack. There is a discrepancy between the results of serializing tuple in Unity and .Net, which can cause the PRC argument to be incorrect.

[MemoryPack.MemoryPackable]
public partial struct A
{
    public int B;
    public int C;

    public override string ToString() => $"({B}, {C})";
}

public static void Test()
{
    var value1 = (new A { B = 1, C = 2 }, -1);
    var bytes = MemoryPackSerializer.Serialize(value1);
    var value2 = MemoryPackSerializer.Deserialize<(A, int)>(bytes);
    Console.WriteLine(value1);
    Console.WriteLine(string.Join("", bytes.Select(b => $"{b:X2}")));
    Console.WriteLine(value2);
}

Unity

((1, 2), -1)
0100000002000000FFFFFFFF
((1, 2), -1)

.Net

((1, 2), -1)
FFFFFFFF010000000200000000000000
((1, 2), -1)
neuecc commented 8 months ago

This issue is about MemoryPack.

MemoryLayout compatibility can be a complicated case when not everything is Sequential. For example, .NET 6/7 compatibility had the following problems https://github.com/Cysharp/MemoryPack/issues/107#issuecomment-1368190580

As far as I can see, everything is handled as Auto in Unity and Sequential in . In this case, A is described as SequentialLayout and ValueTuple as AutoLayout, but in .NET 6 and below, the case seems to be similar to the problem in the comment above, where it does not do as specified.

The best thing to do is to not use ValueTuple (make them all Sequential), but as a workaround, making A an AutoLayout may solve the problem.

AwaharaFujihiro commented 8 months ago

Thank you.

With this sample, it is indeed a MemoryPack issue. I am sorry for the confusion I caused by explaining it with ValueTuple, The same issue is happening with MagicOnion.DynamicArgumentTuple.

DynamicArgumentTuple<A, int>(new A { B = 1, C = 2 }, -1);

in .Net, the memory placed by AutoLayout seems to place primitive types first. I have confirmed that the problem of reversed memory layout order can be avoided by setting DynamicArgumentTuple to Sequential.

AwaharaFujihiro commented 8 months ago

This code can reproduce the problem with MagicOnion.

Common

[MemoryPackable]
[StructLayout(LayoutKind.Auto)]
// This also occurs when TestStruct is set to Sequential.
public partial struct TestStruct
{
    public int A;
    public int B;
}
public interface ITestService : IService<ITestService>
{
    public UnaryResult SendTest(TestStruct ab, int c, int d);
}

Client (Unity)

var testService = MagicOnionClient.Create<ITestService>(channel);
await testService.SendTest(new TestStruct()
{
    A = 1,
    B = 2,
}, c: 3, d: 4);

Server (.Net)

public UnaryResult SendTest(TestStruct ab, int c, int d)
{
    Console.WriteLine($"A={ab.A}");
    Console.WriteLine($"B={ab.B}");
    Console.WriteLine($"C={c}");
    Console.WriteLine($"D={d}");
    return UnaryResult.CompletedResult;
}

Result

A=3
B=4
C=1
D=2
AwaharaFujihiro commented 8 months ago

Resubmit issue in MemoryPack. https://github.com/Cysharp/MemoryPack/issues/191