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.

Sign up for GitHub

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

Jump to bottom

Clone AllowTlsResume as part of SslClientAuthenticationOptionsExtensions.ShallowClone #88155

Merged
merged 1 commit into from
Jun 29, 2023
Merged

Clone AllowTlsResume as part of SslClientAuthenticationOptionsExtensions.ShallowClone #88155

merged 1 commit into from
Jun 29, 2023

Conversation

stephentoub
Copy link
Member

Looks like this was missed as part of adding AllowTlsResume. This helper is used by HttpClient and friends to clone all the settings of an SslClientAuthenticationOptions instance into a new instance.

@stephentoub
Clone AllowTlsResume as part of SslClientAuthenticationOptionsExtensi…
4b2393c 
…ons.ShallowClone

Looks like this was missed as part of adding AllowTlsResume.  This helper is used by HttpClient and friends to clone all the settings of an SslClientAuthenticationOptions instance into a new instance.
@stephentoub stephentoub requested a review from wfurt June 28, 2023 17:57
@ghost ghost assigned stephentoub Jun 28, 2023
@ghost
Copy link

ghost commented Jun 28, 2023

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

Issue Details

Looks like this was missed as part of adding AllowTlsResume. This helper is used by HttpClient and friends to clone all the settings of an SslClientAuthenticationOptions instance into a new instance.

Author: stephentoub
Assignees: -
Labels:

area-System.Net.Security
Milestone: -

This was referenced Jun 28, 2023
Closed
Closed
wfurt
wfurt approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

thanks @stephentoub. Didn't we have some test to cover cases like this?

@stephentoub
Copy link
Member Author

Didn't we have some test to cover cases like this?

Apparently not. Can you add one separately?

@stephentoub stephentoub merged commit 4e16941 into dotnet:main Jun 29, 2023
100 of 103 checks passed
@stephentoub stephentoub deleted the cloneallowtlsresume branch June 29, 2023 10:43
@wfurt
Copy link
Member

wfurt commented Jun 29, 2023

I guess I was thinking about #72326. There is Assert that checks the clone. It makes me wonder why that dod not kick in since AllowTlsResume is initialed to True e.g. not default value. Perhaps because nobody touches it.

How about test that counts properties + comment? That would fail on addition and it would require some attention.

@stephentoub
Copy link
Member Author

How about test that counts properties + comment? That would fail on addition and it would require some attention.

And / or an assert in the relevant clone method that validates the number of properties is what's expected.

We've also now seen multiple places where cloning these is needed... we might consider some kind of public API.

@karelz karelz added this to the 8.0.0 milestone Jul 3, 2023
@wfurt wfurt mentioned this pull request Jul 9, 2023
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wfurt wfurt wfurt approved these changes

Assignees

@stephentoub stephentoub

Labels
area-System.Net.Security
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@stephentoub @wfurt @karelz