Use shared SocketAddress code in QUIC#66794

rzikm · 2022-03-17T21:21:52Z

This PR replaces duplicated code for composing OS-level SOCKADDR_INET structure by preexisting code in SocketAddress class.

Fixes #32068

ghost · 2022-03-18T20:01:00Z

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

Issue Details

This PR replaces duplicated code for composing OS-level SOCKADDR_INET structure by preexisting code in SocketAddress class.

Fixes #32068

Author:	rzikm
Assignees:	rzikm
Labels:	`area-System.Net`
Milestone:	-

ghost · 2022-03-21T08:30:44Z

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

Issue Details

This PR replaces duplicated code for composing OS-level SOCKADDR_INET structure by preexisting code in SocketAddress class.

Fixes #32068

Author:	rzikm
Assignees:	rzikm
Labels:	`area-System.Net`, `area-System.Net.Quic`
Milestone:	-

ManickaP

I gave a cursory look at the internal SocketAddress and I think we should invest in some optimization there and here together.
I might be looking wrongly, but it looks like this will introduce more copies of address bytes as well as more interop calls per each conversion (not necessarily meaning more allocations, though).

ManickaP · 2022-03-21T12:50:20Z

...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs

+            internal uint ListenerStart(SafeMsQuicListenerHandle listener, QuicBuffer* alpnBuffers, uint alpnBufferCount, byte* localAddress)
            {
-                IntPtr __listener_gen_native = default;
+                IntPtr __listener_gen_native;


Why this line change?

Compiler complained that the default-assigned value was never used

We still have it like that in other places since this is source generated code, so could we leave it as it was? Unless it breaks the build due to warnaserror or something like that.

it breaks the build otherwise

Let's keep it then. It's still strange that the other methods or the previous version of this one didn't break the build.

...ystem.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicParameterHelpers.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs

ManickaP · 2022-03-21T13:09:56Z

...ystem.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicParameterHelpers.cs

+            value.Buffer.AsSpan(0, value.Size).CopyTo(address);
+            address.Slice(value.Size).Clear();
+
+            fixed (byte* paddress = &MemoryMarshal.GetReference(address))


Do we need pinning if the the source is stack allocated? I don't know the answer here, just asking. Or what does fixed here exactly do?

It doesn't really do anything here, but the language requires you to use a fixed statement when "taking the address of an unfixed expression" like this.

Alternatively, it could be written as this (not saying it should be):

byte* pAddress = stackalloc byte[Internals.SocketAddress.IPv6AddressSize]; var address = new Span<byte>(pAddress, Internals.SocketAddress.IPv6AddressSize);

wfurt

I'm wondering if this actually works with current quic e.g. 1.9. QUIC_ADDRESS_FAMILY.INET6 is same for all platforms but the new code will actually use the platform value AFAIK.

src/libraries/System.Net.Quic/src/Resources/Strings.resx

rzikm · 2022-03-22T18:38:35Z

I'm wondering if this actually works with current quic e.g. 1.9. QUIC_ADDRESS_FAMILY.INET6 is same for all platforms but the new code will actually use the platform value AFAIK.

I thought that 1.9 is the one currently running as part of our CI, is it not, @ManickaP?

I manually downloaded 1.9 for local testing purposes and it worked on windows.

ManickaP · 2022-03-23T10:14:23Z

QUIC_ADDRESS_FAMILY.INET6 is same for all platforms but the new code will actually use the platform value AFAIK

Nice catch, SocketAddress is using platform dependent values for INET = 2 and INET6 = 10 (on Linux) whereas QUIC_ADDRESS_FAMILY is using predefined constants: INET = 2 and INET6 = 23, which correspond to Windows values.
You should see it blow up if you try to use IPv6 on Linux with msquic 1.9.

I'm wondering whether this means that we don't have any coverage for IPv6 and we should probably add it.

rzikm · 2022-03-23T10:40:30Z

QUIC_ADDRESS_FAMILY is using predefined constants: INET = 2 and INET6 = 23, which correspond to Windows values.

But we should be fine once we upgrade MsQuic, right? since latest MsQuic moved to platform-specific values again.

ManickaP · 2022-03-23T15:29:54Z

But we should be fine once we upgrade MsQuic, right? since latest MsQuic moved to platform-specific values again.

yes

ManickaP · 2022-03-28T20:25:59Z

So to push this to a conclusion. Can we add tests for this:

I'm wondering whether this means that we don't have any coverage for IPv6 and we should probably add it.

If this change is expected to break for IPv6 and didn't actually blow anything up:

Then, an issue for this needs to be filed, against which the new test will be disabled. And finally, we'll resolve the issue with msquic 2.0 update (I already have some WIP so it shouldn't take much longer).

Does it sound reasonable @rzikm ?

…dress

rzikm · 2022-03-29T20:09:36Z

I disabled tests broken by this change and filed #67301 for tracking broken IPv6 on Linux (to be closed after upgrade to MsQuic 2.0).

That should be enough to move this forward, right? @ManickaP?

ManickaP

Thanks!

…dress

Use shared SocketAddress code in QUIC

ac6201e

ghost assigned rzikm Mar 17, 2022

maryamariyan added the area-System.Net label Mar 18, 2022

rzikm added the area-System.Net.Quic label Mar 21, 2022

rzikm removed the area-System.Net label Mar 21, 2022

ManickaP reviewed Mar 21, 2022

View reviewed changes

wfurt reviewed Mar 21, 2022

View reviewed changes

src/libraries/System.Net.Quic/src/Resources/Strings.resx Outdated Show resolved Hide resolved

Code review feedback

e56b8d8

rzikm requested a review from ManickaP March 28, 2022 08:54

Merge remote-tracking branch 'upstream/main' into 32068-quic-socketad…

f62aa6f

…dress

rzikm mentioned this pull request Mar 29, 2022

[QUIC] IPv6 addresses don't work on Linux #67301

Closed

rzikm added 2 commits March 29, 2022 22:03

Disable broken tests

b0f31ac

Rename file

3a56477

ManickaP approved these changes Mar 29, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into 32068-quic-socketad…

5f5954d

…dress

rzikm merged commit 38fe468 into dotnet:main Mar 30, 2022

karelz added this to the 7.0.0 milestone Apr 8, 2022

ghost locked as resolved and limited conversation to collaborators May 8, 2022

Comments

Conversation

rzikm commented Mar 17, 2022

Uh oh!

ghost commented Mar 18, 2022

Uh oh!

ghost commented Mar 21, 2022

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

ManickaP Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

rzikm Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

ManickaP Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

rzikm Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

ManickaP Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ManickaP Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

MihaZupan Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

wfurt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rzikm commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ManickaP commented Mar 23, 2022

Uh oh!

rzikm commented Mar 23, 2022

Uh oh!

ManickaP commented Mar 23, 2022

Uh oh!

ManickaP commented Mar 28, 2022

Uh oh!

rzikm commented Mar 29, 2022

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rzikm commented Mar 22, 2022 •

edited

Loading