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

attempt to stabilize CopyToAsync_AllDataCopied_Large for Tls13 #44741

Merged
merged 7 commits into from
Nov 30, 2020

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Nov 16, 2020

I'm not 100% what is going on but with this change I did several hundreds runs and I did not see any failure.
The TLS1.3 are not quite symmetric as 1.2.

@ghost
Copy link

ghost commented Nov 16, 2020

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

Issue Details
Description:

I'm not 100% what is going on but with this change I did several hundreds runs and I did not see any failure.
The TLS1.3 are not quite symmetric as 1.2.

Author: wfurt
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@wfurt
Copy link
Member Author

wfurt commented Nov 16, 2020

/azp run runtime-libraries-coreclr outerloop-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@geoffkizer
Copy link
Contributor

Why does this make it better? Seems like it shouldn't matter.

@wfurt
Copy link
Member Author

wfurt commented Nov 16, 2020

I'm still digging into it. But here are some (unverified) thoughts. For TLS 1.2 (and less) when handshake is done the streams are really identical and fully interchangeable. With TLS1.3 part of the handshake piggy-backs on first data chunks - that was done to limit round trips and that is reason why we always hit the "RENEGOTIATION" path on Windows. The test sends unidirectional data and and I'm not sure if we have mechanism to send message unless somebody call encrypt data.

Now the interesting part:
The session fails because the writer sends RST. Since the NetworkStream owns the socket, it should call shutdown on dispose and I would expect that would still do normal TCP close using FIN.

image

@geoffkizer
Copy link
Contributor

Yeah, very weird about the RST.

@wfurt
Copy link
Member Author

wfurt commented Nov 16, 2020

I think it may be timing after all @geoffkizer . I opened #44764 to track what I see as underlying problem.

@wfurt wfurt closed this Nov 16, 2020
@wfurt wfurt marked this pull request as ready for review November 20, 2020 18:12
@wfurt wfurt requested a review from a team November 20, 2020 18:12
@wfurt wfurt self-assigned this Nov 20, 2020
@wfurt wfurt added the test-bug Problem in test source code (most likely) label Nov 20, 2020
@wfurt
Copy link
Member Author

wfurt commented Nov 20, 2020

full details are inside #44764. This change should be sufficient to make the test reliable. Alternatively, we would need to change the test to read extra TLS frame(s) from the buffer before Disposing writer.

fixes #44175

@wfurt
Copy link
Member Author

wfurt commented Nov 20, 2020

/azp run runtime-libraries-coreclr outerloop-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member Author

wfurt commented Nov 23, 2020

/azp run runtime-libraries-coreclr outerloop-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member Author

wfurt commented Nov 25, 2020

/azp run runtime-libraries-coreclr outerloop-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member Author

wfurt commented Nov 25, 2020

test failures are unrelated.

@wfurt wfurt merged commit 059e6bb into dotnet:master Nov 30, 2020
@wfurt wfurt deleted the tls13_44175 branch November 30, 2020 17:23
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2020
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security test-bug Problem in test source code (most likely)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants