Skip to content

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented May 13, 2021

This test helps with verification when IO does not land on frame a boundary or we get less than expected bytes.
This was originally part of #49743.

@wfurt wfurt added area-System.Net.Security test-enhancement Improvements of test source code labels May 13, 2021
@wfurt wfurt self-assigned this May 13, 2021
@ghost
Copy link

ghost commented May 13, 2021

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

Issue Details

This test helps with verification when IO does not land on frame a boundary or we get less than expected bytes.
This was originally part of #49743.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security, test enhancement

Milestone: -

@geoffkizer
Copy link
Contributor

Should this test be part of the Stream Conformance tests? It doesn't seem like it's specific to SslStream.

I do think it's a really useful test to have, that's why I kind of want to put it in Stream Conformance....

@stephentoub

@wfurt
Copy link
Member Author

wfurt commented May 13, 2021

Most streams (like NetworkStream or FileStream) do not have any internal structure and it is really opaque pipe. SslStream is different as it maintains state around messages of various types.
During the refactor, I created bug that only showed up as failure in linker test doing GET to www.microsoft.com. All the existing SslStream and Http tests were passing fine so I felt there is test coverage gap.
So I feel it is valuable to force short IOs for SslStream to cover all the code variants but value for other streams may be low.

@geoffkizer
Copy link
Contributor

Most streams (like NetworkStream or FileStream) do not have any internal structure and it is really opaque pipe. SslStream is different as it maintains state around messages of various types.

I think there are other streams that have internal structure, like HTTP chunked encoding or crypto, that could benefit from this. Basically the wrapping streams (of which SslStream is one).

public async Task SslStream_RandomSizeWrites_OK(int bufferSize, int readBufferSize, int writeBufferSize, bool useAsync)
{
byte[] dataToCopy = RandomNumberGenerator.GetBytes(bufferSize);
byte[] dataReceived = new byte[dataToCopy.Length + readBufferSize]; // make the buffer bigger to have chance to read more
Copy link
Contributor

Choose a reason for hiding this comment

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

Is readBufferSize here significant? Could we just do dataToCopy.Length + 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

somewhat. It makes readBuffer.Slice(totalLength, readBufferSize) to always work without worrying about how much is left.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, seems reasonable, but a comment would explain that :)

@geoffkizer
Copy link
Contributor

In general this is in good shape, a couple nits above.

@wfurt wfurt merged commit f96ce3a into dotnet:main May 21, 2021
@wfurt wfurt deleted the randomIO branch May 21, 2021 10:01
@ghost ghost locked as resolved and limited conversation to collaborators Jun 20, 2021
@karelz karelz added this to the 6.0.0 milestone Jul 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Security test-enhancement Improvements of test source code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants