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

The throw keyword shows the wrong thrown exception line #9518

Closed
jkotas opened this issue Jan 8, 2018 · 16 comments
Closed

The throw keyword shows the wrong thrown exception line #9518

jkotas opened this issue Jan 8, 2018 · 16 comments
Labels
enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@jkotas
Copy link
Member

jkotas commented Jan 8, 2018

From @NicolasDorier on August 22, 2017 14:56

Using .NETCore2.0, Win10, it seems the throw keywor

Using the throw keyword loose all the initial stacktrace information.
You can workaround with ExceptionDispatchInfo.Capture(ex).Throw();, but this seems like a bug to me.

using System;

namespace ConsoleApp3
{
	class Program
	{
		static void Main(string[] args)
		{
			try
			{
				DoStuff();
			}
			catch(Exception ex)
			{
				Console.WriteLine(ex.StackTrace);
			}
			
		}

		private static void DoStuff()
		{
			try
			{
				throw new Exception("Boom!");
			}
			catch(Exception ex)
			{
				throw;
			}
		}
	}
}

Actual output: (Exception line 28)

   at ConsoleApp3.Program.DoStuff() in c:\users\nicolasdorier\documents\visual studio 2017\Projects\ConsoleApp3\ConsoleApp3\Program.cs:line 28
   at ConsoleApp3.Program.Main(String[] args) in c:\users\nicolasdorier\documents\visual studio 2017\Projects\ConsoleApp3\ConsoleApp3\Program.cs:line 11

Expected output: (Exception line 24)

   at ConsoleApp3.Program.DoStuff() in c:\users\nicolasdorier\documents\visual studio 2017\Projects\ConsoleApp3\ConsoleApp3\Program.cs:line 24
   at ConsoleApp3.Program.Main(String[] args) in c:\users\nicolasdorier\documents\visual studio 2017\Projects\ConsoleApp3\ConsoleApp3\Program.cs:line 11

Workaround using ExceptionDispatchInfo.Capture(ex).Throw();:

using System;
using System.Runtime.ExceptionServices;

namespace ConsoleApp3
{
	class Program
	{
		static void Main(string[] args)
		{
			try
			{
				DoStuff();
			}
			catch(Exception ex)
			{
				Console.WriteLine(ex.StackTrace);
			}
			
		}

		private static void DoStuff()
		{
			try
			{
				throw new Exception("Boom!");
			}
			catch(Exception ex)
			{
				ExceptionDispatchInfo.Capture(ex).Throw();
				throw;
			}
		}
	}
}

Copied from original issue: dotnet/corefx#23470

@jkotas
Copy link
Member Author

jkotas commented Jan 8, 2018

From @Clockwork-Muse on August 22, 2017 16:1

... this is the behavior of the standard framework, too, though (And I'm on Win7 at the moment). I'd consider this (mostly) expected behavior.

It might be better to restate this as a request along the lines of:

Consider making the re-throw shortcut contain the original trace information.

@jkotas
Copy link
Member Author

jkotas commented Jan 8, 2018

From @danmosemsft on August 22, 2017 16:8

I see the same behavior in desktop back to 2.0. Which isn't to say ideally it would be improved.

@jkotas
Copy link
Member Author

jkotas commented Jan 8, 2018

From @danmosemsft on August 23, 2017 0:10

In fact I remember logging this as a bug many years ago. @jkotas is this (reset of line number of frame in which throw; statement is) feasible to ever fix?

@jkotas
Copy link
Member Author

jkotas commented Jan 8, 2018

I do not see a fundamental problem with fixing it in .NET Core. The usual due-diligence with figuring out the breaking potential of the change would apply.

Do you happen to have the bug number that you have filled years ago?

Note that C# let's you write exception filters that solve the problem of catching exception conditionally as well. The filters give you even better diagnostic experience because of the crash dump will have the stacktrace of the original throw site preserved.

        try
        {
...
        }
        catch (Exception e) when (....)
        {
...
        }

@jkotas
Copy link
Member Author

jkotas commented Jan 8, 2018

From @NicolasDorier on August 23, 2017 1:59

Really it is expected behavior? I am coding in .NET since 2007 or something... it turns out I only noticed out now...

Note that C# let's you write exception filters that solve the problem of catching exception conditionally as well. The filters give you even better diagnostic experience because of the crash dump will have the stacktrace of the original throw site preserved.

You solved another big mystery for me: I had no idea why Exception filter were useful.

@jkotas
Copy link
Member Author

jkotas commented Jan 8, 2018

From @NicolasDorier on August 23, 2017 2:1

@jkotas Ha, you are right, I thought it was specific to .NET Core, but it turns out it does not, it has always been that way.

https://www.thomaslevesque.com/2015/06/21/exception-filters-in-c-6/

@jkotas
Copy link
Member Author

jkotas commented Jan 8, 2018

From @NicolasDorier on August 23, 2017 2:1

You may close this issue or keep it open if you think improving it in .NET core in the future.

@jkotas
Copy link
Member Author

jkotas commented Jan 8, 2018

From @joperezr on September 14, 2017 22:44

Closing for now given the above discussion. Feel free to reopen if needed.

@jkotas
Copy link
Member Author

jkotas commented Jan 8, 2018

From @danmosemsft on December 6, 2017 23:35

I don't think anyone wants the line number on the throw;, it is a bug, we would accept a fix for it, I think it is clearest to reopen this. The fix would be in CoreCLR.

danmoseley referenced this issue in dotnet/coreclr Feb 21, 2018
* Show the expected stack trace from a rethrown exception.

Fix #15780

* Remove now unused methods

- StackTraceArray::AppendSkipLast
- StackTraceElement::PartiallyEqual
- StackTraceElement::PartialAtomicUpdate
@jnm2
Copy link
Contributor

jnm2 commented Feb 21, 2018

I can't believe this is really happening! 🎉

@felipepessoto
Copy link
Contributor

felipepessoto commented Feb 26, 2018

@danmosemsft, will this change be included in .NET Core 2.1?
What about be ported to .NET Framework?

Thx!

@jnm2
Copy link
Contributor

jnm2 commented Feb 26, 2018

Yes! That is what I immediately wanted to know! This would be very interesting to a large group of people.

@danmoseley
Copy link
Member

@fujiy yes this will be in .NET Core 2.1, albeit not in the Preview 1 as that branched already. I don't think CoreFX ingested it yet, but in a few days, please try it in master.

For .NET Framework - it is marked netfx-port-consider for our next pass through potential ports. I can't give a timeframe for that.

@felipepessoto
Copy link
Contributor

@danmosemsft, @jkotas, @ateoi.

Considering the following example:

using System;

namespace ConsoleApp3
{
    class Program
    {
        static void Main(string[] args)
        {
            try
            {
                DoStuff(); //line 11
            }
            catch(Exception ex)
            {
                Console.WriteLine(ex.StackTrace);
            }
            Console.ReadLine();
        }

        private static void DoStuff()
        {
            try
            {
                throw new ArgumentNullException("Boom!"); //line 24
            }
            catch (ArgumentException)
            {
                throw; //line 28
            }
            catch(Exception)
            {
                throw;
            }
        }
    }
}

Wouldn't be better to keep both frames (the throwing and the re-throwing ones)? This way it would be easier to identify which catch block captured the exception in DoStuff() method.

In this case the output would be:

   at ConsoleApp3.Program.DoStuff() in C:\Users\fepessot\Source\ExceptionTest\Program.cs:line 24
   at ConsoleApp3.Program.DoStuff() in C:\Users\fepessot\Source\ExceptionTest\Program.cs:line 28
   at ConsoleApp3.Program.Main(String[] args) in C:\Users\fepessot\Source\ExceptionTest\Program.cs:line 11

instead of:

   at ConsoleApp3.Program.DoStuff() in C:\Users\fepessot\Source\ExceptionTest\Program.cs:line 24
   at ConsoleApp3.Program.Main(String[] args) in C:\Users\fepessot\Source\ExceptionTest\Program.cs:line 11

safern referenced this issue in dotnet/corefx Apr 4, 2018
* Added tests to verify the reported call stacks for rethrown exceptions, as specified by dotnet/coreclr#15780.

1. Exception rethrown in a method different than the method where it was originally thrown: the stack trace contains both the location in the method where the exception was originally thrown, and the location where the method that threw the exception was called.

2. Exception is thrown and later rethrown in the same method: the stack trace only contains the location where the exception was originally thrown and does not include the location where the exception was rethrown.

* - Make test work with full framework. Test will fail after porting dotnet/core#15780 to full framework.
- Fixed variable name

* Write reported call stack to console

Writing call stack to console to debug test errors on NETFX and Linux.

* Add netfx configuration to test project

* Remove redundant throw

A redundant throw in "ThrowAndRethrowOtherMethod"  was being removed by the optimizer and causing error in exception line calculation.

* Remove full call stack verification

Verify only the rethrown stack frame due to inconsistencies found on build/release exception call stacks - #28059 (comment)
@ericsampson
Copy link

@danmosemsft did this end up getting ported to X version of the netfx? Is there a good way in general of telling which issues labelled 'netfx-port-consider' got addressed or not? Thanks!

@jkotas
Copy link
Member Author

jkotas commented Jun 27, 2019

It was not ported. The .NET Framework release notes list all changes that went into particular .NET Framework version, e.g. https://github.com/microsoft/dotnet/tree/master/releases/net48

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

6 participants