Home Dashboard Directory Help
Search

ExceptionDispatchInfo API modifications by Stephen Cleary


Status: 

Closed
 as Won't Fix Help for as Won't Fix


5
0
Sign in
to vote
Type: Bug
ID: 689516
Opened: 9/19/2011 11:04:32 AM
Access Restriction: Public
Moderator Decision: Sent to Engineering Team for consideration
1
Workaround(s)
view
0
User(s) can reproduce this bug

Description

As a followup to my previous feedback:
http://connect.microsoft.com/VisualStudio/feedback/details/633822/allow-preserving-stack-traces-when-rethrowing-exceptions

The new (.NET 4.5 dev preview) way of preserving a stack trace is as such:
ExceptionDispatchInfo.Capture(ex).Throw();
which does not interoperate well with code analyzers. Currently there is no mechanism for marking a method as "this method always throws".

As such, this causes problems for code analyis - from ReSharper to Code Contracts to the basic C# compiler itself.

I recommend exposing the functionality of Exception.RestoreExceptionDispatchInfo separately from ExceptionDispatchInfo.Throw, so that the exception can be rethrown using the throw keyword explicitly. e.g.:
public void ExceptionDispatchInfo.Restore(); // this.m_Exception.RestoreExceptionDispatchInfo(this);

It would then be easy to create an Exception extension method as such:
public static Exception PrepareForRethrow(this Exception ex)
{ ExceptionDispatchInfo.Capture(ex).Restore(); return ex; }
which can be used as such:
throw ex.PrepareForRethrow();
Details
Sign in to post a comment.
Posted by Microsoft on 10/6/2011 at 4:59 PM
Hello Stephen,

Our main concern with your suggestion is that it will lead to misuse of this API. Having Throw appear to return an exception and throw the exception may make people think they can do things like this:

                Exception e = ExceptionDispatchInfo.Throw();
                // do some other stuff
                throw e;

The “other stuff” will never actually execute.

Additionally, this will create an inconsistency in the way Base Class Library APIs are designed in that Throw will never return anything, and yet the proposed signature will appear to return a value.

ExceptionDispatchInfo is an infrastructure component and apps that use it will be extremely limited - mostly frameworks that use Async. After giving due consideration to your suggestion, we have decided not to fix this in the current release. If we hear enough complaints around this in future we will re-visit this issue.

Thanks,
-Abhishek
Posted by Stephen Cleary on 10/6/2011 at 12:53 PM
Hello, Abhishek -

It is not necessary to change the semantics of "throw". That would be a major change. What I'm asking for is quite minor.

How about this instead: change the return type of "ExceptionDispatchInfo.Throw" from "void" to "Exception". You don't even have to return a value. Would that be acceptable?

It is true that there is a workaround. But doesn't that just strike you wrong? The fact that a workaround is necessary? If there's a need for a "general-purpose library" just for fixing API mistakes, then that is a strong indicator of a failure of the platform. IMO.

Thanks for your consideration,
         -Steve
Posted by Microsoft on 10/6/2011 at 12:40 PM
Hello Stephen,

After taking to the experts in this area and following through your reasoning, I agree that what you are complaining about is a reasonable issue. However we have to change the semantics of "throw" in order to use your suggestion.

As you may understand there is only so much time and resource and so many suggestions coming our way, that we have to end up prioritizing customer requests based on how many people are asking for a feature. Since there is a workaround to the issue you are reporting and since we have no other customers complaining about this, we have decided not to fix this for now.
However, if we do hear many more people asking as to fix this, we will give it due consideration.

Thanks for your suggestion.
Abhishek Mondal
Program Manger
Common Language Runtime.
Posted by Stephen Cleary on 9/23/2011 at 5:42 AM
I'd like to add that this is not an academic exercise. I have real-world code where using Throw - as it currently exists - causes compiler errors. So I find myself having to put in a workaround that I feel should just not be necessary.
Posted by Stephen Cleary on 9/23/2011 at 5:41 AM
Hi, Abhishek -

Thanks for the response. My reply is below.

> The "Throw" method, by design, performs the restoration of exception propagation details and throws the
> exception in an atomic fashion. Separating these two operations will break the invariant and could result in an
> incorrect usage of the API by the user.

I agree. The user *could* misuse the API. Just like a whole lot of other APIs.

However, the only users who would be using this API are ones who are trying to propogate exception details from one context to another. It's not unreasonable to assume that these users would be smart enough to use the API correctly - especially if a note was placed in the documentation summary.

> Also, performing a rethrow ("throw;") requires an exception dispatch to be active and can only be done from within
> a catch block. ExceptionDispatchInfo was design for propagation of exception details across threads, where the
> logical rethrow would need to be performed and that need not happen from inside a catch block - it can happen in
> any part of the application (this is why the method is called "Throw" and not "Rethrow"). Hence, a "throw;" (i.e.
> regular rethrow) will not address this scenario.

Right. If you look at my suggested syntax, I do not recommend "throw;". I recommend "throw (some expression);", which would work perfectly well.

> Finally, the design is also to keep propagation information (EDI) separate from error information (which is the
> Exception object) and thus, we didn’t modify the existing Exception type.

You can still do this. e.g., define a "static Exception ExceptionDispatchInfo.PrepareForRethrow(Exception);" and then it can be used like this:

throw ExceptionDispatchInfo.PrepareForRethrow(ex);

which is not exposed on Exception (so "regular" users won't find it) but it still uses the throw keyword, so it doesn't break code analysis.
Posted by Microsoft on 9/22/2011 at 2:27 PM
Hello Stephen,

The "Throw" method, by design, performs the restoration of exception propagation details and throws the exception in an atomic fashion. Separating these two operations will break the invariant and could result in an incorrect usage of the API by the user.

Also, performing a rethrow ("throw;") requires an exception dispatch to be active and can only be done from within a catch block. ExceptionDispatchInfo was design for propagation of exception details across threads, where the logical rethrow would need to be performed and that need not happen from inside a catch block - it can happen in any part of the application (this is why the method is called "Throw" and not "Rethrow"). Hence, a "throw;" (i.e. regular rethrow) will not address this scenario.

Finally, the design is also to keep propagation information (EDI) separate from error information (which is the Exception object) and thus, we didn’t modify the existing Exception type.
Please let me know if you have any questions.

Thanks,
-Abhishek
Posted by Stephen Cleary on 9/22/2011 at 6:27 AM
Hi Abhishek -

You completely missed my point. I understand how "void" works.

My point is that ExceptionDispatchInfo.Throw is a poor design because it interferes with code analysis.

Consider this restated example (CS0161):


public static int Div(int num, int den)
{
try
{
    return Task.Factory.StartNew(() => num / den).Result;
}
catch (AggregateException ex)
{
    ExceptionDispatchInfo.Capture(ex).Throw();
}
}


or this example (CS0165):


public static string Test()
{
string ret;
try
{
    ret = string.Empty;
}
catch (AggregateException ex)
{
    ExceptionDispatchInfo.Capture(ex.InnerException).Throw();
}

return ret.ToUpper();
}


or this example (CS0163):


public static int Test(int den)
{
int i = 0;

switch (i)
{
    case 13:
     try
     {
        return Task.Factory.StartNew(() => i / den).Result;
     }
     catch (AggregateException ex)
     {
        ExceptionDispatchInfo.Capture(ex.InnerException).Throw();
     }

    default:
     return i;
}
}


These are just compiler errors. The same examples will cause incorrect results in any static code analysis tool (Code Contracts, ReSharper, ...), unless each and every tool adds special code to treat ExceptionDispatchInfo.Throw differently than every other method.

There are workarounds for each of these individual examples. But there shouldn't *need* to be workarounds; it's not hard to change the design so that the throw *keyword* is used instead of a throw *method*. With an improved design (such as the one I suggested), no workarounds are necessary.

Regards,
     -Steve
Posted by Microsoft on 9/21/2011 at 4:48 PM
Hello Stephen,

The Throw() method always throws and hence returns nothing (void).
In your repro code the compiler is complaining because you want Test to return an 'int' wheras it can't return anything since you call Throw().
Changing the return type of Test to 'void' should solve your issue.

Please let me know if you anymore questions.
Thanks,
-Abhishek
Posted by MS-Moderator08 [Feedback Moderator] on 9/19/2011 at 8:25 PM
Thank you for submitting feedback on Visual Studio 2010 and .NET Framework. Your issue has been routed to the appropriate VS development team for investigation. We will contact you if we require any additional information.
Posted by MS-Moderator01 on 9/19/2011 at 11:43 AM
Thank you for your feedback, we are currently reviewing the issue you have submitted. If this issue is urgent, please contact support directly(http://support.microsoft.com)
Sign in to post a workaround.
Posted by Anna Wawrzyniak on 7/24/2012 at 11:27 PM
You can implement an extension method to achieve similar effect:

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using System.Runtime.ExceptionServices;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication
{
    static class ExceptionDispatchInfoExtensionMethods
    {
        public static Exception Throw2(this ExceptionDispatchInfo edi)
        {
            edi.Throw();
            return null;
        }
    }

    class Program
    {
        
        static void Main(string[] args)
        {
            Foo();
        }

        static int Foo()
        {
            try
            {
                throw new Exception("foo");
            }
            catch (Exception ex)
            {
                throw ExceptionDispatchInfo.Capture(ex).Throw2();
            }
        }
    }    
}