dotnet / orleans

Cloud Native application framework for .NET
https://docs.microsoft.com/dotnet/orleans
MIT License
10.14k stars 2.04k forks source link

Orleans Serializer Throws when using a record type as a type argument in a ImmutableSortedDictionary<,> #8302

Open AlgorithmsAreCool opened 1 year ago

AlgorithmsAreCool commented 1 year ago

Hello, and thank you for all the amazing work on Orleans.

I have noticed that if you specify a record type as a type argument in a ImmutableSortedDictionary<,>, you get an exception.

Orleans.Serialization.CodecNotFoundException: Could not find a codec for type System.Collections.Generic.GenericEqualityComparer`1[TestRecord]

Stacktrace

Unhandled exception. Orleans.Serialization.CodecNotFoundException: Could not find a codec for type System.Collections.Generic.GenericEqualityComparer`1[TestRecord].    
   at Orleans.Serialization.Serializers.CodecProvider.ThrowCodecNotFound(Type fieldType) in /_/src/Orleans.Serialization/Serializers/CodecProvider.cs:line 655
   at Orleans.Serialization.Serializers.CodecProvider.GetCodec(Type fieldType) in /_/src/Orleans.Serialization/Serializers/CodecProvider.cs:line 161
   at Orleans.Serialization.Serializers.AbstractTypeSerializer.WriteField[TBufferWriter](Writer`1& writer, UInt32 fieldIdDelta, Type expectedType, Object value) in /_/src/Orleans.Serialization/Serializers/AbstractTypeSerializer.cs:line 52
   at Orleans.Serialization.Serializers.AbstractTypeSerializerWrapper`1.WriteField[TBufferWriter](Writer`1& writer, UInt32 fieldIdDelta, Type expectedType, TField value) in /_/src/Orleans.Serialization/Serializers/AbstractTypeSerializer.cs:line 33
   at Orleans.Serialization.ServiceCollectionExtensions.ValueSerializerHolder`1.Serialize[TBufferWriter](Writer`1& writer, TField& value) in /_/src/Orleans.Serialization/Hosting/ServiceCollectionExtensions.cs:line 180
   at Orleans.Serialization.Codecs.GeneralizedReferenceTypeSurrogateCodec`2.WriteField[TBufferWriter](Writer`1& writer, UInt32 fieldIdDelta, Type expectedType, TField value) in /_/src/Orleans.Serialization/Codecs/GeneralizedReferenceTypeSurrogateCodec.cs:line 57
   at Orleans.Serialization.Serializer.SerializeToArray[T](T value) in /_/src/Orleans.Serialization/Serializer.cs:line 56
   at Program.<Main>$(String[] args) in D:\junk\orleans-immutablesorteddictionary-bug\Program.cs:line 22

Repro

// See https://aka.ms/new-console-template for more information
using System.Collections.Immutable;
using Orleans.Serialization.Codecs;
using Orleans.Serialization;
using Microsoft.Extensions.DependencyInjection;

var services = new ServiceCollection();
services.AddSerializer();
var provider = services.BuildServiceProvider();

var serializer = provider.GetRequiredService<Serializer>();

serializer.SerializeToArray(ImmutableDictionary<string, TestRecord>.Empty);//does not throw
serializer.SerializeToArray(ImmutableSortedDictionary<string, TestRecord>.Empty);//throws 💥

[GenerateSerializer]
public record TestRecord;
ReubenBond commented 1 year ago

Looks like we're missing a codec for System.Collections.Generic.GenericEqualityComparer, which is a type I am unfamiliar with.

ReubenBond commented 1 year ago

I think the best way to handle these internal implementations of EqualityComparer<T> is to treat them as the EqualityComparer<T>.Default Eg:

We can do similarly for Comparer<T> and its ilk.

AlgorithmsAreCool commented 1 year ago

I noticed does not affect the ImmutableDictionary, which internally uses an EqualityComparer, but it isn't respected by the surrogate. (copied code below for convenience)

The equality comparer affects the behavior of the Sorted* collections, but it also affects the behavior other keyed collections such as HashSet. It seems inconsistent that we try to preserve this behavior for some collections but not others (although pragmatically I understand why this was written this way).

https://github.com/dotnet/orleans/blob/1771da4b348749a1fc24ce1195f9009eed196712/src/Orleans.Serialization/Codecs/ImmutableSortedDictionaryCodec.cs#L44

    [GenerateSerializer]
    public struct ImmutableSortedDictionarySurrogate<TKey, TValue>
    {
        /// <summary>
        /// Gets or sets the values.
        /// </summary>
        /// <value>The values.</value>
        [Id(0)]
        public List<KeyValuePair<TKey, TValue>> Values;

        /// <summary>
        /// Gets or sets the key comparer.
        /// </summary>
        /// <value>The key comparer.</value>
        [Id(1)]
        public IComparer<TKey> KeyComparer;
        [Id(2)]
        public IEqualityComparer<TValue> ValueComparer;
    }

https://github.com/dotnet/orleans/blob/1771da4b348749a1fc24ce1195f9009eed196712/src/Orleans.Serialization/Codecs/ImmutableSortedDictionaryCodec.cs#L44

    [GenerateSerializer]
    public struct ImmutableDictionarySurrogate<TKey, TValue>
    {
        /// <summary>
        /// Gets or sets the values.
        /// </summary>
        /// <value>The values.</value>
        [Id(0)]
        public Dictionary<TKey, TValue> Values;
    }
ReubenBond commented 1 year ago

If the surrogates set the comparer to null when it's equivalent to the default for a given TKey, that might suffice to maintain behavior.

adityamandaleeka commented 1 year ago

I think the best way to handle these internal implementations of EqualityComparer<T> is to treat them as the EqualityComparer<T>.Default Eg:

  • GenericEqualityComparer<T>
  • ByteEqualityComparer
  • NullableEqualityComparer<T>
  • ObjectEqualityComparer<T>
  • EnumEqualityComparer<T>

We can do similarly for Comparer<T> and its ilk.

Triage: we should do this ^