Skip to content

Conversation

@teo-tsirpanis
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis commented Jul 7, 2024

Fixes #104528.

Similar to #48535, this PR makes StackTrace.ToString resilient to exceptions when performing some more custom attribute retrieving. We do this by introducing ***Safe edition of reflection methods that perform the operation and return a negative result if an exception was thrown, and updating all uses of IsDefined and GetCustomAttributes in the StackTrace class.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 7, 2024
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

@jkotas
Copy link
Member

jkotas commented Jul 8, 2024

I do not think we want to be catching exceptions in reflection to make it resilient against missing assemblies. It is very fragile path. For example, this fix would make unsealing an attribute potentially breaking change going forward.

#104528 should be fixed by ignoring reflection failures in the stacktrace printing code.

@teo-tsirpanis teo-tsirpanis changed the title Ignore type resolution failures when filtering for custom attributes with a sealed type. Ignore type resolution failures when checking for attributes in StackTrace.ToString. Jul 9, 2024
@teo-tsirpanis teo-tsirpanis force-pushed the ca-ignore-resolver-failures branch from 3386509 to 6d27738 Compare July 9, 2024 00:47
…m throwing.

An `IsDefinedSafe` method was added that returns false if checking for the attribute throws, and other usages in the file were updated as well. This also allows us to remove a big try catch block.
@teo-tsirpanis teo-tsirpanis force-pushed the ca-ignore-resolver-failures branch from 6d27738 to 2afa59f Compare July 9, 2024 00:50
@teo-tsirpanis
Copy link
Contributor Author

For example, this fix would make unsealing an attribute potentially breaking change going forward.

Didn't think about this, thanks! I updated the PR as you suggested, and guarded another possible source of resolver failures.

@jkotas
Copy link
Member

jkotas commented Jul 9, 2024

Build breaks...

@teo-tsirpanis
Copy link
Contributor Author

Fixed build failures. Current CI failures are seemingly unrelated.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

@jkotas jkotas merged commit fa6c8b0 into dotnet:main Jul 12, 2024
@teo-tsirpanis teo-tsirpanis deleted the ca-ignore-resolver-failures branch July 12, 2024 17:49
@github-actions github-actions bot locked and limited conversation to collaborators Aug 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

F# program fails with an unprintable exception when FSharp.Core is not referenced.

3 participants