Skip to content

Capture ExceptionDispatchInfo in ToAsyncEnumerable#1736

Open
saul wants to merge 1 commit into
dotnet:mainfrom
saul:capture-edi
Open

Capture ExceptionDispatchInfo in ToAsyncEnumerable#1736
saul wants to merge 1 commit into
dotnet:mainfrom
saul:capture-edi

Conversation

@saul

@saul saul commented Mar 29, 2022

Copy link
Copy Markdown

Hi

This PR fixes an issue where stacktraces of exceptions are lost in ToAsyncEnumerable. See the added test - before this PR, the ThrowsException method isn't mentioned in the stacktrace at all. The stacktrace only contained System.Reactive internals. This PR uses ExceptionDispatchInfo to capture the stacktrace of active exceptions (i.e. those that have been raised).

Many thanks

@saul

saul commented Apr 19, 2022

Copy link
Copy Markdown
Author

@danielcweber any chance you could do a quick review?

@saul

saul commented Mar 19, 2023

Copy link
Copy Markdown
Author

@idg10 any chance of a review here?

@idg10 idg10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The basic goal seems fine, but I've got one question: is this the only place in the entire library where this is an issue, do you know?

(I've mainly been focusing on Rx so far so I'm not familiar with Ix yet. I know we have a load of places in Rx where exceptions get caught. So it surprises me to see only one.)

Comment thread Ix.NET/Source/System.Linq.Async.Tests/System/Linq/AsyncEnumerableTests.cs Outdated
@saul

saul commented Mar 30, 2023

Copy link
Copy Markdown
Author

We make extensive use of System.Reactive in our codebase and this is the only stacktrace issue we've seen.

Thank you for the review - I'll work through the comments tomorrow!

@saul

saul commented May 12, 2023

Copy link
Copy Markdown
Author

@idg10 I believe all comments are resolved now

@saul

saul commented Oct 13, 2023

Copy link
Copy Markdown
Author

@idg10 please can this be merged?

@saul

saul commented Apr 4, 2024

Copy link
Copy Markdown
Author

Gentle nudge @idg10 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants