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

[release/6.0] Add System.Net.Http ServerCertificateCustomValidationCallback ILLink Suppression #58508

Merged
merged 2 commits into from
Sep 8, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 1, 2021

Backport of #58456 to release/6.0

Fixes #57537

During PR #57555, it was unclear what the correct suppression syntax would be for a return type of Func<HttpRequestMessage, X509Certificate2?, X509Chain?, SslPolicyErrors, bool>. With some helpful switches to generate the right warning suppression syntax via --generate-warning-suppressions xml in RuntimePackILLinkArgs, this PR adds the correct warning suppression.

/cc @mdh1418

Customer Impact

When supporting either using the native HttpMessageHandler types in Xamarin or SocketsHttpHandler as the underlying handler in HttpClientHandler, some APIs such as ServerCertificateCustomValidationCallback had thrown PNSE when the native handler was enabled. It didn't make sense to always throw PNSE, and a majority of the methods were modified to suppress ILLink error messages except for ServerCertificateCustomValidationCallback because of its return type.

Removes PNSE from ServerCertificateCustomValidationCallback
Adds ILLink error message suppression for ServerCertificateCustomValidationCallback

Without this PR, ServerCertificateCustomValidationCallback will continue to throw PNSE.

Testing

Risk

Low, this PR will reenable ServerCertificateCustomValidationCallback instead of throwing Arg_PlatformNotSupported exception.

@ghost
Copy link

ghost commented Sep 1, 2021

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

Issue Details

Backport of #58456 to release/6.0

/cc @mdh1418

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@karelz karelz added this to the 6.0.0 milestone Sep 3, 2021
@ghost
Copy link

ghost commented Sep 3, 2021

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #58456 to release/6.0

Fixes #57537

During PR #57555, it was unclear what the correct suppression syntax would be for a return type of Func<HttpRequestMessage, X509Certificate2?, X509Chain?, SslPolicyErrors, bool>. With some helpful switches to generate the right warning suppression syntax via --generate-warning-suppressions xml in RuntimePackILLinkArgs, this PR adds the correct warning suppression.

/cc @mdh1418

Customer Impact

When supporting either using the native HttpMessageHandler types in Xamarin or SocketsHttpHandler as the underlying handler in HttpClientHandler, some APIs such as ServerCertificateCustomValidationCallback had thrown PNSE when the native handler was enabled. It didn't make sense to always throw PNSE, and a majority of the methods were modified to suppress ILLink error messages except for ServerCertificateCustomValidationCallback because of its return type.

Removes PNSE from ServerCertificateCustomValidationCallback
Adds ILLink error message suppression for ServerCertificateCustomValidationCallback

Without this PR, ServerCertificateCustomValidationCallback will continue to throw PNSE.

Testing

Risk

Low, this PR will reenable ServerCertificateCustomValidationCallback instead of throwing Arg_PlatformNotSupported exception.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net.Http, os-android

Milestone: -

@marek-safar
Copy link
Contributor

@mdh1418 this needs more work to fix the build

@mdh1418 mdh1418 self-requested a review September 3, 2021 15:01
They're 99% identical so we can just use one .cs file with ifdefs instead. This reduces the chance of missing one of the files when a new method is added.
@lewing
Copy link
Member

lewing commented Sep 7, 2021

is this ready now?

@marek-safar marek-safar merged commit 01c4028 into release/6.0 Sep 8, 2021
@marek-safar marek-safar deleted the backport/pr-58456-to-release/6.0 branch September 8, 2021 07:15
@ghost ghost locked as resolved and limited conversation to collaborators Oct 9, 2021
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.

5 participants