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.

Sign up for GitHub

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

Jump to bottom

Merge several version of MsQuicStream SendAsync code #68772

Merged
merged 8 commits into from
May 24, 2022
Merged

Merge several version of MsQuicStream SendAsync code #68772

merged 8 commits into from
May 24, 2022

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented May 2, 2022 •
edited
Loading

Fixes #55818.

This PR extracts common code from several Send*Async methods used to implement WriteAsync into a single WriteAsyncInternal method.

@ghost ghost assigned rzikm May 2, 2022
@ghost
Copy link

ghost commented May 2, 2022

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

Issue Details

Fixes #55818.

This PR extracts common code from several Send*Async methods

Author: rzikm
Assignees: -
Labels:

area-System.Net.Quic
Milestone: -

@runfoapp runfoapp bot mentioned this pull request May 2, 2022
Closed
@rzikm
Copy link
Member Author

rzikm commented May 3, 2022

Test failures seem to be all some permission errors, no actual test failures as far as I can see

@rzikm
Copy link
Member Author

rzikm commented May 3, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm rzikm requested review from CarnaViire and ManickaP May 3, 2022 13:50
CarnaViire
CarnaViire reviewed May 3, 2022
View reviewed changes
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs Outdated
{
if (state.SendBufferMaxCount < count)
{
NativeMemory.Free((void*)state.SendQuicBuffers);
Copy link
Member

Choose a reason for hiding this comment

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

I haven't looked much into this yet, but it feels like it might be better to wait for #68288 to go in to be able to reuse stuff like MsQuicBuffers that was added there?

Copy link
Member Author

@rzikm rzikm May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That PR does not use them for buffers inside MsQuicStream, and based on one of the comments it would be something for a follow-up PR. I probably don't mind much putting this on hold for a bit until that PR is merged, but that could take a while more.

Thoughts, @ManickaP?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please wait with this on my PR.

@rzikm
Copy link
Member Author

rzikm commented May 3, 2022

Test failure is unrelated (System.Net.Primitives.UnitTests.Tests)

@rzikm rzikm marked this pull request as draft May 3, 2022 16:47
@rzikm
Copy link
Member Author

rzikm commented May 3, 2022

Putting this on hold until #68288 is in.

@rzikm rzikm marked this pull request as ready for review May 5, 2022 12:55
@rzikm
Copy link
Member Author

rzikm commented May 5, 2022

I reused the MsQuicBuffers struct @ManickaP added, but I think I still need to keep the adaptor structs, unless we are fine with replacing it with lambdas. I briefly tried managed function pointers, but they can't be used in non-unsafe context, and you can't await in unsafe context....

@rzikm rzikm requested a review from CarnaViire May 5, 2022 12:57
@rzikm
Copy link
Member Author

rzikm commented May 6, 2022

Since #68288 got reverted in #68940, we need to put this PR on hold

@rzikm rzikm marked this pull request as draft May 6, 2022 07:04
@rzikm rzikm force-pushed the 55818-sendasync-refactor branch from fcf5b8c to 491ce39 Compare May 12, 2022 13:53
@rzikm rzikm marked this pull request as ready for review May 12, 2022 13:53
@rzikm
Copy link
Member Author

rzikm commented May 12, 2022

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented May 13, 2022

Test failures are unrelated:

  • System.Text.RegularExpressions.Tests.RegexIgnoreCaseTests.Unicode_IgnoreCase_Tests
  • LibraryImportGenerator.UnitTests.Compiles.ValidateSnippetsFallbackForwarder

ManickaP
ManickaP reviewed May 18, 2022
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

In general looks good, thanks.

...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs Outdated Show resolved Hide resolved
...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs Outdated Show resolved Hide resolved
...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs Outdated Show resolved Hide resolved
...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs Show resolved Hide resolved
...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs Outdated Show resolved Hide resolved
@rzikm rzikm mentioned this pull request May 18, 2022
Closed
ManickaP
ManickaP approved these changes May 23, 2022
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs Outdated Show resolved Hide resolved
CarnaViire
CarnaViire approved these changes May 23, 2022
View reviewed changes
Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

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

LGTM modulo nits, thanks!

...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs Outdated
{
int count = buffers.Length;
Reserve(count);
_count = count;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I believe it is already part of Reserve(count)

Suggested change
_count = count;

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs
@@ -85,15 +81,19 @@ private sealed class State
// Set once stream have been shutdown.
public readonly TaskCompletionSource ShutdownCompletionSource = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);

public State()
{
SendBuffers = new MsQuicBuffers();
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason it should be in a constructor and not assigned on declaration as other fields?

Copy link
Member Author

Choose a reason for hiding this comment

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

Putting = new MsQuicBuffers() in the field initializer triggers "Don't assign default values" or similar warning, and leaving it uninitialized breaks because the MsQuicBuffers._handles is null.

@rzikm rzikm merged commit fff4ff0 into dotnet:main May 24, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 23, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ManickaP ManickaP ManickaP approved these changes

@CarnaViire CarnaViire CarnaViire approved these changes

Assignees

@rzikm rzikm

Labels
area-System.Net.Quic
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

[QUIC] Merge several version of stream SendAsync
4 participants
@rzikm @CarnaViire @ManickaP @karelz