[QUIC] Adopted msquic generated interop by ManickaP · Pull Request #68288 · dotnet/runtime

ManickaP · 2022-04-20T18:36:20Z

ghost · 2022-04-20T18:36:28Z

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

Issue Details

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

src/libraries/Common/tests/TestUtilities/TestEventListener.cs

ManickaP · 2022-04-20T18:40:34Z

...raries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicException.cs

+
+namespace Microsoft.Quic
+{
+    internal class MsQuicException : QuicException


I'm not 100% convinced about this, we could merge this with QuicException. I just wanted to keep msquic error based exception separate. Or can deal with this when we discus more broader exception topics like #55619 and #56954

ManickaP · 2022-04-20T18:44:04Z

...aries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicSafeHandle.cs

+
+namespace System.Net.Quic.Implementations.MsQuic.Internal
+{
+    internal abstract class MsQuicSafeHandle : SafeHandle


Extracted some similar code from all our SafeHandle implementations. We don't actually need SafeHandles atm since we're working directly with native pointers in p/invokes. But we discussed using reference counting for stream-connection lifetime relation so I'm keeping this in for now.

ManickaP · 2022-04-20T18:46:33Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/msquic.cs

@@ -0,0 +1,191 @@
+#pragma warning disable IDE0073
+//
+// Copyright (c) Microsoft Corporation.


@samsp-msft do we have any guidelines about including other code in our repo?
I workaround the compilation with warning disable but I got a feeling there might be some legal implications since dotnet belongs to the foundation and not Microsoft.

I already mentioned I think we should find a way you don't have to check this file in, but if you do, since it's MIT licensed, that practically means you can take it and do what you want with it, which I believe, means you could change it to your normal .NET license header.

The general guideline is at https://github.com/dotnet/runtime/blob/main/CONTRIBUTING.md#copying-files-from-other-projects

There needs to be some details captured on where this file came from and what modifications were done to it. Either add it to the top of the file or add it in a separate file.

This should be added to https://github.com/dotnet/runtime/blob/main/THIRD-PARTY-NOTICES.TXT

Is the #pragma warning disable OK in this case or is there another way to suppress the error if I'm to leave the original notice.

Also the example file has our notice:

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/IdnMapping.cs

Lines 1 to 2 in 0ae6b95

// Licensed to the .NET Foundation under one or more agreements.

// The .NET Foundation licenses this file to you under the MIT license.

So should I start with our notice and then the original and then info about where it came and what changes I've made?

I'll add third party notice as well, I'm mostly asking about the specifics of the imported files.

If it is a verbatim copy of the file from the other project without any changes, it is better to leave it intact, without the .NET Foundation header. It is easier for consuming updates.

We have a lot of similar cases under https://github.com/dotnet/runtime/tree/main/src/native/external . Notice that all files in the subdirectories are verbatim copies of other projects, without the .NET Foundation header.

it is not verbatim copy, right, @ManickaP? From the original list of changes, most made it to MsQuic, but we still change the type of exception thrown from ThrowIfFailure.

It's not verbatim.

Whats still missing? I'm happy to make it verbatim. I thought I got everything, but might have missed something.

I see what I missed. Let me PR it into MsQuic. The only missing thing will be using System.Net.Quic, but that isn't actually needed from what I see.

Ah. I see theres also some differences in MsQuicException.cs, but those I won't be able to fully solve. I'll get close though.

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

…ons/MsQuic/MsQuicListener.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs

rzikm · 2022-04-20T19:17:20Z

Suppose there is a change in MsQuic API in the future, what actions are needed to update the interop layer from our end? In other words, what changes did you make to the generated files so that they can be used by the runtime?

ManickaP · 2022-04-21T09:14:57Z

Suppose there is a change in MsQuic API in the future, what actions are needed to update the interop layer from our end? In other words, what changes did you make to the generated files so that they can be used by the runtime?

3 things:

#pragma warning disable/restore
making all types internal instead of public
QuicException -> MsQuicException

I strongly recommend comparing the files in some visual diff tool (e.g. beyond compare) to see the changes.

Also if anyone has a better idea how to consume it with less "manual work", I'm all ears.

...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs

ManickaP · 2022-05-03T20:12:18Z

@jkotas Thanks a lot for taking the time and helping me with the buffers! I really appreciate it.

...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs

nibanks

LGTM 👍

CarnaViire

LGTM, thanks! 🚀

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs

...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs

ManickaP · 2022-05-05T10:32:09Z

Failures unrelated.

This reverts commit 992b395.

…" (dotnet#68940)" This reverts commit 4820674.

* Revert "Revert "[QUIC] Adopted msquic generated interop (#68288)" (#68940)" This reverts commit 4820674. * Exclude S.N.Quic from Mono AOT * Fix #68954

ManickaP added 12 commits April 20, 2022 18:47

MsQuicApi adjusted

c6edafb

QuicParamHelper adjusted

ea31c49

MsQuicConfiguration adjusted

8d41cbb

Added MsQuicException

55dc554

MsQuicConnection adjusted

259545e

MsQuicListener adjusted

d0fb04e

MsQuicStream adjusted

b3d76cf

It compiles!

103cd1f

Some test fixes

08b5b47

Fixed stream read event handling

7794b58

Re-enabled IPv6 tests

46a7401

latest greatest

182a8f0

ghost added the area-System.Net.Quic label Apr 20, 2022

ghost assigned ManickaP Apr 20, 2022

ManickaP commented Apr 20, 2022

View reviewed changes

src/libraries/Common/tests/TestUtilities/TestEventListener.cs Show resolved Hide resolved

ManickaP commented Apr 20, 2022

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs Outdated Show resolved Hide resolved

Update src/libraries/System.Net.Quic/src/System/Net/Quic/Implementati…

2b0172c

…ons/MsQuic/MsQuicListener.cs

nibanks reviewed Apr 20, 2022

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs Outdated Show resolved Hide resolved

ManickaP mentioned this pull request Apr 20, 2022

MsQuic interop ManickaP/runtime#1

Closed

runfoapp bot mentioned this pull request Apr 20, 2022

System.IO.Compression.Tests.ZipFile_Unix.UnixCreateSetsPermissionsInExternalAttributes fail on OSX #68293

Closed

MsQuic interop enum names

3162353

ManickaP requested review from CarnaViire, samsp-msft and wfurt April 21, 2022 09:16

jkotas reviewed Apr 27, 2022

View reviewed changes

...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs Outdated Show resolved Hide resolved

CarnaViire mentioned this pull request May 3, 2022

Merge several version of MsQuicStream SendAsync code #68772

Merged

Merge branch 'main' into mapichov/quic_interop

059c3d4

ManickaP force-pushed the mapichov/quic_interop branch from 30dfdb9 to 059c3d4 Compare May 3, 2022 16:57

ManickaP added 2 commits May 3, 2022 19:40

Newest msquic interop

cd3b00d

Buffers feedback

c6e0d6c

jkotas reviewed May 3, 2022

View reviewed changes

...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs Outdated Show resolved Hide resolved

Remove MemoryHandle pooling

de15868

nibanks approved these changes May 4, 2022

View reviewed changes

CarnaViire approved these changes May 4, 2022

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs Outdated Show resolved Hide resolved

...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs Outdated Show resolved Hide resolved

jkotas reviewed May 4, 2022

View reviewed changes

...braries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicBuffers.cs Outdated Show resolved Hide resolved

ManickaP added 3 commits May 4, 2022 19:00

Buffers feedback

a73cfa8

Logging feedback

d52fcdb

Merge branch 'main' into mapichov/quic_interop

cf2377c

ManickaP merged commit 992b395 into dotnet:main May 5, 2022

ManickaP deleted the mapichov/quic_interop branch May 5, 2022 10:32

SingleAccretion mentioned this pull request May 5, 2022

Do not fold relocatable constants into displacements #68851

Merged

jkotas mentioned this pull request May 5, 2022

LLVM AOT cross-compile CoreCLR tests fail in all PR #68932

Closed

jkotas added a commit that referenced this pull request May 5, 2022

Revert "[QUIC] Adopted msquic generated interop (#68288)"

a523fcc

This reverts commit 992b395.

jkotas mentioned this pull request May 5, 2022

Revert "[QUIC] Adopted msquic generated interop" #68940

Merged

jkotas added a commit that referenced this pull request May 6, 2022

Revert "[QUIC] Adopted msquic generated interop (#68288)" (#68940)

4820674

This reverts commit 992b395.

jkotas mentioned this pull request May 6, 2022

failfast in native code invoked by MsQuicStream.Dispose in CI #68954

Closed

ManickaP added a commit to ManickaP/runtime that referenced this pull request May 7, 2022

Revert "Revert "[QUIC] Adopted msquic generated interop (dotnet#68288)…

b8f3819

…" (dotnet#68940)" This reverts commit 4820674.

This was referenced May 7, 2022

"[QUIC] Adopted msquic generated interop (#68288)" attempt no 2 #69009

Merged

[mono] FullAOT runtime test failures: condition `out_obj' not met #57361

Closed

ghost locked as resolved and limited conversation to collaborators Jun 4, 2022

karelz added this to the 7.0.0 milestone Jul 19, 2022

	// Licensed to the .NET Foundation under one or more agreements.
	// The .NET Foundation licenses this file to you under the MIT license.

Conversation

ManickaP commented Apr 20, 2022

Uh oh!

ghost commented Apr 20, 2022

Uh oh!

Uh oh!

ManickaP Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rzikm commented Apr 20, 2022

Uh oh!

ManickaP commented Apr 21, 2022

Uh oh!

Uh oh!

ManickaP commented May 3, 2022

Uh oh!

Uh oh!

nibanks left a comment

Choose a reason for hiding this comment

Uh oh!

CarnaViire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ManickaP commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ManickaP Apr 20, 2022 •

edited

Loading