FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Capture ExceptionDispatchInfo in ToAsyncEnumerable by saul · Pull Request #1736 · dotnet/reactive · GitHub

/ reactive Public

Capture ExceptionDispatchInfo in ToAsyncEnumerable - #1736

Open
saul wants to merge 1 commit into
dotnet:mainfrom
saul:capture-edi
ConversationCommits (1)ChecksFiles changed
Open

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

Conversation

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 commented Apr 19, 2022

Copy link
Copy Markdown
Author

@danielcweber any chance you could do a quick review?

saul commented Mar 19, 2023

Copy link
Copy Markdown
Author

@idg10 any chance of a review here?

idg10 reviewed Mar 30, 2023
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Comment thread Ix.NET/Source/System.Linq.Async.Tests/System/Linq/AsyncEnumerableTests.cs

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 commented May 12, 2023

Copy link
Copy Markdown
Author

@idg10 I believe all comments are resolved now

saul commented Oct 13, 2023

Copy link
Copy Markdown
Author

@idg10 please can this be merged?

saul commented Apr 4, 2024

Copy link
Copy Markdown
Author

Gentle nudge @idg10 :)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

idg10 idg10 approved these changes

Assignees

No one assigned

Labels

[area] Ix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL