Capgemini / Cauldron

C# Toolkit
MIT License
77 stars 18 forks source link

IMethodAttribute not working as expected for non async method #61

Closed Dhairya-Sangoi closed 6 years ago

Dhairya-Sangoi commented 6 years ago
    class InterceptedClass
    {
        [InterceptorAttribute]
        public void VoidFunction()
        {
            Console.WriteLine("void function");
        }
    }

The above code gets converted into:

  internal class InterceptedClass
  {
    private IMethodInterceptor <VoidFunction>_attrib0_fv4gr5f5100x1;

    public void VoidFunction()
    {
      if (this.<VoidFunction>_attrib0_fv4gr5f5100x1 == null)
        this.<VoidFunction>_attrib0_fv4gr5f5100x1 = (IMethodInterceptor) new InterceptorAttribute();
      try
      {
        // ISSUE: method reference
        // ISSUE: type reference
        this.<VoidFunction>_attrib0_fv4gr5f5100x1.OnEnter(typeof (InterceptedClass), (object) this, MethodBase.GetMethodFromHandle(__methodref (InterceptedClass.VoidFunction), __typeref (InterceptedClass)), (object[]) null);
        Console.WriteLine("void function");
      }
      catch (Exception ex)
      {
        if (!this.<VoidFunction>_attrib0_fv4gr5f5100x1.OnException(ex))
          return;
        throw;
      }
      finally
      {
        this.<VoidFunction>_attrib0_fv4gr5f5100x1.OnExit();
      }
    }
  }

Note that there is a new property private IMethodInterceptor <VoidFunction>_attrib0_fv4gr5f5100x1 generated for the VoidFunction() method. This property is instantiated only once, during the first invocation of VoidFunction(). For next invocations of VoidFunction(), the same instance is used. This works as expected if no state is stored in InterceptorAttribute class. But if we store any state in the InterceptorAttribute class, then it is no longer thread-safe.

For async methods, this is not the case, as a new state machine instance is created for each invocation, and the InterceptorAttribute class instance is a property for that state machine class, which gets instantiated for each invocation of the async method.

There might be a need to store states in InterceptorAttribute class, since there are no parameters currently provided with OnExit() method of the IMethodInterceptor interface. So, if we need to log, lets say, the execution time, then we need to store the start time in the InterceptorAttribute object during the OnEntry() method call.

Some of the fixes could be:

Cheers!

reflection-emit commented 6 years ago

Have you tried using the ISyncRoot interface? ISyncRoot.

The ISyncRoot interface introduces an object that can be used to lock the attribute. It's what I did here: InjectAttribute

Dhairya-Sangoi commented 6 years ago

What if a unique state is required for each invocation of the method? The solution you proposed would not be able to handle unique states for each invocation. Locking the sync object cannot solve two parallel threads setting the value one after the other. The first thread still expects the value it set during OnEntry() to be same as when it would access during OnExit(). But the second thread overwrites the value once the first thread releases the lock when its OnEntry() method returns.

Dhairya-Sangoi commented 6 years ago

Currently I am trying to log the execution time of the method. For this, the start time needs to be stored in the attribute class created. t0 -> Thread A sets start time in OnEntry() method t1 -> Thread B sets start time in OnEntry() method t2 -> In OnExit() method, Thread A retrieves start time it set during OnEntry() method to calculate execution time. But, the time calculated is wrong since the state was modified by Thread B. This happens as the state is shared across multiple threads.

Dhairya-Sangoi commented 6 years ago

Also, storing the state as a dictionary, with threadId as key wont work for async functions, since the function may not resume on the same thread.

reflection-emit commented 6 years ago

Ah ok I see... I'll add an option where it is possible to select between single instance and multi-instance. Because there are certain implementations where it is required that the interceptor has a single instance.

Dhairya-Sangoi commented 6 years ago

That would be great. Thanks!

reflection-emit commented 6 years ago

Version 3.0.16 has this option: Setting the AlwaysCreateNewInstance to true will weave the interceptor to a local variable instead of a field.

Dhairya-Sangoi commented 6 years ago

Hey, thanks for the quick resolution. But it seems, there is a compile time error when using the IMethodInterceptor attribute with async void method. Below is the code which throws compile time error:

class InterceptedClass
{
    [InterceptorWithoutInstance]
    public async void AsyncVoidFunction()
    {
        await Task.Delay(1000);
    }
}

It works correctly with the non async methods and async methods without void return type. Below is the snippet from the build errors:

Source:
mscorlib
TargetSite:
System.Object InvokeMethod(System.Object, System.Object[], System.Signature, Boolean)
Object reference not set to an instance of an object.
Type:
System.NullReferenceException
StackTrace:
   at Cauldron.Interception.Cecilator.Coders.BooleanExpressionCoder.Load(Field field)
   at Weaver_Method.<>c__DisplayClass5_9.<InterceptMethods>b__26(BooleanExpressionCoder x)
   at Cauldron.Interception.Cecilator.Coders.Coder.If(Func`2 booleanExpression, Func`2 then)
   at Weaver_Method.<>c__DisplayClass5_8.<InterceptMethods>b__25(Coder context)
   at Cauldron.Interception.Cecilator.Coders.Coder.Context(Func`2 code)
   at Weaver_Method.InterceptMethods(Builder builder)
Source:
Cauldron.Interception.Cecilator
TargetSite:
Cauldron.Interception.Cecilator.Coders.BooleanExpressionFieldCoder Load(Cauldron.Interception.Cecilator.Field)  cauldronbugfixcheck         
reflection-emit commented 6 years ago

Fixed.... Uploading nuget packages right now

Dhairya-Sangoi commented 6 years ago

It works now. Thanks!

Dhairya-Sangoi commented 6 years ago

Hey, when I am using Cauldron with my project (attaching the csproj), I am getting the following errors during weaving. I tried debugging a lot, but couldn't find any source of the issue. This is the error:

2>MSBUILD : error : Failed to execute weaver Cauldron.Interception.Fody.3.0.17\netclassicweaver\Cauldron.Interception.Fody.dll
2>MSBUILD : error : Type:
2>MSBUILD : error : System.Exception
2>MSBUILD : error : StackTrace:
2>MSBUILD : error :    at InnerWeaver.ExecuteWeavers()
2>MSBUILD : error :    at InnerWeaver.Execute()
2>MSBUILD : error : Source:
2>MSBUILD : error : FodyIsolated
2>MSBUILD : error : TargetSite:
2>MSBUILD : error : Void ExecuteWeavers()
2>MSBUILD : error : Object reference not set to an instance of an object.
2>MSBUILD : error : Type:
2>MSBUILD : error : System.NullReferenceException
2>MSBUILD : error : StackTrace:
2>MSBUILD : error :    at Cauldron.Interception.Cecilator.TypeDefinitionEqualityComparer.GetHashCode(TypeDefinition obj)
2>MSBUILD : error :    at System.Linq.Set`1.InternalGetHashCode(TElement value)
2>MSBUILD : error :    at System.Linq.Set`1.Find(TElement value, Boolean add)
2>MSBUILD : error :    at System.Linq.Enumerable.<DistinctIterator>d__64`1.MoveNext()
2>MSBUILD : error :    at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
2>MSBUILD : error :    at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
2>MSBUILD : error :    at Cauldron.Interception.Cecilator.CecilatorBase..ctor(WeaverBase weaver)
2>MSBUILD : error :    at Cauldron.Interception.Cecilator.Extension.CreateBuilder(WeaverBase weaver)
2>MSBUILD : error :    at Cauldron.Interception.Cecilator.WeaverBase.Execute()
2>MSBUILD : error :    at InnerWeaver.ExecuteWeavers()
2>MSBUILD : error : Source:
2>MSBUILD : error : Cauldron.Interception.Cecilator
2>MSBUILD : error : TargetSite:
2>MSBUILD : error : Int32 GetHashCode(Mono.Cecil.TypeDefinition)
2>MSBUILD : error :

This is the csproj format which I am using:

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net461</TargetFramework>
    <IsServiceFabricServiceProject>True</IsServiceFabricServiceProject>
    <RuntimeIdentifier>win7-x64</RuntimeIdentifier>
    <Platforms>AnyCPU;x64</Platforms>
    <GenerateAssemblyInfo>false</GenerateAssemblyInfo>
  </PropertyGroup>

  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
    <DefineConstants>TRACE;DEBUG;NET461</DefineConstants>
  </PropertyGroup>

  <ItemGroup>
    <Content Include="FodyWeavers.xml" />
  </ItemGroup>

  <ItemGroup>
    <Reference Include="Microsoft.CodeDom.Providers.DotNetCompilerPlatform, Version=1.0.7.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
      <HintPath>..\packages\Microsoft.CodeDom.Providers.DotNetCompilerPlatform.1.0.7\lib\net45\Microsoft.CodeDom.Providers.DotNetCompilerPlatform.dll</HintPath>
    </Reference>
    <Reference Include="Microsoft.CSharp" />
    <Reference Include="System.Web.DynamicData" />
    <Reference Include="System.Web.Entity" />
    <Reference Include="System.Web.ApplicationServices" />
    <Reference Include="System.ComponentModel.DataAnnotations" />
    <Reference Include="System" />
    <Reference Include="System.Data" />
    <Reference Include="System.Core" />
    <Reference Include="System.Data.DataSetExtensions" />
    <Reference Include="System.Web.Extensions" />
    <Reference Include="System.Xml.Linq" />
    <Reference Include="System.Drawing" />
    <Reference Include="System.Web" />
    <Reference Include="System.Xml" />
    <Reference Include="System.Configuration" />
    <Reference Include="System.Web.Services" />
    <Reference Include="System.EnterpriseServices" />
  </ItemGroup>
  <ItemGroup>
    <DotNetCliToolReference Include="Microsoft.VisualStudio.Web.CodeGeneration.Tools" Version="2.0.2" />
  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="Cauldron.Interception.Fody" Version="3.0.17" />
  </ItemGroup>

  <ItemGroup>
    <WCFMetadata Include="Connected Services" />
  </ItemGroup>

</Project>

Note: <GenerateAssemblyInfo>false</GenerateAssemblyInfo> this was added to the csproj <PropertyGroup></PropertyGroup> since not adding it was throwing exception, since we already have the AssemblyInfo.cs file. I tried removing our AssemblyInfo.cs file and removing the GenerateAssemblyInfo property, but it still throws the same issue.

Note: Interceptors\Cauldron.BasicInterceptors-3.0.0.17.dll exists in the same folder as the csproj file in file system. I haven't included it as an <ItemGroup></ItemGroup> in the csproj file as of now. But including it in the csproj also throws same error.

Did you face some similar issues with any particular project? Also, its hard to debug since I was unable to attach to the msbuild.exe. Could you please suggest how to debug Cauldron/Fody. This would be very helpful.

Thanks!

reflection-emit commented 6 years ago

This is a NetCore project right?

Debugging the weaver is a bit tricky... I usually rely on the stack trace... It's enough to track down the bugs in most cases actually. In this case I am not really sure what is causing this. Types shouldn't be null... I added the following code now, this will hopefully solve the problem.

.Where(x => x.Module != null && x.Module.Assembly != null)

Download the attach file and replace your version of the dll. Cauldron.Interception.Cecilator.dll.zip There are two paths... One in the .nuget directory and one the solutions package directory. I am not sure which one is used by Fody in your case. Just replace both.

Dhairya-Sangoi commented 6 years ago

Hey, I used the dll which you provided. Now I am getting the following error:

2>MSBUILD : error : Failed to execute weaver Cauldron.Interception.Fody.3.0.17\netclassicweaver\Cauldron.Interception.Fody.dll
2>MSBUILD : error : Type:
2>MSBUILD : error : System.Exception
2>MSBUILD : error : StackTrace:
2>MSBUILD : error :    at InnerWeaver.ExecuteWeavers()
2>MSBUILD : error :    at InnerWeaver.Execute()
2>MSBUILD : error : Source:
2>MSBUILD : error : FodyIsolated
2>MSBUILD : error : TargetSite:
2>MSBUILD : error : Void ExecuteWeavers()
2>MSBUILD : error : Unable to proceed. The type 'System.Reflection.IntrospectionExtensions' does not contain a method 'GetTypeInfo'
2>MSBUILD : error : Type:
2>MSBUILD : error : Cauldron.Interception.Cecilator.MethodNotFoundException
2>MSBUILD : error : StackTrace:
2>MSBUILD : error :    at Cauldron.Interception.Cecilator.BuilderType.GetMethodInternal(Modifiers modifier, BuilderType returnType, String name, Boolean throwException, Nullable`1 parameterCount, TypeReference[] parameterTypes)
2>MSBUILD : error :    at Cauldron.Interception.Cecilator.BuilderType.GetMethod(String name, Int32 parameterCount, Boolean throwException)
2>MSBUILD : error :    at Cauldron.Interception.Fody.ModuleWeaver.<>c.<AddEntranceAssemblyHACK>b__1_1(BuilderType y)
2>MSBUILD : error :    at Cauldron.Interception.Cecilator.Extension.With[TType,TNew](TType target, Func`2 predicate)
2>MSBUILD : error :    at Cauldron.Interception.Fody.ModuleWeaver.AddEntranceAssemblyHACK(Builder builder)
2>MSBUILD : error :    at Cauldron.Interception.Fody.ModuleWeaver.OnExecute()
2>MSBUILD : error :    at Cauldron.Interception.Cecilator.WeaverBase.Execute()
2>MSBUILD : error :    at InnerWeaver.ExecuteWeavers()
2>MSBUILD : error : Source:
2>MSBUILD : error : Cauldron.Interception.Cecilator
2>MSBUILD : error : TargetSite:
2>MSBUILD : error : Cauldron.Interception.Cecilator.Method GetMethodInternal(Cauldron.Interception.Cecilator.Modifiers, Cauldron.Interception.Cecilator.BuilderType, System.String, Boolean, System.Nullable`1[System.Int32], Mono.Cecil.TypeReference[])
2>MSBUILD : error :

Actually this was the error which I was getting from the start. But I made some changes to the project to resolve it, and started getting the previously mentioned NullReferenceException error. I assumed, the current error IntrospectionExtensions was resolved and NullReferenceException is the error I am getting after fixing the IntrospectionExtensions one. But I guess I am wrong. IntrospectionExtensions is the actual error.

Dhairya-Sangoi commented 6 years ago

I checked if System.Reflection.IntrospectionExtensions namespace contains GetTypeInfo method bu looking in the C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.6.1\mscorlib.dll file.

reflection-emit commented 6 years ago

The error with the IntrospectionExtensions is solved on 3.0.18 ... I found that yesterday when I compiled the first time in my life a Net47 project :) It was actually there because of UWP. Until 462 IntrospectionExtensions has that method... Now its somehow gone?

IntrospectionExtensions is now completely removed in 3.0.18... It will be only weaved on UWP projects now.

Dhairya-Sangoi commented 6 years ago

Actually, my project is not a UWP. So, would this resolve my issue as well?

reflection-emit commented 6 years ago

Yap... It should because it's removed 👍

reflection-emit commented 6 years ago

This is the replacement for that particular code GetAssemblyWeaver As you can see, the IntrospectionExtensions is only weaved in if the assembly is UWP.

Dhairya-Sangoi commented 6 years ago

It works with the latest nuget version. Thanks!