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

Return false from ComWrappers.Try... methods #90553

Merged
merged 2 commits into from
Aug 15, 2023
Merged

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Aug 14, 2023

Return false from ComWrappers.TryGetComInstance/TryGetObject instead of throwing PNSE. It saves callers from needing to protect against PNSE.

Fix #90311

Return false from ComWrappers.TryGetComInstance/TryGetObject instead of
throwing PNSE. It saves callers from needing to protect against PNSE.

Fix dotnet#90311
@ghost
Copy link

ghost commented Aug 14, 2023

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

Issue Details

Return false from ComWrappers.TryGetComInstance/TryGetObject instead of throwing PNSE. It saves callers from needing to protect against PNSE.

Fix #90311

Author: jkotas
Assignees: -
Labels:

area-System.ComponentModel

Milestone: -

@jkotas
Copy link
Member Author

jkotas commented Aug 14, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AaronRobinsonMSFT
Copy link
Member

@ericstj I prefer this approach if there are no concerns.

@ericstj
Copy link
Member

ericstj commented Aug 15, 2023

That's fine by me. I recall the same was discussed to avoid the Windows check in the original fix and folks didn't want to do it.

@ericstj ericstj closed this Aug 15, 2023
@ericstj ericstj reopened this Aug 15, 2023
@ericstj
Copy link
Member

ericstj commented Aug 15, 2023

With this you might be able to remove

else if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)

@JeremyKuhne

@jkotas
Copy link
Member Author

jkotas commented Aug 15, 2023

With this you might be able to remove

I agree. Removed.

@jkotas
Copy link
Member Author

jkotas commented Aug 15, 2023

With this you might be able to remove
I agree. Removed.

The platform analyzer is complaining about the unconditional TryGetComInstance call on mobile platforms. I am going to leave it as is.

@jkotas jkotas closed this Aug 15, 2023
@jkotas jkotas reopened this Aug 15, 2023
@steveharter steveharter self-requested a review August 15, 2023 13:56
@ericstj
Copy link
Member

ericstj commented Aug 15, 2023

The platform analyzer is complaining about the unconditional TryGetComInstance call on mobile platforms. I am going to leave it as is.

Yeah, we'd need to remove that annotation too. I suppose that might be a bit further than we'd want to go though. We'll make the APIs not throw, but they're still not "supporting" this scenario.

@ericstj
Copy link
Member

ericstj commented Aug 15, 2023

I filed one issue for a failing test, the other seems to be a timeout running System.Runtime.Tests on Mac (recurring issue with Macs).

There are two asserts on WASI - but those legs aren't currently blocking.

I think this is OK to merge @jkotas. After that let's trigger a backport to RC1.

@jkotas jkotas merged commit 762030c into dotnet:main Aug 15, 2023
166 of 170 checks passed
@jkotas jkotas deleted the issue-90311 branch August 15, 2023 18:40
@jkotas
Copy link
Member Author

jkotas commented Aug 15, 2023

/backport to release/8.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5870547092

@jkotas
Copy link
Member Author

jkotas commented Aug 15, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/5871431638

@ghost ghost locked as resolved and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PlatformNotSupportedException thrown for System.ComponentModel.TypeDescriptor.NodeFor
5 participants