SslStream - Linux Solving it all in 4 lines (and deleting 2) by Drawaes · Pull Request #25646 · dotnet/corefx

Drawaes · 2017-12-01T23:32:13Z

Well, I sat, I thought, ... I read some c.. and I think I figured it out..

Basically the problem is if another bit of code that used this thread didn't clean up after itself (bad manners if you ask me but you can't control everyone).

The errors are just a queue and we can "peek_last_error" but the problem is that we are calling Ssl_Get_Error.

Ssl_Get_Error is a about 100 lines of code that does a bunch of other things than just check the error queue (checks internal bio states for async needs read/write etc).

So we can't change that, and we can't change that it needs the error code of the latest error. And we can't change that it looks at the "first" item in the queue.

The current solution is to clear the error queue before each encrypt/decrypt but that hits the evil global lock and causes all the grief. And removing the clear of course causes potentially the wrong item at the top of the error queue.

However the clear is on the hot path, and what isn't on the hot path is the "Ssl_Get_Error" mostly because SslStream ensures sending of complete frames to OpenSSL so we don't rely on the "Needs Read/Write" in hot situations. We will get these for an actual error, but then that is now a slow path anyway.

To get to it, my solution is not to do the clear, but when we are in the error state, just makesure the queues first and last error number are the same with a Peek of the first and last (the only operations I have other than get). If they aren't the same then do a get to pop one off the queue and repeat until they equal.

Master ~200k rps
Throttle ~400k rps
Throttle with ProcCount/2 ~580k rps
With my error change

713021.51

This is my preferred solution over #25187

Drawaes · 2017-12-01T23:33:32Z

@benaadams @bartonjs @stephentoub

Drawaes · 2017-12-01T23:33:51Z

The best bit, no throttle, no PAL crazy, so so simple.

benaadams · 2017-12-01T23:40:01Z


 extern "C" int32_t CryptoNative_SslGetError(SSL* ssl, int32_t ret)
 {
+    while(ERR_peek_error() != ERR_peek_last_error())


I think this needs some code comments explaining what is happening (and why)

Good idea, added

benaadams · 2017-12-01T23:55:14Z

@dotnet-bot test Outerloop Linux x64 Debug Build please
@dotnet-bot test Outerloop Linux x64 Release Build please

stephentoub · 2017-12-02T00:30:26Z

If @bartonjs is happy with it, I am, too.

benaadams · 2017-12-02T00:46:55Z

RedHat seg fault in regular release

Output: Discovering: System.Net.Http.Functional.Tests
Output: Discovered:  System.Net.Http.Functional.Tests
Output: Starting:    System.Net.Http.Functional.Tests
Output: /home/helixbot/dotnetbuild/work/bfe57023-f46d-466f-95f0-add96a0cd0a9/Work/35f9d9c3-7fe6-43ee-9f89-bbfe45adfa8b/Unzip/RunTests.sh: 
 line 91:   869 Segmentation fault      (core dumped) $RUNTIME_PATH/dotnet xunit.console.netcore.exe System.Net.Http.Functional.Tests.dll 
 -xml testResults.xml -notrait category=nonnetcoreapptests -notrait category=nonlinuxtests -notrait category=failing
Output: Trying to find crash dumps for project: System.Net.Http.Functional.Tests
Output: No new dump file was found in /home/helixbot/dotnetbuild/work/bfe57023-f46d-466f-95f0-add96a0cd0a9/Work/35f9d9c3-7fe6-43ee-9f89-bbfe45adfa8b/Unzip
Output: ~/dotnetbuild/work/bfe57023-f46d-466f-95f0-add96a0cd0a9/Work/35f9d9c3-7fe6-43ee-9f89-bbfe45adfa8b/Unzip
Output: Finished running tests. End time=16:39:39. Return value was 139
Exit Code: 139
ERROR: scriptrunner(87): _main: Error: No exception thrown, but XUnit results not created

@dotnet-bot test Linux x64 Release Build please

benaadams · 2017-12-02T00:59:50Z

fedora.25.amd64.Open:Debug-x64 Outerloop issue, same place

Output: Discovering: System.Net.Http.Functional.Tests
Output: Discovered:  System.Net.Http.Functional.Tests
Output: Starting:    System.Net.Http.Functional.Tests
Output: /home/helixbot/dotnetbuild/work/bfe57023-f46d-466f-95f0-add96a0cd0a9/Work/35f9d9c3-7fe6-43ee-9f89-bbfe45adfa8b/Unzip/RunTests.sh:
 line 91:   869 Segmentation fault      (core dumped) $RUNTIME_PATH/dotnet xunit.console.netcore.exe System.Net.Http.Functional.Tests.dll
 -xml testResults.xml -notrait category=nonnetcoreapptests -notrait category=nonlinuxtests -notrait category=failing
Output: Trying to find crash dumps for project: System.Net.Http.Functional.Tests
Output: No new dump file was found in /home/helixbot/dotnetbuild/work/bfe57023-f46d-466f-95f0-add96a0cd0a9/Work/35f9d9c3-7fe6-43ee-9f89-bbfe45adfa8b/Unzip
Output: ~/dotnetbuild/work/bfe57023-f46d-466f-95f0-add96a0cd0a9/Work/35f9d9c3-7fe6-43ee-9f89-bbfe45adfa8b/Unzip
Output: Finished running tests. End time=16:39:39. Return value was 139
Exit Code: 139
ERROR: scriptrunner(87): _main: Error: No exception thrown, but XUnit results not created

bartonjs

I'm having trouble deciding if this is bad or not. I can't immediately think of a reason why it would be bad, but it's not obviously not-bad, either.

I guess you can always revert it when people start reporting nonsense errors.

bartonjs · 2017-12-02T00:56:32Z

+    // This pops off "old" errors left by other operations
+    // until the first and last error are the same
+    // this should be looked at again when OpenSsl 1.1 is migrated to
+    while(ERR_peek_error() != ERR_peek_last_error())


Please run clang-format -i on this file. (At least accepting any changes in this function)

bartonjs · 2017-12-02T00:59:29Z

+    // This pops off "old" errors left by other operations
+    // until the first and last error are the same
+    // this should be looked at again when OpenSsl 1.1 is migrated to
+    while(ERR_peek_error() != ERR_peek_last_error())


Technically this will stop if there's a repeat value. And the error could be stale.

Yeah the stale is an issue, let me try a couple of things and see the perf.

…ring Ssl_GetError

Drawaes · 2017-12-02T02:13:22Z

@dotnet-bot test Linux x64 Release Build please

Drawaes · 2017-12-02T02:41:47Z

@dotnet-bot test Outerloop Linux x64 Debug Build please
@dotnet-bot test Outerloop Linux x64 Release Build please

benaadams · 2017-12-02T04:53:24Z

@dotnet-bot test Outerloop Linux x64 Debug Build please
@dotnet-bot test Outerloop Linux x64 Release Build please

bartonjs · 2017-12-02T06:10:25Z

+    // This pops off "old" errors left by other operations
+    // until the first and last error are the same
+    // this should be looked at again when OpenSsl 1.1 is migrated to
+    while(ERR_peek_error() != ERR_peek_last_error())


This should be while<space>(. Please run clang-format -i against this file.

Drawaes · 2017-12-02T10:50:29Z

@dotnet-bot test Outerloop Linux x64 Debug Build please
@dotnet-bot test Outerloop Linux x64 Release Build please

stephentoub

As long as @bartonjs says it's sound, this is certainly a better approach than the manual throttling.

…otnet#25646)" This reverts commit 03c7617.

pjanotti · 2018-04-19T16:43:08Z

@Drawaes what did you use in your benchmark? I would like to keep tracking the perf status of this while we do some more work to ensure no errors are left on the queue.

Drawaes · 2018-04-19T17:12:08Z

The aspnetcore plaintext test from the asp team. They have it in docker now

…

On Thu, 19 Apr 2018, 17:43 Paulo Janotti, ***@***.***> wrote: @Drawaes <https://github.com/Drawaes> what did you use in your benchmark? I would like to keep tracking the perf status of this while we do some more work to ensure no errors are left on the queue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APpZuYo5qgaAGJIgGvVsQVBkLxvTcrRAks5tqL69gaJpZM4QzDIq> .

…corefx#25646) * Change get error to remove all but the last. * Remove clear errors * Added code comment * Try peeking at the errors before clearing * Make sure the error queue is cleared if there were multiple errors during Ssl_GetError * Format with clang Commit migrated from dotnet/corefx@03c7617

Drawaes added 2 commits December 1, 2017 22:25

Change get error to remove all but the last.

b143586

Remove clear errors

cbc638b

benaadams reviewed Dec 1, 2017

View reviewed changes

Added code comment

b6394f5

benaadams approved these changes Dec 1, 2017

View reviewed changes

bartonjs reviewed Dec 2, 2017

View reviewed changes

Drawaes changed the title ~~SslStream - Linux Solving it all in 4 lines (and deleting 2)~~ [WIP] SslStream - Linux Solving it all in 4 lines (and deleting 2) Dec 2, 2017

Drawaes added 2 commits December 2, 2017 01:46

Try peeking at the errors before clearing

f6a7e81

Make sure the error queue is cleared if there were multiple errors du…

ec0c6df

…ring Ssl_GetError

bartonjs reviewed Dec 2, 2017

View reviewed changes

Drawaes changed the title ~~[WIP] SslStream - Linux Solving it all in 4 lines (and deleting 2)~~ SslStream - Linux Solving it all in 4 lines (and deleting 2) Dec 2, 2017

Format with clang

5189c51

stephentoub approved these changes Dec 2, 2017

View reviewed changes

bartonjs approved these changes Dec 2, 2017

View reviewed changes

stephentoub merged commit 03c7617 into dotnet:master Dec 2, 2017

stephentoub mentioned this pull request Dec 2, 2017

[WIP] SslStream Throttle #25187

Closed

Drawaes deleted the TestSkipDeleteErrors branch December 2, 2017 21:00

karelz added this to the 2.1.0 milestone Dec 4, 2017

Priya91 pushed a commit to Priya91/corefx-1 that referenced this pull request Dec 22, 2017

Revert "SslStream - Linux Solving it all in 4 lines (and deleting 2) (d…

348bcc8

…otnet#25646)" This reverts commit 03c7617.

stephentoub mentioned this pull request Jan 31, 2020

[Regression] Test failed: System.Net.Http.Functional.Tests.HttpClientHandler_DangerousAcceptAllCertificatesValidator_Test / SetDelegate_ConnectionSucceeds dotnet/runtime#24335

Closed

pjanotti mentioned this pull request Jan 31, 2020

Add config to force OpenSsl error queue cleanup before Encrypt/Decrypt dotnet/runtime#25930

Closed

Uh oh!

Conversation

Drawaes commented Dec 1, 2017

Uh oh!

Drawaes commented Dec 1, 2017

Uh oh!

Drawaes commented Dec 1, 2017

Uh oh!

benaadams Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

Drawaes Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

benaadams commented Dec 1, 2017

Uh oh!

stephentoub commented Dec 2, 2017

Uh oh!

benaadams commented Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Dec 2, 2017

Uh oh!

bartonjs left a comment

Choose a reason for hiding this comment

Uh oh!

bartonjs Dec 2, 2017

Choose a reason for hiding this comment

Uh oh!

bartonjs Dec 2, 2017

Choose a reason for hiding this comment

Uh oh!

Drawaes Dec 2, 2017

Choose a reason for hiding this comment

Uh oh!

Drawaes commented Dec 2, 2017

Uh oh!

Drawaes commented Dec 2, 2017

Uh oh!

benaadams commented Dec 2, 2017

Uh oh!

bartonjs Dec 2, 2017

Choose a reason for hiding this comment

Uh oh!

Drawaes Dec 2, 2017

Choose a reason for hiding this comment

Uh oh!

Drawaes commented Dec 2, 2017

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

pjanotti commented Apr 19, 2018

Uh oh!

Drawaes commented Apr 19, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

benaadams commented Dec 2, 2017 •

edited

Loading