Do we need a thread-safe alternative to `ModuleScope.DefineType`?

[stakx]

@zvirja alerted me to the fact that after deprecating Lock (in #391), it seems no longer possible in the layers above DynamicProxy to use ModuleScope.DefineType in a thread-safe manner; see https://github.com/nsubstitute/NSubstitute/pull/420/files#r197765187.

I see three options to resolve this:

0. Do nothing.
   That is, leave it up to downstream libraries to ensure that if they access ModuleScope's ModuleBuilders, they must not call into their ProxyGenerator at the same time.

1. Deprecate ModuleScope.DefineType as well.
   Not very realistic because, for instance, NSubstitute and Moq both rely on this API in order to mock delegate types.

2. Revert Deprecate Lock and ModuleScope's public type cache API #391.
   I'm biased here, but I think it would be a pity because that PR went in the right direction of closing doors into DynamicProxy's internals.

3. Expose the ReaderWriterLockSlim lock that ModuleScope uses internally.
   ModuleScope has an API that exposes the two ModuleBuilders that it uses, which essentially means that once a user starts accessing ModuleBuilders, all thread-safety guarantees are off. We can no longer expose a Lock, but we could expose the internal ReaderWriterLockSlim instead.

4. Augment ModuleScope with a thread-safe alternative for DefineType.
   Apart from (0) above, this is my personal preference. This could be in the form of a new method on ModuleScope which performs the necessary locking internally, for example:

   +[Obsolete("This method is not thread-safe. Please use `ModuleBuilder.CreateType` instead.")]
    public TypeBuilder DefineType(bool inSignedModulePreferably, string name, TypeAttributes flags)
    {
        …
    }
   
   +public Type CreateType(bool inStrongNamedModulePreferably,
   +                       string name,
   +                       TypeAttributes flags,
   +                       Func<TypeBuilder, Type> build)
   +{
   +    TypeCache.Lock.EnterWriteLock();
   +    try
   +    {
   +        var module = ObtainDynamicModule(disableSignedModule == false && inStrongNamedModulePreferably);
   +        var typeBuilder = module.DefineType(name, flags);
   +        return build.Invoke(typeBuilder);
   +    }
   +    finally
   +    {
   +        TypeCache.Lock.ExitWriteLock();
   +    }
   +}

   (This option would probably entail deprecating the ObtainDynamicModule API as well. ModuleScope would eventually have a less powerful API, but one where thread-safety would already be taken care of for downstream libraries.)

@castleproject/committers, do you see any other options here? If not, which one of the above do you think we should go for?

[zvirja]

@stakx Thanks for raising the issue.

I'm in for the option 4, however I'd vote to shape the API in a bit different way:

public Type DefineTypeSynchronized(
    bool inStrongNamedModulePreferably,
    Func<ModuleBuilder, Type> builder)
{
    TypeCache.Lock.EnterWriteLock();
    try
    {
        var module = ObtainDynamicModule(disableSignedModule == false && inStrongNamedModulePreferably);
        return build.Invoke(module);
    }
    finally
    {
        TypeCache.Lock.ExitWriteLock();
    }
}

Such API will allow to perform the lookup before creating any type - you might want to look up for the already defined type and return it if it exists. That's exactly the scenario I met - I need to define System.Runtime.CompilerServices.IsReadOnlyAttribute type and do that only if it doesn't already exist. Otherwise, the exception about duplicate type is raised.

[stakx]

You might want to look up for the already defined type and return it if it exists.

Wouldn't it be more logical to use a Lazy<Type> at your end? It seems somewhat strange to give access to a whole module, but ask back for only a type.

(Btw. you can still access the module by querying a TypeBuilder's Module property, IIRC.)

[zvirja]

One more thing to consider given that we are around. Currently you work with ModuleBuilder without any synchronization and rely that it's thread-safe (all synchronization is around cache only). However, in fact, this thread safety is not guaranteed and is implementation specific. Given that we target .NET Standard, I'd suggest to play safely.

Therefore, I'm both hands for the option 4 where each access to the ModuleBuilder is fully synchronized.

Wouldn't it be more logical to use a Lazy at your end?

Please provide a sample, not sure I got it 😟

(Btw. you can still access the module by querying a TypeBuilder's Module property, IIRC.)

The point is to test before creating a TypeBuilder. Otherwise, you'll get an exception and it isn't very suitable. If we consume the ModuleBuilder, we have an option to not define any type.

[stakx]

The point is to test before creating a TypeBuilder.

Ah, of course. Now I feel a little stupid. :-D

Granted, perhaps such a new method would have to deal with failure more gracefully. It could be converted to e.g. the Try or GetOrAdd pattern.

Please provide a sample, not sure I got it 😟

(See nsubstitute/NSubstitute#420 (comment).)

[zvirja]

It could be converted to e.g. the Try or GetOrAdd pattern.

Probably it's a good idea. But we must be sure that introduced API enables all the potential usages, rather than a single dedicated scenario. The way with callback provided us with flexibility (e.g. perform any non-trivial look-ups), while GetOrAdd might offer only a limited usage scenario.

Wouldn't it be more logical to use a Lazy at your end? It seems somewhat strange to give access to a whole module, but ask back for only a type.

(See nsubstitute/NSubstitute#420 (comment).)

It might be hard to follow this issue if we need to constantly switch between this issue and original PR caused it 😅 Therefore, let's omit that part from here 😄

The goal is to provide API which allows to synchronize type creation between Castle.Core and any potential client. It might rather a callback or an exposed RWLock - doesn't matter too much. Let's see what other maintainers think about the shape of API.

[jonorossi]

Sorry for the silence from my end. The current API allows a user to obtain the lock and write multiple types before releasing the lock, is this something the new API should also support?

[zvirja]

@jonorossi Could please provide a snippet of code? I want to sync with Castle's code and my own code (so we all are taking the same lock) and am unable to find the appropriate API.

[jonorossi]

Could please provide a snippet of code? I want to sync with Castle's code and my own code (so we all are taking the same lock) and am unable to find the appropriate API.

BaseProxyGenerator.ObtainProxyType synchronises each generator's "GenerateType" factory method using ModuleScope.TypeCache.GetOrAdd, ModuleScope.Lock is the same lock instance. Unless I'm misunderstanding the current code, this is the lock you'd take to be able to call ModuleScope.DefineType

ModuleScope.DefineType was probably never intended for external use even though it is marked public. I need to look at how the NSubstitute and Moq are actually using this for delegate types as I'm unsure why they need to emit their own types into DP's assembly and couldn't just use their own. If it is just a matter of convenience (i.e. not having to deal with internals visible to) then okay. Without me understanding how this is used externally I couldn't really comment on how the public API should look.

[zvirja]

factory method using ModuleScope.TypeCache.GetOrAdd

This method doesn't use the ModuleScope.Lock as a sync object. In fact, the ModuleScope.Lock is never used in your codebase now, so we can't use it reliably. Additionally, the ModuleScope.Lock property is obsolete now.

I'm unsure why they need to emit their own types into DP's assembly and couldn't just use their own.

I wasn't thinking about that before, but yes, it could happen that user tries to create delegate with e.g. internal types for parameters. In this case he can use InternalsVisibleTo for the Castle.Core. But if we decide to use our own assembly, it will be a mess to work with all that stuff both for us and for client.

[jonorossi]

This method doesn't use the ModuleScope.Lock as a sync object. In fact, the ModuleScope.Lock is never used in your codebase now, so we can't use it reliably. Additionally, the ModuleScope.Lock property is obsolete now.

ModuleScope.Lock returns the ModuleScope.cacheLock field which is created from the typeCache, it is the sole reason SynchronizedDictionary has a Lock property of type ReaderWriterLockSlim to expose its internal lock.

this.cacheLock = Lock.CreateFor(typeCache.Lock);

Yes, ModuleScope.Lock is marked obsolete but it won't be removed in the 4.x lineage so is still safe to use.

I wasn't thinking about that before, but yes, it could happen that user tries to create delegate with e.g. internal types for parameters. [...] But if we decide to use our own assembly, it will be a mess to work with all that stuff both for us and for client.

If both Moq and NSubstitute are doing the same thing for mocking delegates maybe this is something DP should support, quickly skimming the Moq codebase I don't yet understand why it needs to create an interface with an Invoke method matching the delegate's signature, if someone wants to explain it to me that would be great, otherwise I'll try to find some time to look at it.

With the amount of work the .NET teams are putting into AOT I can see we'll have scenarios to support in the near future where we can't have others generating random types into our DP assembly, otherwise we won't be able to load up the DP assembly and for things to just work. Moq's delegateInterfaceCache won't work if we loaded up the DP assembly because it wouldn't know about the existing types and would try creating new ones.

I'd like to see if we can do "1. Deprecate ModuleScope.DefineType as well".

[stakx]

skimming the Moq codebase I don't yet understand why it needs to create an interface with an Invoke method matching the delegate's signature, if someone wants to explain it to me that would be great,

I'll try to come up with a detailed explanation, right now I'm not too sure myself. I suspect it has to do with Moq being able to internally store delegate mocks the same way as regular class/interface mocks.

I'd like to see if we can do "1. Deprecate ModuleScope.DefineType as well".

I have a feeling this might be problematic for downstream libs, but regarding Moq, I'll do some brainstorming to see whether there's another way to mock delegates that might also work for NSubstitute et al.

If we cannot deprecate this, we could always use the callbsck approach we're using with GetOrAdd to hide the locking logic. This could either be done for defining types (DefineType(string typeName, Action<TypeBuilder>)) or even at the module level if downstream libs require that much freedom (DoWithStrongNamedModule(Action<ModuleBuilder>) — though I don't like the latter suggestion at all.

[jonorossi]

@stakx thanks. I'm just brainstorming so I'm happy to see us unpick this and work out the best outcome once we know all the details. If we do have to go with the callback option then we should resolve the problem of Moq's delegate cache being broken if the assembly is saved and reloaded.

[zvirja]

I don't yet understand why it needs to create an interface with an Invoke method matching the delegate's signature, if someone wants to explain it to me that would be great, otherwise I'll try to find some time to look at it.

I could explain why we do so for the NSubstitute. Previously to support delegate proxy we created the expression trees and compiled them. However, it has been reported that it was very slow:

Method	Median
SubstituteForInterface	6.3367 us
SubstituteForClass	6.5393 us
SubstituteForDelegate	545.3938 us

I found a way to improve that:

• emit a dynamic interface with the Invoke() method matching the delegate's signature
• ask Castle.Core to create a proxy for that interface
• create a delegate based on the returned proxy's Invoke() method.

It helped a lot:

Method	Mean
SubstituteForInterface	5.165 us
SubstituteForClass	5.399 us
SubstituteForDelegate	10.312 us

Now delegate's proxy is only twice slower which is fine.

It appeared to be very simple, but effective approach. And it allowed to unify the proxy behavior, as we use the Castle in all the scenarios.

Moq's delegateInterfaceCache won't work if we loaded up the DP assembly because it wouldn't know about the existing types and would try creating new ones.

I'm not sure I got the point why you are going to load the DP assembly with the already constructed interface types - they are created at run time, so you don't know about them upfront... 😕 In any case, if the problem is only that we might start to create the duplicated types, it's enough to use the namespace with Guid, so the interface types will be always unique 😉 Will add that at NSubstitute part, it's a very good point!