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

Add MsQuic implementation of System.Net.Quic #427

Merged
merged 49 commits into from
Jan 9, 2020
Merged

Add MsQuic implementation of System.Net.Quic #427

merged 49 commits into from
Jan 9, 2020

Conversation

jkotalik
Copy link
Contributor

@jkotalik jkotalik commented Dec 2, 2019

This introduces an implementation of System.Net.Quic using MsQuic as our Quic library. This loosely copied what we have in Kestrel today to fit the object-model of System.Net.Quic.

I went ahead and added DisposeAsync. I'm hitting issues with how to handle shutdown. Right now in my PR, a normal call to Dispose will do sync over async to do a graceful shutdown. This could also do a abortive shutdown instead and require people to call both ShutdownRead and ShutdownWrite to guarantee reads and writes are sent/received.

Currently, I'm using System.IO.Pipelines for handling synchronization when reading the request. The overall problem is how to handle synchronizing calls to ReadAsync on the System.IO.Stream and with MsQuic receiving a callback that data has been received. Either of these could occur first. I initially wasn't using System.IO.Pipelines and trying to handroll a single buffer synchronization construct between these calls, but I quickly realized I was mostly re-implementing pipes. IIRC Pipes are currently not in the BCL. I don't quite know the status of how this package is built/delivered, but until that discussion is decided, I'd like to make sure Pipes isn't brought into the BCL.

I added a few basic unit tests for creating connections and streams. These are skipped for now until we figure out our strategy for getting MsQuic.

I'm think I hit issues with adding a new native pinvoke library. I was getting the error: msquic!MsQuicOpen is not supported on one\more targeted platforms.

There are probably also some style fixes that I need to address as well. It's expected when coming from different style guidelines 😄

@jkotalik jkotalik added this to the 5.0 milestone Dec 2, 2019
@jkotalik jkotalik requested review from davidfowl and a team December 2, 2019 06:35
gfoidl
gfoidl reviewed Dec 2, 2019
View reviewed changes
@scalablecory
Copy link
Contributor

I went ahead and added DisposeAsync. I'm hitting issues with how to handle shutdown. Right now in my PR, a normal call to Dispose will do sync over async to do a graceful shutdown. This could also do a abortive shutdown instead and require people to call both ShutdownRead and ShutdownWrite to guarantee reads and writes are sent/received.

I'd definitely like us to support abortive dispose.

@scalablecory
Copy link
Contributor

We like to organize p/invoke signatures and constants in an Interop file/class. Is that reasonable to do here?

@scalablecory
Copy link
Contributor

Can we use safe handles here?

jkotas
jkotas reviewed Dec 2, 2019
View reviewed changes
nibanks
nibanks reviewed Dec 2, 2019
View reviewed changes
jkotas
jkotas reviewed Dec 2, 2019
View reviewed changes
...s/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicNativeMethods.cs Outdated
internal static extern int MsQuicOpen(int version, out NativeApi* registration);

[StructLayout(LayoutKind.Sequential)]
internal struct NativeApi
Copy link
Member

Choose a reason for hiding this comment

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

The single entry-point that returns an array of pointers to call looks odd for a C library. It just makes the interop more painful than what it needs to be. And makes it impossible to get rid of unused code when the library is linked statically (important for mobile apps).

Copy link
Member

@jkotas jkotas Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You have marked this as resolved without any comments, but I do not see it addressed in the delta either. Any thoughts about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't a question I can answer. @nibanks ?

Copy link

Choose a reason for hiding this comment

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

I don't understand the question.

Copy link
Member

Choose a reason for hiding this comment

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

The question is why you have to go through the extra hoop to get the pointers to actual method to call. Why not export the C APIs directly?

Copy link

Choose a reason for hiding this comment

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

This is how we version the API. We have a different function table per API version. BTW, the API is not statically linked. It's a dynamic library.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, it is very Windows-centric design. The de-facto standards for popular cross-platforms open-source libraries are different.

Copy link

Choose a reason for hiding this comment

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

Perhaps, but not many cross platform APIs also have to work in Windows kernel mode and support Windows updates.

Copy link
Member

Choose a reason for hiding this comment

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

You can have a simple C API for the public consumption, and keep the Windows kernel/inbox specific extra complexity to be Windows kernel/inbox only.

Copy link

Choose a reason for hiding this comment

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

It's all the same API. I have no plans to change this design at this time.

@jkotalik jkotalik force-pushed the jkotalik/systemNetQuic branch from 8936c93 to ea82daf Compare January 6, 2020 19:31
@jkotalik
Copy link
Contributor Author

jkotalik commented Jan 7, 2020

From what I can tell, System.Net.NetworkInformation.Functional.Tests is always failing for all builds right now.

scalablecory
scalablecory approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

Approving early to unblock HTTP/3. We will need to continue iterating this.

Before merging, please take any outstanding PR comments and make them TODOs in code so we can keep track of them later. Also, double check that all tests are disabled so we don't break CI for anyone.

@jkotalik
Copy link
Contributor Author

jkotalik commented Jan 9, 2020

Before merging, please take any outstanding PR comments and make them TODOs in code so we can keep track of them later. Also, double check that all tests are disabled so we don't break CI for anyone.

TODOs have been created. Will create a follow up issue tracking outstanding work based on the TODOs

@jkotalik jkotalik merged commit 8eb20aa into dotnet:master Jan 9, 2020
@analogrelay
Copy link
Contributor

A scene from the movie "Anchorman". Four men in suits jump up excitedly.

janvorli added a commit to janvorli/runtime that referenced this pull request Jan 13, 2020
@janvorli
Fix System.Net.Quick.csproj configurations
12fad48 
The Configurations.props file was modified to have $(NetCoreAppCurrent)-Linux and
$(NetCoreAppCurrent)-OSX instead of $(NetCoreAppCurrent)-Unix in a recent PR dotnet#427,
but the Configurations in the System.Net.Quic.csproj were not updated accordingly.
This leads to local compilation failure.

This change fixes it.
@janvorli janvorli mentioned this pull request Jan 13, 2020
Merged
@scalablecory scalablecory mentioned this pull request Jan 15, 2020
Closed
ViktorHofer pushed a commit that referenced this pull request Jan 22, 2020
@janvorli @ViktorHofer
Fix System.Net.Quick.csproj configurations (#1675)
5971ba4 
The Configurations.props file was modified to have $(NetCoreAppCurrent)-Linux and
$(NetCoreAppCurrent)-OSX instead of $(NetCoreAppCurrent)-Unix in a recent PR #427,
but the Configurations in the System.Net.Quic.csproj were not updated accordingly.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@analogrelay analogrelay analogrelay left review comments

@halter73 halter73 halter73 left review comments

@davidfowl davidfowl davidfowl left review comments

@Tratcher Tratcher Tratcher left review comments

@gfoidl gfoidl gfoidl left review comments

@jkotas jkotas jkotas left review comments

@geoffkizer geoffkizer geoffkizer left review comments

@nibanks nibanks nibanks left review comments

@scalablecory scalablecory scalablecory approved these changes

Assignees
No one assigned
Labels
area-System.Net
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.