Improve zero-byte read handling in SslStream to reduce memory usage #49123

geoffkizer · 2021-03-04T09:48:25Z

When a user issues a zero-byte read against a Stream, they are indicating that they want to be notified when data is available, without having to hold on to a read buffer to perform an actual read. In other words, they are willing to trade off some added CPU cost for reduced memory usage.

This PR improves the zero-byte handling in SslStream so that, when the user performs a zero-byte read and we have no data available, we will perform a zero-byte read against the underlying stream before allocating a buffer for the read. This should hopefully reduce memory usage in cases where the user does zero-byte reads and the connection is idle for periods of time, e.g. idle WebSocket or HTTP connections.

Also, add a test to the stream conformance tests that validates this behavior for arbitrary wrapping streams. This is behind a flag and I've only enabled the flag for SslStream for now, since as far as I know, no other Stream implements similar behavior (though arguably they should).

Also, ensure we are never holding on to an empty read buffer in SslStream -- there were some exception cases where this was happening currently. And some minor SslStream cleanup.

Fixes #49019

cc @wfurt @scalablecory @stephentoub @davidfowl

… to stream conformance tests

…d on underlying stream when appropriate

ghost · 2021-03-04T09:48:29Z

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

Issue Details

When a user issues a zero-byte read against a Stream, they are indicating that they want to be notified when data is available, without having to hold on to a read buffer to perform an actual read. In other words, they are willing to trade off some added CPU cost for reduced memory usage.

This PR improves the zero-byte handling in SslStream so that, when the user performs a zero-byte read and we have no data available, we will perform a zero-byte read against the underlying stream before allocating a buffer for the read. This should hopefully reduce memory usage in cases where the user does zero-byte reads and the connection is idle for periods of time, e.g. idle WebSocket or HTTP connections.

Also, add a test to the stream conformance tests that validates this behavior for arbitrary wrapping streams. This is behind a flag and I've only enabled the flag for SslStream for now, since as far as I know, no other Stream implements similar behavior (though arguably they should).

Also, ensure we are never holding on to an empty read buffer in SslStream -- there were some exception cases where this was happening currently. And some minor SslStream cleanup.

Fixes #49019

cc @wfurt @scalablecory @stephentoub @davidfowl

Author:	geoffkizer
Assignees:	-
Labels:	`area-System.Net.Security`
Milestone:	6.0.0

geoffkizer · 2021-03-04T09:49:29Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-03-04T09:49:42Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2021-03-04T10:38:03Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-03-04T10:38:16Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2021-03-04T11:46:49Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-03-04T11:47:04Z

Azure Pipelines successfully started running 1 pipeline(s).

stephentoub

src changes LGTM. Thanks.

wfurt

LGTM.
build broken by this change so we should hold back merge until green CI.

geoffkizer · 2021-03-05T20:16:00Z

Yeah, I've broken a bunch of test projects that use the conformance tests. Working on a fix.

… test projects

geoffkizer · 2021-03-06T04:20:19Z

I pushed a change to encapsulate the StreamConformanceTests in their own assembly. This means that test projects that use these tests can simply reference the assembly, instead of having to include the code manually. And it means that future updates to the StreamConformanceTests will not require updating every test project that uses them.

@stephentoub Can you take a look?

geoffkizer · 2021-03-06T04:20:56Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-03-06T04:21:09Z

Azure Pipelines successfully started running 1 pipeline(s).

stephentoub · 2021-03-06T11:27:40Z

And it means that future updates to the StreamConformanceTests will not require updating every test project that uses them.

If you add a new file you mean, right?

stephentoub · 2021-03-06T11:28:52Z

src/libraries/Common/tests/StreamConformanceTests/StreamConformanceTests.csproj

+    <Compile Include="$(CommonPath)System\Threading\Tasks\TaskToApm.cs" Link="System\Threading\Tasks\TaskToApm.cs" />
+    <Compile Include="$(CommonPath)System\Net\ArrayBuffer.cs" Link="Common\System\Net\ArrayBuffer.cs" />
+    <Compile Include="$(CommonPath)System\Net\MultiArrayBuffer.cs" Link="Common\System\Net\MultiArrayBuffer.cs" />
+    <Compile Include="$(CommonPath)System\Net\StreamBuffer.cs" Link="Common\System\Net\StreamBuffer.cs" />
+    <Compile Include="$(CommonPath)System\IO\DelegatingStream.cs" Link="Common\System\IO\DelegatingStream.cs" />
+    <Compile Include="$(CommonTestPath)System\IO\CallTrackingStream.cs" Link="Common\System\IO\CallTrackingStream.cs" />


Should most of these files be in TestUtilities instead?

(edit to explain in more detail)

The ones in CommonPath are shared source files (not test files) and thus have to be internal, and can't be in TestUtilities.

CallTrackingStream is a shared test code, so we have more flexibility there. We could move it to TestUtilities, but it wasn't there before so I didn't. Right now it's only used in the stream conformance tests. so another possibility would be to move it out of shared test code and into the stream conformance tests proper. I don't have a strong opinion here.

stephentoub · 2021-03-06T11:31:19Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

@@ -759,6 +759,8 @@ private async ValueTask<int> ReadAsyncInternal<TIOAdapter>(TIOAdapter adapter, M
                throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, nameof(SslStream.ReadAsync), "read"));
            }

+            Debug.Assert(_internalBuffer is null || _internalBufferCount > 0 || _decryptedBytesCount > 0, "_internalBuffer allocated when no data is buffered.");


What's the reason for the last part of the condition around _decryptedBytesCount > 0?

It's because there can be two different kinds of data in the _internalBuffer -- encrypted or decrypted -- and the two checks here test for each.

The field names aren't ideal; _internalBufferCount should probably be something like _encryptedBytesCount.

Note that ReturnReadBufferIfEmpty (just above) does the same check before returning the unused buffer to the pool.

stephentoub

As noted offline, it's strange to me that we now have two different ways of including shared tests: some suites putting them in assemblies (streams), and some suites putting them in files pulled into each test project (collections). If we're moving the former to use a shared assembly, I'd like us to do the same for the others as well. I don't remember if there's more than just collections.

geoffkizer · 2021-03-07T20:54:48Z

it's strange to me that we now have two different ways of including shared tests

My opinion is that assemblies are much easier to deal with -- you just need to reference the assembly from the test project. I don't see any downside to using assemblies, either. So I'd suggest we simply move to that in general.

geoffkizer · 2021-03-07T21:02:10Z

In fact, in general I don't think there's a good reason to ever share test code on a file-by-file basis. (This gets to your question above re TestUtilities.)

General purpose test helper code can go in TestUtilities.

Area-specific shared test code -- either stuff like full-on test suites like StreamConformanceTests, or just area-specific helper classes -- should go in some sort of area-specific shared assembly as appropriate.

I don't plan on actively reorganizing any test code, but it's something to consider moving forward as we make changes.

Geoffrey Kizer added 4 commits March 3, 2021 23:08

add ZeroByteRead_PerformsZeroByteReadOnUnderlyingStreamWhenDataNeeded…

ddf7755

… to stream conformance tests

remove dead code

b785128

change SslStream zero byte read implementation to issue zero-byte rea…

fa44d27

…d on underlying stream when appropriate

ensure buffer is always freed when not needed, and assert this

ee157db

geoffkizer added the area-System.Net.Security label Mar 4, 2021

geoffkizer added this to the 6.0.0 milestone Mar 4, 2021

stephentoub approved these changes Mar 4, 2021

View reviewed changes

wfurt approved these changes Mar 4, 2021

View reviewed changes

create StreamConformanceTests assembly and reference from appropriate…

529700c

… test projects

geoffkizer force-pushed the zerobyte branch from 36f42ab to 529700c Compare March 6, 2021 04:07

minor cleanup

416d1e7

stephentoub reviewed Mar 6, 2021

View reviewed changes

stephentoub approved these changes Mar 6, 2021

View reviewed changes

geoffkizer merged commit 027c093 into dotnet:main Mar 7, 2021

stephentoub mentioned this pull request Mar 8, 2021

FileStream rewrite part II #48813

Merged

ghost locked as resolved and limited conversation to collaborators Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve zero-byte read handling in SslStream to reduce memory usage #49123

Improve zero-byte read handling in SslStream to reduce memory usage #49123

geoffkizer commented Mar 4, 2021

ghost commented Mar 4, 2021

geoffkizer commented Mar 4, 2021

azure-pipelines bot commented Mar 4, 2021

geoffkizer commented Mar 4, 2021

azure-pipelines bot commented Mar 4, 2021

geoffkizer commented Mar 4, 2021

azure-pipelines bot commented Mar 4, 2021

stephentoub left a comment

wfurt left a comment

geoffkizer commented Mar 5, 2021

geoffkizer commented Mar 6, 2021 •

edited

Loading

geoffkizer commented Mar 6, 2021

azure-pipelines bot commented Mar 6, 2021

stephentoub commented Mar 6, 2021

stephentoub Mar 6, 2021

geoffkizer Mar 7, 2021 •

edited

Loading

stephentoub Mar 6, 2021

geoffkizer Mar 7, 2021

stephentoub left a comment

geoffkizer commented Mar 7, 2021

geoffkizer commented Mar 7, 2021

Improve zero-byte read handling in SslStream to reduce memory usage #49123

Improve zero-byte read handling in SslStream to reduce memory usage #49123

Conversation

geoffkizer commented Mar 4, 2021

ghost commented Mar 4, 2021

geoffkizer commented Mar 4, 2021

azure-pipelines bot commented Mar 4, 2021

geoffkizer commented Mar 4, 2021

azure-pipelines bot commented Mar 4, 2021

geoffkizer commented Mar 4, 2021

azure-pipelines bot commented Mar 4, 2021

stephentoub left a comment

Choose a reason for hiding this comment

wfurt left a comment

Choose a reason for hiding this comment

geoffkizer commented Mar 5, 2021

geoffkizer commented Mar 6, 2021 • edited Loading

geoffkizer commented Mar 6, 2021

azure-pipelines bot commented Mar 6, 2021

stephentoub commented Mar 6, 2021

stephentoub Mar 6, 2021

Choose a reason for hiding this comment

geoffkizer Mar 7, 2021 • edited Loading

Choose a reason for hiding this comment

stephentoub Mar 6, 2021

Choose a reason for hiding this comment

geoffkizer Mar 7, 2021

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

geoffkizer commented Mar 7, 2021

geoffkizer commented Mar 7, 2021

geoffkizer commented Mar 6, 2021 •

edited

Loading

geoffkizer Mar 7, 2021 •

edited

Loading