Skip to content

SslStream/NegotiateStream breaking change#20979

Merged
gewarren merged 4 commits intodotnet:masterfrom
gewarren:begin-end
Oct 13, 2020
Merged

SslStream/NegotiateStream breaking change#20979
gewarren merged 4 commits intodotnet:masterfrom
gewarren:begin-end

Conversation

@gewarren
Copy link
Contributor

@gewarren gewarren commented Oct 8, 2020

@gewarren gewarren requested a review from a team as a code owner October 8, 2020 00:52
@dotnet-bot dotnet-bot added this to the October 2020 milestone Oct 8, 2020
@gewarren
Copy link
Contributor Author

gewarren commented Oct 8, 2020

It seems like the Remarks sections for each of these overloads may need to be updated. Can you comment on that @wfurt? E.g. for SslStream.BeginAuthenticateAsServer.

@gewarren gewarren requested a review from wfurt October 8, 2020 01:01
@wfurt
Copy link
Member

wfurt commented Oct 8, 2020

Is there specific part of the remarks @gewarren? They generally seem good to me. The Begin* operation does not block and End* must be called to complete the operation.

Calling another Begin before calling End was never allowed and that logic was enforced in the Begin/End functions. With the 5.0 change the Begin/End is implemented on top of underlying Task. So it is possible to call Begin even before End is finished as ling as the underlying Task is done. I think there is marginal window of opportunity when something would fail in the past but may work in 5.0

@@ -0,0 +1,41 @@
### NegotiateStream and SslStream allow successive Begin operations

Error cases on security streams are handled differently and successive calls to `BeginAuthenticateAsServer` or `BeginAuthenticateAsClient` no longer fail.
Copy link
Member

Choose a reason for hiding this comment

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

per the discussion in the original notes, this should be something like "may no longer fail" The exclusiveness is now enforced by the underlying task instead of the Begin/End pair so there is some reface condition. It is still generally not OK to call Begin multiple times without calling End.

@gewarren
Copy link
Contributor Author

gewarren commented Oct 9, 2020

Is there specific part of the remarks @gewarren? They generally seem good to me. The Begin* operation does not block and End* must be called to complete the operation.

Calling another Begin before calling End was never allowed and that logic was enforced in the Begin/End functions. With the 5.0 change the Begin/End is implemented on top of underlying Task. So it is possible to call Begin even before End is finished as ling as the underlying Task is done. I think there is marginal window of opportunity when something would fail in the past but may work in 5.0

I just thought since the implementation was no longer using APM that the wording may need to be changed, but it sounds like the answer is "no". Thanks for checking!

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.

LGTM. thanks.

@gewarren gewarren merged commit 7fdd167 into dotnet:master Oct 13, 2020
@gewarren gewarren deleted the begin-end branch October 13, 2020 20:19
BillWagner added a commit that referenced this pull request Oct 14, 2020
* Respond to feedback for deserialization constructors (#21051)

* Fixes #20949. (#21052)

* Add f1-keyword for `class` used as generic type constraint (#21037)

* Add f1-keyword for `class` used as generic type constraint

* Add structconstraint

* Update CS0267 message and simplify example (#21047)

* Update CS0267 message and simplify example

* Update ms.date

* Added a link (#21058)

* Fixes #20958. (#21054)

* fix the source path for retired C# 7.x content (#21059)

The "whats-new" folder was missing.

* Removing redundant pipe character (#20931)

* Update single-file.md (#20925)

update ReadyToRun link now that we have the docs for it.

* Remove statement about TLS 1.3 for .NET Core 3.0 (#20922)

I think, TLS 1.3 won't be supported by .NET Core 3.0 since it's out of support and the implementation is coming with .NET 5, as stated in [Transport Layer Security (TLS) best practices with the .NET Framework](#4675 (comment)).

This info should be removed, so we have no misleading information out there... 😉

* SslStream/NegotiateStream breaking change (#20979)

* Add workload and app model to glossary (#21046)

Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>

* Breaking change for key-value pair serialization (#21050)

* Update .NET Standard overview for .NET 5 (#20955)

Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>

* Update get-started-vscode.md (#21072)

http to https after verifying link

Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>
Co-authored-by: Jon Senchyna <TheSench@users.noreply.github.com>
Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: Bill Wagner <wiwagn@microsoft.com>
Co-authored-by: William Whitwell Bailey <45574209+willwhitwellbailey@users.noreply.github.com>
Co-authored-by: Lakshan Fernando <lakshanf@hotmail.com>
Co-authored-by: kapsiR <kapsiR@users.noreply.github.com>
Co-authored-by: Tom Dykstra <tdykstra@microsoft.com>
Co-authored-by: icyfire0573 <40814526+icyfire0573@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NegotiateStream and SslStream will no longer fail on multiple Begin operations without End

4 participants