Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First-chance HandlerException thrown for optional parameters #449

Closed
jnm2 opened this issue Oct 26, 2018 · 3 comments
Closed

First-chance HandlerException thrown for optional parameters #449

jnm2 opened this issue Oct 26, 2018 · 3 comments
Milestone

Comments

@jnm2
Copy link
Contributor

jnm2 commented Oct 26, 2018

It's good practice to minimize exception throwing and handling where possible. First-chance exceptions cause the debugger to break when you're debugging some unrelated issue which requires you to turn off Just My Code. In order to get to the exception or breakpoint you care about, you have to skip through many exceptions you don't care about. It can make an otherwise short series of debugging sessions frustrating, especially if the debugger is a bit sluggish.

This is a regular thorn in the side caused by other libraries as well, including the BCL itself. See https://github.com/dotnet/corefx/issues/19928#issuecomment-330630112 and lower, for instance.

Culprit:

private IHandler TryGetHandlerFromKernel(DependencyModel dependency, CreationContext context)
{
// we are doing it in two stages because it is likely to be faster to a lookup
// by key than a linear search
var handler = kernel.LoadHandlerByType(dependency.DependencyKey, dependency.TargetItemType, context.AdditionalArguments);
if (handler == null)
{
throw new HandlerException(string.Format("Handler for {0} was not found.", dependency.TargetItemType), null);
}

(The Try name implies that it is not an exceptional case if the handler cannot be gotten from the kernel, and therefore an exception should not be thrown. Optional parameters are indeed not an exceptional case, so it would be better to follow the Try pattern here instead of throwing.)

Repro code that works outside the debugger, suitable for unit testing:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.ExceptionServices;
using Castle.MicroKernel.Registration;
using Castle.Windsor;

public static class Program
{
    public sealed class SomeService
    {
        public SomeService(IEqualityComparer<string> optionalParam = null)
        {
        }
    }

    public static void Main()
    {
        using (var container = new WindsorContainer())
        {
            container.Register(Component.For<SomeService>());

            var service = AssertNoFirstChanceExceptions(() => container.Resolve<SomeService>());

            container.Release(service);
        }
    }

    private static T AssertNoFirstChanceExceptions<T>(Func<T> function)
    {
        T returnValue;
        var firstChanceExceptions = new List<Exception>();

        var handler = new EventHandler<FirstChanceExceptionEventArgs>((sender, e) =>
            firstChanceExceptions.Add(e.Exception));

        AppDomain.CurrentDomain.FirstChanceException += handler;
        try
        {
            returnValue = function.Invoke();
        }
        finally
        {
            AppDomain.CurrentDomain.FirstChanceException -= handler;
        }

        if (firstChanceExceptions.Any())
        {
            Assert.Fail("First-chance exceptions thrown:\r\n\r\n"
                + string.Join("\r\n\r\n", firstChanceExceptions));
        }

        return returnValue;
    }
}
@jnm2
Copy link
Contributor Author

jnm2 commented Oct 27, 2018

I wrote an NUnit test action that fails tests that cause first-chance exceptions. It found another place that Windsor is causing and catching exceptions internally in production code, from tests AssignableHandlersTestCase.Ignores_generic_components_where_generic_constrants_are_violated and OpenGenericsTestCase.ResolveAll_properly_skips_open_generic_service_with_generic_constraints_that_dont_match:

[DebuggerHidden]
public static Type TryMakeGenericType(this Type openGeneric, Type[] arguments)
{
try
{
return openGeneric.MakeGenericType(arguments);
}
catch (ArgumentException)
{
// Any element of typeArguments does not satisfy the constraints specified for the corresponding type parameter of the current generic type.
// NOTE: We try and catch because there's no public API to reliably, and robustly test for that upfront
// there's RuntimeTypeHandle.SatisfiesConstraints method but it's internal.
return null;
}
catch (TypeLoadException e)
{
//Yeah, this exception is undocumented, yet it does get thrown in some cases (I was unable to reproduce it reliably)
var message = new StringBuilder();

//Yeah, this exception is undocumented, yet it does get thrown in some cases (I was unable to reproduce it reliably)

I'm getting TypeLoadException every time.

These two tests each throw and swallow TypeLoadException twice. OpenGenericsTestCase.ResolveAll_properly_skips_open_generic_service_with_generic_constraints_that_dont_match also throws and swallows a third exception in production code:

// 3. at this point we should be 99% sure we have arguments that don't satisfy generic constraints of out service.
throw new GenericHandlerTypeMismatchException(genericArguments, ComponentModel, this);

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 27, 2018

In addition, this is not a first-chance exception in production code, but I discovered that CreatingContainerTestCase.With_configuration_file_relative loads XmlFiles/hasFileIncludes.xml which has an XML file with an invalid include URI.

<configuration>
  <include uri="include1.xml"/>
  <!-- this is also valid -->
  <include uri="file://include2.xml"/>
</configuration>
First-chance exceptions:
System.ArgumentException: Invalid Uri: no scheme delimiter found on XmlFiles/hasFileIncludes.xml
   at Castle.Core.Resource.CustomUri.ParseIdentifier(String identifier)
   at Castle.MicroKernel.SubSystems.Resource.DefaultResourceSubSystem.CreateResource(String resource)
   at Castle.Windsor.WindsorContainer.GetInterpreter(String configurationUri)

Looks like a bad assumption on the part of whoever wrote hasFileIncludes.xml? Either that, or this is intended behavior and this too represents a first-chance exception that happens in production code.

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 27, 2018

Here is the source I'm using to make these tests fail:

DetectFirstChanceExceptionsAttribute.cs
#if !NETCOREAPP1_0
[assembly: CastleTests.DetectFirstChanceExceptions]

namespace CastleTests
{
	using System;
	using System.Collections.Generic;
	using System.Diagnostics;
	using System.Linq;
	using System.Reflection;
	using System.Runtime.ExceptionServices;
	using System.Text;

	using NUnit.Framework;
	using NUnit.Framework.Constraints;
	using NUnit.Framework.Interfaces;
	using NUnit.Framework.Internal;

	public sealed class DetectFirstChanceExceptionsAttribute : Attribute, ITestAction
	{
		private readonly Dictionary<string, List<Exception>> firstChanceExceptionsByTest = new Dictionary<string, List<Exception>>();

		public void BeforeTest(ITest test)
		{
			firstChanceExceptionsByTest.Add(test.Id, new List<Exception>());
			AppDomain.CurrentDomain.FirstChanceException += CurrentDomain_FirstChanceException;
		}

		public void AfterTest(ITest test)
		{
			AppDomain.CurrentDomain.FirstChanceException -= CurrentDomain_FirstChanceException;

			var firstChanceExceptions = firstChanceExceptionsByTest[test.Id];
			firstChanceExceptionsByTest.Remove(test.Id);

			if (firstChanceExceptions.Any())
			{
				var message = new StringBuilder();
				for (var i = 0; i < firstChanceExceptions.Count; i++)
				{
					message.Append("First-chance exception ").Append(i + 1).Append(" of ").Append(firstChanceExceptions.Count).AppendLine(":");
					message.AppendLine(firstChanceExceptions[i].ToString());
					message.AppendLine();
				}

				message.Append("Expected: no first-chance exceptions.");

				Assert.Fail(message.ToString());
			}
		}

		// This one neither can be inlined nor can tail call, so it will be in the stack trace if this family of methods is used.
		private static readonly MethodInfo AssertThrowsMethod = typeof(Assert).GetMethod(
			nameof(Assert.Throws),
			BindingFlags.Public | BindingFlags.Static | BindingFlags.DeclaredOnly,
			null,
			new[] { typeof(IResolveConstraint), typeof(TestDelegate), typeof(string), typeof(object[]) },
			null);

		private void CurrentDomain_FirstChanceException(object sender, FirstChanceExceptionEventArgs e)
		{
			var exceptionTrace = new StackTrace(e.Exception);

			if (exceptionTrace.FrameCount != 0
			    && exceptionTrace.GetFrame(0).GetMethod().DeclaringType?.Assembly == TestExecutionContext.CurrentContext.CurrentTest.TypeInfo.Assembly)
			{
				// Ignore exceptions thrown by methods declared in the test project
				return;
			}

			// exceptionTrace doesn’t go far enough back to include the Assert.Throws method
			var fullTrace = new StackTrace();
			for (var i = 0; i < fullTrace.FrameCount; i++)
			{
				if (fullTrace.GetFrame(i).GetMethod() == AssertThrowsMethod) return;
			}

			firstChanceExceptionsByTest[TestContext.CurrentContext.Test.ID].Add(e.Exception);
		}

		public ActionTargets Targets => ActionTargets.Test;
	}
}
#endif

@jonorossi jonorossi added this to the v5.0 milestone Oct 29, 2018
@jonorossi jonorossi reopened this Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants