{"action":"created","issue":{"url":"https://api.github.com/repos/castleproject/Core/issues/193","repository_url":"https://api.github.com/repos/castleproject/Core","labels_url":"https://api.github.com/repos/castleproject/Core/issues/193/labels{/name}","comments_url":"https://api.github.com/repos/castleproject/Core/issues/193/comments","events_url":"https://api.github.com/repos/castleproject/Core/issues/193/events","html_url":"https://github.com/castleproject/Core/issues/193","id":164036149,"node_id":"MDU6SXNzdWUxNjQwMzYxNDk=","number":193,"title":"TypeLoadException or BadImageFormatException under heavy multi-threaded proxy generation","user":{"login":"bluerobotch","id":3074874,"node_id":"MDQ6VXNlcjMwNzQ4NzQ=","avatar_url":"https://avatars2.githubusercontent.com/u/3074874?v=4","gravatar_id":"","url":"https://api.github.com/users/bluerobotch","html_url":"https://github.com/bluerobotch","followers_url":"https://api.github.com/users/bluerobotch/followers","following_url":"https://api.github.com/users/bluerobotch/following{/other_user}","gists_url":"https://api.github.com/users/bluerobotch/gists{/gist_id}","starred_url":"https://api.github.com/users/bluerobotch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bluerobotch/subscriptions","organizations_url":"https://api.github.com/users/bluerobotch/orgs","repos_url":"https://api.github.com/users/bluerobotch/repos","events_url":"https://api.github.com/users/bluerobotch/events{/privacy}","received_events_url":"https://api.github.com/users/bluerobotch/received_events","type":"User","site_admin":false},"labels":[{"id":332238400,"node_id":"MDU6TGFiZWwzMzIyMzg0MDA=","url":"https://api.github.com/repos/castleproject/Core/labels/bug","name":"bug","color":"e11d21","default":true},{"id":84189335,"node_id":"MDU6TGFiZWw4NDE4OTMzNQ==","url":"https://api.github.com/repos/castleproject/Core/labels/up-for-grabs","name":"up-for-grabs","color":"0e8a16","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":114,"created_at":"2016-07-06T09:35:56Z","updated_at":"2018-06-25T07:29:15Z","closed_at":null,"author_association":"NONE","body":"We use Moq 4.5.10 with Castle.Core 3.3.3 and xUnit 2.1.0 for our unit tests. For xUnit we have the option activated to only have one app domain for all tests. We also enabled parallelization of test execution in xUnit.\n\nSporadically we get TypeLoadExceptions like the following example:\n`System.TypeLoadException : Could not load type 'Castle.Proxies.Invocations.IExecutorAccessor_Prepare' from assembly 'DynamicProxyGenAssembly2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=a621a9e7e5c32e69'.\nat Castle.Proxies.IExecutorAccessorProxy.Prepare[TExecutionUnit](TExecutionUnit executionUnit)\n....\n`\n\nWe reviewed the source code of Moq to ensure there is no threading issue with the ProxyGenerator caching. But Moq looks fine.\n\nWhen disabling xUnit test parallelization we don't get TypeLoadExceptions anymore. For this reason we think it is a race condition in Castle.Core or even in the .Net Framework.\n\nAs far as we see, the missing type is the type that is created for the invocation by InterfaceProxyWithoutTargetContributor.GetInvocationType. The TypeLoadException seems to be raised in the code that is emitted for the invocation in MethodWithInvocationGenerator.BuildProxiedMethodBody. The invocation type is built before it is used in the emitted code. Therefore we do not see a reason why the type cannot be loaded.\n"},"comment":{"url":"https://api.github.com/repos/castleproject/Core/issues/comments/399858315","html_url":"https://github.com/castleproject/Core/issues/193#issuecomment-399858315","issue_url":"https://api.github.com/repos/castleproject/Core/issues/193","id":399858315,"node_id":"MDEyOklzc3VlQ29tbWVudDM5OTg1ODMxNQ==","user":{"login":"stakx","id":104481,"node_id":"MDQ6VXNlcjEwNDQ4MQ==","avatar_url":"https://avatars1.githubusercontent.com/u/104481?v=4","gravatar_id":"","url":"https://api.github.com/users/stakx","html_url":"https://github.com/stakx","followers_url":"https://api.github.com/users/stakx/followers","following_url":"https://api.github.com/users/stakx/following{/other_user}","gists_url":"https://api.github.com/users/stakx/gists{/gist_id}","starred_url":"https://api.github.com/users/stakx/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stakx/subscriptions","organizations_url":"https://api.github.com/users/stakx/orgs","repos_url":"https://api.github.com/users/stakx/repos","events_url":"https://api.github.com/users/stakx/events{/privacy}","received_events_url":"https://api.github.com/users/stakx/received_events","type":"User","site_admin":false},"created_at":"2018-06-25T07:29:15Z","updated_at":"2018-06-25T07:29:15Z","author_association":"MEMBER","body":"@BrunoJuchli - That's another interesting result!\r\n\r\n> I guess, then, that we should now concentrate on bringing this down to a minmal repro for Microsoft, right?\r\n\r\nRight. However I fear that we're already about as close to minimal as we can get without investing a lot of time to take DynamicProxy apart. I'd say that for the sake of an issue report to Microsoft, we should revert to the official latest NuGet package and treat DynamicProxy a black box implementation detail about which two facts are known: (1) It heavily uses Reflection and Reflection.Emit; and (2) it is to be considered thread-unsafe.\r\n\r\nThen we focus on trimming down the actual repro code. A few suggestions:\r\n\r\n1. Drop the `DuplicateCode` project. Simply have one pre-generated file with the test interfaces:\r\n\r\n   ```csharp\r\n   public interface IClass1 { void Method(); }\r\n   public interface IClass2 { void Method(); }\r\n   public interface IClass3 { void Method(); }\r\n   …\r\n   ```\r\n\r\n2. Instead of having one `TestN` class per `IClassN` interface, let's just have one parameterized `Test` class that receives the various interface types as a `Type interfaceTypeToProxy` parameter. This removes 1000 classes from the repro, as well as some reflection / `Activator.CreateInstance` / `MethodInfo.Invoke` stuff (IIRC).\r\n\r\n3. You may have noticed in (1) that I changed the interface methods' signature to `void ()`. Let's try to remove any and all unneeded interceptor logic, i.e. let's just use a `Castle.DynamicProxy.StandardInterceptor` (which is essentially a no-op).\r\n\r\n---\r\n\r\n> We should give consideration to the observed effect of putting a lock (sequentializing) around it did not prevent the issue from occurring, but did massively decrease the likelihood.\r\n\r\nThat is interesting, isn't it? I thought of one possible source that might contribute to this observation. And that is the fact that we're now using a different type of lock than before. The `Castle.Core.Internal.Lock`s that you have removed are based on `System.Threading.ReaderWriterLockSlim`. The C# `lock` that you're now using is based on `System.Threading.Monitor`. What if `ReaderWriterLockSlim` were buggy and sometimes let two threads enter write or upgradeable read locks?\r\n\r\nIt's highly speculative, but it should be easy to test that theory in two ways:\r\n\r\n1. Move the `lock` from your repro code into `BaseProxyGenerator.ObtainProxyType`. Does this make a difference?\r\n\r\n2. If it does not make a difference, remove the `lock` with `ReaderWriterLockSlim`. Take care to stretch out the locking over taking a read lock first, then an upgradeable read lock, then upgrading to a write lock (which is the pattern DynamicProxy uses). Does that make a difference?"}}
{"actor":{"display_login":"stakx"}}