Non-static SerializationManager #2592

ReubenBond · 2017-01-13T03:58:53Z

Disclaimer: this is a work in progress. It needs work, more functional testing, and to be split into smaller, more mergable PRs. SerialziationManager is a core part of our system and GrainReference is serializable, so this PR is necessarily going to be large

Implements changes from #2591 & #467 (with changes).

When using an external serializer (eg, JSON.NET, ProtoBuf), GrainReference instances must be bound via a call to IGrainFactory.Bind(IAddressable) before they can be used. When using the Orleans serialization framework, this happens automatically.

Once this is ready, we will be close to completing #467. Only some minor additional work is required to complete the task, basically changing the GrainClient class itself.

Note: this is a breaking change.

EDIT: to be clear, I don't expect this code to be reviewed in detail while I've got the work-in-progress tag on it. When it's ready, I'll remove the tag and update this PR.

xiazen · 2017-01-13T18:52:14Z

great work @ReubenBond !

jason-bragg · 2017-01-13T21:11:40Z

src/ClientGenerator/ClientGenerator.cs

+            services.AddSingleton<RoslynCodeGenerator>();
+            services.AddSingleton<SerializationManager>();
+            var serviceProvider = services.BuildServiceProvider();
+            var codeGenerator = serviceProvider.GetRequiredService<RoslynCodeGenerator>();


This seems like a quite peculiar way of constructing a RoslynCodeGenerator. Would you comment on your reasoning?

Oh yeah, it is not what DI is for ;-) totally agreeing with @jason-bragg!

What's peculiar about it?

Seems peculiar to construct a container to construct a class rather than just constructing the class directly.

There's some ugliness around this, by the way, since we don't link OrleansRuntime against the code generator and we have this CodeGeneratorManager type which tries to perform the linking at runtime. CodeGeneratorManager uses RoslynCodeGenerator, but in this revision of the PR, it will use a different instance. I've fixed that locally, so it will use the container instance if it's available, but it's still ugly.

I'll see if I can remove AssemblyProcessor and CodeGeneratorManager from this project

jason-bragg · 2017-01-13T21:26:55Z

src/Orleans/AssemblyLoader/AssemblyLoader.cs

@@ -79,23 +80,22 @@ internal class AssemblyLoader
            return discoveredAssemblyLocations;
        }
 #endif
-        public static T TryLoadAndCreateInstance<T>(string assemblyName, Logger logger) where T : class
+        public static T TryLoadAndCreateInstance<T>(string assemblyName, IServiceProvider serviceProvider, Logger logger) where T : class


Suggestion - feel free to ignore, as this can be done later.
Suggest introducing a generic type factory interface and passing it in rather than the entire IServiceProvider. Something like:

interface ITypeFactory
{
T Create(Type concreteType) where T : class;
}

An impl of this that calls the activator utilities with the service provider can be passed in making this call path more specific and not depend on DI.

Again, this can be done as a minor refactor later..

I think we should leave this for later, as we discussed. I've removed all new abstractions from this PR so that it's more focused on the main goal.

jason-bragg · 2017-01-13T21:28:50Z

src/Orleans/CodeGeneration/CodeGeneratorManager.cs

+
+        public CodeGeneratorManager(IServiceProvider serviceProvider)
+        {
+            this.serviceProvider = serviceProvider;


ITypeFactory again. In this case, it could also be registered in the service provider to allow it to be injected into the code generator manager

jason-bragg · 2017-01-13T21:33:04Z

src/Orleans/Configuration/ClientConfiguration.cs

+        /// <summary>
+        /// Gets or sets the service configuration delegate.
+        /// </summary>
+        public Func<IServiceCollection, IServiceProvider> ServiceConfigurator { get; set; } = services => services.BuildServiceProvider();


In a general sense this is functionality not configuration. Does this belong here?

I should remove it from this PR and we can talk about introducing it later. It's a good hook to have, and I consider it configuration since it's used only as part of the configuration phase and is the same signature as our Startup.ConfigureServices method (& same as ASP.NET), but it's not serializable.

jason-bragg · 2017-01-13T22:23:19Z

src/Orleans/Core/GrainClient.cs

@@ -327,7 +331,7 @@ public static Logger Logger
            get
            {
                CheckInitialized();
-                return RuntimeClient.Current.AppLogger;
+                return ((IRuntimeClient)outsideRuntimeClient).AppLogger;


Why is cast necessary here?

OutsideRuntimeClient explicitly implements IRuntimeClient for AppLogger

jason-bragg · 2017-01-13T22:32:14Z

src/Orleans/Runtime/GrainReference.cs

        }

        #endregion

+        public void Bind(IGrainFactory factory)


I don't see where this is used? Concerned that a grain reference read from storage may not be bound prior to being used in a grain call.

I should remove this method and add it as an extension method in a separate PR. There's a #warning you can search for to see where the runtime client is set during deserialization. Currently, references to foreign clusters will always be deserialized as though they are references to the cluster whose SerializationManager is deserializing it.

jason-bragg · 2017-01-13T22:32:46Z

src/Orleans/Runtime/GrainReference.cs

            }
            else if (systemTargetIndex >= 0)
            {
                grainIdStr = trimmed.Substring(grainIdIndex, systemTargetIndex - grainIdIndex).Trim();
                string systemTargetStr = trimmed.Substring(systemTargetIndex + (SYSTEM_TARGET_STR + "=").Length);
                SiloAddress siloAddress = SiloAddress.FromParsableString(systemTargetStr);
-                return FromGrainId(GrainId.FromParsableString(grainIdStr), null, siloAddress);


if these have null runtime, who ensures they get bound?

There's a #warning on this method in my local branch - it needs to be fixed. Trying to use a grain ref which is not bound will throw a well-typed exception

I still need to work out the best approach. The simplest is to add a GetGrainFromKeyString(string) method to IGrainFactory or a new service and to move this method there.

This is resolved - no nulls.

jason-bragg · 2017-01-13T22:34:29Z

src/Orleans/Runtime/OutsideRuntimeClient.cs

+            this.InternalGrainFactory = this.ServiceProvider.GetRequiredService<IInternalGrainFactory>();
+            this.ClientStatistics = this.ServiceProvider.GetRequiredService<ClientStatisticsManager>();
+            this.SerializationManager = this.ServiceProvider.GetRequiredService<SerializationManager>();
+            this.messageFactory = this.ServiceProvider.GetService<MessageFactory>();


As before, peculiar construction pattern.

I don't think so, consider that:

We prefer to have just one instance of this class (really any stateless class)

This class relies on some services in the container

Some services in the container rely on this class

As with earlier comment, I'm not suggesting this is wrong or should be changed, just trying to understand the reasoning for this pattern.
In this case I can at least see that the service provider is kept around for future use, but it seems to only be needed for the StreamProviderManager.
Suggest container construction be separated and passed to the OutsideRuntimeClient constructor for testability reasons. Can even be a static factory function if needed.

static IServiceProvider DefaultServiceProvider(ClientConfiguration cfg)
{
var services = new ServiceCollection();
....
return services.BuildServiceProvider();
}

public OutsideRuntimeClient(IServiceProvider sp, ClientConfiguration cfg, bool secondary)
{
...
}

public OutsideRuntimeClient(ClientConfiguration cfg, bool secondary = false)
: this(DefaultServiceProvider(cfg), cfg, secondary)
{
...
}

Suggest container construction be separated and passed to the OutsideRuntimeClient constructor for testability reasons.

Yeah, I'm not entirely happy with the way the container is constructed today - either here or in Silo.cs (also, it's not even consistent: why is it not constructed in InsideRuntimeClient if here it's in OutsideRuntimeClient, seems illogical, right?)

I see it as a post-PR topic, since we should change both and move towards a 'SiloBuilder'/'ClientBuilder' approach. I'm certainly not firm on that, but I'd like to have a separate discussion on what the root of our system is. Currently, I'd say it's the IRuntimeClient implementation, but IRuntimeClient is internal and so users cannot make use of it.

Understood. Huge PR, so I'm quite ok with pushing this to a later refactor. Just wanted to make sure I understood why it was this way. Having said that, the more of this that you can structure well in this pass, the more well formed the code will be if we don't get back to this immediately. I'm not confident we've a good record of doing cleanup passes yet.

ReubenBond · 2017-01-18T21:44:08Z

src/Orleans/Core/GrainExtensions.cs

+        /// </summary>
+        /// <param name="grain">The grain reference.</param>
+        /// <param name="grainFactory">The grain factory.</param>
+        public static void BindGrainReference(this IAddressable grain, IGrainFactory grainFactory)


This is a convenience method to allow grains to be bound to the grain factory. As you can see, it's trivial. Should it be removed?

jason-bragg · 2017-01-19T22:37:41Z

src/OrleansProviders/Streams/Memory/MemoryMessageBody.cs

@@ -29,16 +30,28 @@ public interface IMemoryMessageBodySerializer
    /// Default IMemoryMessageBodySerializer
    /// </summary>
    [Serializable]
-    public class DefaultMemoryMessageBodySerializer : IMemoryMessageBodySerializer
+    public class DefaultMemoryMessageBodySerializer : IMemoryMessageBodySerializer, IOnDeserialized


After looking at this a bit more, I think a better solution would be for the [de]serialization calls in the IMemoryMessageBodySerializer to take a serialization manager as an argument, and for the GetEvents call in the IBatchContainer to also take a serialization manager. The stream consumer extension getting the messages from the batch container would need to pass in a serialization manager. This, IMO, is a more straight forward approach.

@jason-bragg I'm happy with that approach. I didn't want to change the interface, since it implies that it's supposed to be serializer agnostic.

I don't want to have to add IOnDeserialized just for this.

Understood. I'd also prefer a solution that didn't alter the public surface, as this will be a breaking change, but I'm more open to breaking changes in the streaming system as that is still one of the newer systems which is, imo, still in flux. We can also probably count the number of users that implemented custom IBatchContainers on one hand...

jdom · 2017-02-23T18:07:35Z

                                "Type {0} from assembly {1} has the RegisterSerializer attribute but no public static void Register method",

I'm not sure if I had a comment on a previous PR or it was here and got lost, but this exception message needs an update, especially since it's a sneaky breaking change that would not break at compile time.

Refers to: src/Orleans/Serialization/SerializationManager.cs:634 in 975bdbb. [](commit_id = 975bdbb4fbfb5e7d8504df67bd6880a16803bbc2, deletion_comment = False)

jdom · 2017-02-23T18:14:58Z

src/OrleansProviders/Streams/Memory/MemoryBatchContainer.cs

+
+        void IOnDeserialized.OnDeserialized(ISerializerContext context)
+        {
+            this.serializer = ActivatorUtilities.GetServiceOrCreateInstance<TSerializer>(context.ServiceProvider);


Awesome, this is what I was advocating for. Nevertheless, for performance reasons, I think there is a method in ActivatorUtilities that allows you to create an object factory and cache it, and avoid reflection in subsequent calls, since this is going to be called extremely frequently for these same types

The updated method would look like this:

this.serializer = context.ServiceProvider.GetService<TSerializer>() ?? (TSerializer)ObjectFactory(context.ServiceProvider, null);

Where ObjectFactory is

private static readonly ObjectFactory ObjectFactory = ActivatorUtilities.CreateFactory( typeof(TSerializer), null);

Sound good?

sounds good, although I'd make this field Lazy<ObjectFactory> so that exceptions flow to the right place, and not on type initialization.

BTW, I'd call this field serializerFactory, not ObjectFactory.

jdom · 2017-02-23T18:19:54Z

src/OrleansProviders/Streams/Memory/MemoryMessageBody.cs

+
+        void IOnDeserialized.OnDeserialized(ISerializerContext context)
+        {
+            this.serializationManager = context.SerializationManager;


correct me if I'm wrong, but with the latest, this class is never (de)serialized, so the IOnDeserialized interface is not needed here. It will always be constructed using ActivatorUtilities. In theory you could also drop the [Serializable] attribute from it (it wasn't being used before either). Unless you added this in case someone was incorrectly using it and serializing it for a different purpose?

Unless you added this in case someone was incorrectly using it and serializing it for a different purpose?

precisely :)

jdom

I'm happy with these changes, I believe the IOnDeserialized interface is an elegant solution.

ReubenBond · 2017-02-23T21:05:16Z

How does this sound for an updated error message?

"Type {0} from assembly {1} has the RegisterSerializer attribute but no public static void Register(SerializationManager sm) method."

jdom · 2017-02-23T21:15:18Z

Message looks good. We might want to assert if the old signature was used instead, and provide a more informative message, but I'm fine not doing this extra work.

ReubenBond · 2017-02-23T21:16:18Z

I think we should make Roslyn Analyzers for these cases - it's a much better experience

jdom · 2017-02-23T21:25:40Z

src/OrleansProviders/Streams/Memory/MemoryAdapterFactory.cs

@@ -79,6 +72,7 @@ public void Init(IProviderConfiguration providerConfig, string name, Logger log,

            // 10 meg buffer pool.  10 1 meg blocks
            bufferPool = new FixedSizeObjectPool<FixedSizeBuffer>(adapterConfig.CacheSizeMb, () => new FixedSizeBuffer(1 << 20));
+            this.serializer = ActivatorUtilities.CreateInstance<TSerializer>(this.serviceProvider);


hmmm, this is creating the serializer differently than in MemoryBatchContainer...

Consolidated in latest

jdom · 2017-02-23T21:42:47Z

src/OrleansProviders/Streams/Memory/MemoryAdapterFactory.cs

@@ -79,6 +77,9 @@ public void Init(IProviderConfiguration providerConfig, string name, Logger log,

            // 10 meg buffer pool.  10 1 meg blocks
            bufferPool = new FixedSizeObjectPool<FixedSizeBuffer>(adapterConfig.CacheSizeMb, () => new FixedSizeBuffer(1 << 20));
+
+            this.serializer = this.serviceProvider.GetService<TSerializer>() ??
+                              (TSerializer)ObjectFactory.Value(this.serviceProvider, null);


nit: I'd avoid duplication (which should always match) by extracting this to an internal static CreateSerializer(IServiceProvider) method, and then the MemoryBatchContainer just calls MemoryAdapterFactory.CreateSerializer

I figured duplication was cheaper, but I'll change it.

Does the latest commit look like what you're after?

absolutely, thanks

jason-bragg · 2017-02-23T22:26:22Z

LG2M

jdom · 2017-02-24T01:30:46Z

Woohooo!! this is in! 🎉 Thanks @ReubenBond for bearing with us! :)

ReubenBond · 2017-02-24T01:32:37Z

I'm so happy :D Thanks for putting up with it - it was a long road and a big PR, but this lays the groundwork for more modularity as well as #467

attilah · 2017-02-24T04:09:47Z

Shipit!

sergeybykov · 2017-02-25T01:30:10Z

This was yet another epic thing by Reuben!

ReubenBond added the Status: work-in-progress label Jan 13, 2017

dnfclas added the cla-already-signed label Jan 13, 2017

jason-bragg reviewed Jan 13, 2017

View reviewed changes

ReubenBond force-pushed the remove-statics branch from 142ecb6 to 5d5cbde Compare January 14, 2017 00:21

sergeybykov mentioned this pull request Jan 14, 2017

Attempt to make the serialization manager non static #2100

Closed

sergeybykov added this to the 1.5.0 milestone Jan 14, 2017

ReubenBond mentioned this pull request Jan 15, 2017

Serialization/CodeGen: Avoid generating Register methods #2596

Merged

ReubenBond force-pushed the remove-statics branch 5 times, most recently from 1e37449 to 5fafab1 Compare January 18, 2017 21:42

ReubenBond commented Jan 18, 2017

View reviewed changes

ReubenBond force-pushed the remove-statics branch 6 times, most recently from 43ab123 to ce7c9a8 Compare January 19, 2017 05:27

ReubenBond changed the title ~~[WIP] Non-static SerializationManager~~ Non-static SerializationManager Jan 19, 2017

jason-bragg reviewed Jan 19, 2017

View reviewed changes

ReubenBond force-pushed the remove-statics branch from c227f04 to 6b7eacc Compare January 22, 2017 23:01

ReubenBond force-pushed the remove-statics branch from e5995ff to 24e94d6 Compare February 23, 2017 03:09

revert unchanged files

277082a

ReubenBond force-pushed the remove-statics branch 3 times, most recently from 76f4c07 to 411ef9f Compare February 23, 2017 09:10

Add IOnDeserialized post-deserialization lifecycle hook

46d1e81

ReubenBond force-pushed the remove-statics branch from 411ef9f to 46d1e81 Compare February 23, 2017 09:12

jdom reviewed Feb 23, 2017

View reviewed changes

jdom approved these changes Feb 23, 2017

View reviewed changes

Review feedback

e6bbd08

jdom reviewed Feb 23, 2017

View reviewed changes

review feedback

09e7925

jdom reviewed Feb 23, 2017

View reviewed changes

ReubenBond force-pushed the remove-statics branch from f6e78dc to dfc27e1 Compare February 23, 2017 21:48

MemoryMessageBodySerializerFactory

5a9d837

ReubenBond force-pushed the remove-statics branch from dfc27e1 to 5a9d837 Compare February 24, 2017 00:25

jdom merged commit 70fc5c4 into dotnet:master Feb 24, 2017

ReubenBond deleted the remove-statics branch February 24, 2017 01:33

jdom mentioned this pull request Feb 27, 2017

Changed the SerializationManager to remove the read locks. #2081 #2082

Closed

github-actions bot locked and limited conversation to collaborators Dec 9, 2023

Non-static SerializationManager #2592

Non-static SerializationManager #2592

Conversation

ReubenBond commented Jan 13, 2017 • edited

xiazen commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReubenBond Jan 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReubenBond Jan 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReubenBond Jan 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReubenBond Jan 13, 2017 • edited

Choose a reason for hiding this comment

jason-bragg Jan 14, 2017 • edited

Choose a reason for hiding this comment

ReubenBond Jan 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdom commented Feb 23, 2017

Choose a reason for hiding this comment

ReubenBond Feb 23, 2017 • edited

Choose a reason for hiding this comment

jdom Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdom left a comment

Choose a reason for hiding this comment

ReubenBond commented Feb 23, 2017

jdom commented Feb 23, 2017

ReubenBond commented Feb 23, 2017

Choose a reason for hiding this comment

ReubenBond Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jason-bragg commented Feb 23, 2017

jdom commented Feb 24, 2017

ReubenBond commented Feb 24, 2017

attilah commented Feb 24, 2017

sergeybykov commented Feb 25, 2017

ReubenBond commented Jan 13, 2017 •

edited

ReubenBond Jan 13, 2017 •

edited

ReubenBond Jan 13, 2017 •

edited

ReubenBond Jan 13, 2017 •

edited

ReubenBond Jan 13, 2017 •

edited

jason-bragg Jan 14, 2017 •

edited

ReubenBond Jan 14, 2017 •

edited

ReubenBond Feb 23, 2017 •

edited

jdom Feb 23, 2017 •

edited

ReubenBond Feb 23, 2017 •

edited