[QUIC] API QuicConnection - #71783

Merged
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection
Jul 13, 2022
Merged

[QUIC] API QuicConnection#71783
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection

Conversation

@ManickaP

@ManickaPManickaP commented Jul 7, 2022

Copy link
Copy Markdown
Member

Fixes#68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghostghost assigned ManickaPJul 7, 2022
@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

Author:ManickaP
Assignees:-
Labels:

new-api-needs-documentation, area-System.Net.Quic

Milestone:-

@ManickaPManickaP mentioned this pull request Jul 8, 2022
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch 4 times, most recently from 88ad8f3 to e02c32cCompareJuly 8, 2022 18:36
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch from 189b9f5 to b480845CompareJuly 10, 2022 20:25
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is fairly stable now, ready for review. Still letting it in draft due to unapproved API.
@CarnaViire@wfurt@rzikm if you could give it a look.

Also note that stream will be soon in PR as well.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated

internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint iPEndPoint)
{
// TODO: is the layout same for SocketAddress.Buffer and QuicAddr on all platforms?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should AFAIK. It would be nice to have some assert about it. It differs among platforms but MsQUIC now uses native layout e.g. same as SocketAddress.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

authenticationOptions.ClientCertificates ?? new X509CertificateCollection(),
null,
Array.Empty<string>());
if (selectedCertificate.HasPrivateKey())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to throw or at least log. I know this is what we we in SslStream for historical reasons but the failures when we silently ignore given certificate are annoying IMHO.
(this could be separate work)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ManickaPManickaP mentioned this pull request Jul 12, 2022
2 tasks
@ManickaP
ManickaP marked this pull request as ready for review July 12, 2022 19:57
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is update based on the last version of API from #68902 (comment)
Main changes:

  • behavior of DisposeAsync (automatically will close with DefaultCloseErrorCode)
  • removed required from the options and replaced by explicit validation

@CarnaViire@wfurt@rzikm please give it look

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, modulo comments.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicDefaults.cs Outdated
@ManickaP

Copy link
Copy Markdown
MemberAuthor

Failure is #71890, it's been closed in the meantime.

@ManickaP
ManickaP merged commit d434c4a into dotnet:mainJul 13, 2022
@ManickaP
ManickaP deleted the mapichov/quic-connection branch July 13, 2022 07:22
@ManickaP

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-preview7

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2664595774

@github-actions

Copy link
Copy Markdown
Contributor

@ManickaP backporting to release/7.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Listener comment; PreviewFeature attribute
Applying: Feedback
Applying: QuicConnection new API including compilable implementation
error: sha1 information is lacking or useless (src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 QuicConnection new API including compilable implementation
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

ManickaP added a commit to ManickaP/runtime that referenced this pull request Jul 13, 2022
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
@ManickaP

Copy link
Copy Markdown
MemberAuthor

@JamesNK this will need your reaction as well. Note that the options classes were changed in the last API review and we removed the required keyword. If you hit any issues with this PR, let me know.

carlossanlop pushed a commit that referenced this pull request Jul 13, 2022
…72031) (#72106)
* [QUIC] API QuicConnection (#71783)
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
* [QUIC] API QuicStream (#71969)
* Quic stream API surface
* Fixed test compilation
* Fixed http test compilation
* HttpLoopbackConnection Dispose -> DisposeAsync
* QuicStream implementation
* Fixed some tests
* Fixed all QUIC and HTTP tests
* Fixed exception type for stream closed by connection close
* Feedback
* Fixed WebSocket.Client test build
* Feedback, test fixes
* Fixed build on framework and windows
* Fixed winhandler test
* Swap variable based on order in defining class
* Post merge fixes
* Feedback and build
* Reverted connection state to pass around abort error code
* Fixed exception type.
* [QUIC] System.Net.Quic API made public (#72031)
* System.Net.Quic removed from ASP transport package and made part of SDK ref
* Removed manual references to System.Net.Quic.csproj
@karelzkarelz added this to the 7.0.0 milestone Jul 19, 2022
@ghostghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: [QUIC] QuicConnection

5 participants

@ManickaP@CarnaViire@wfurt@rzikm@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[QUIC] API QuicConnection - #71783

Merged
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection
Jul 13, 2022
Merged

[QUIC] API QuicConnection#71783
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection

Conversation

@ManickaP

@ManickaPManickaP commented Jul 7, 2022

Copy link
Copy Markdown
Member

Fixes#68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghostghost assigned ManickaPJul 7, 2022
@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

Author:ManickaP
Assignees:-
Labels:

new-api-needs-documentation, area-System.Net.Quic

Milestone:-

@ManickaPManickaP mentioned this pull request Jul 8, 2022
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch 4 times, most recently from 88ad8f3 to e02c32cCompareJuly 8, 2022 18:36
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch from 189b9f5 to b480845CompareJuly 10, 2022 20:25
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is fairly stable now, ready for review. Still letting it in draft due to unapproved API.
@CarnaViire@wfurt@rzikm if you could give it a look.

Also note that stream will be soon in PR as well.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated

internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint iPEndPoint)
{
// TODO: is the layout same for SocketAddress.Buffer and QuicAddr on all platforms?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should AFAIK. It would be nice to have some assert about it. It differs among platforms but MsQUIC now uses native layout e.g. same as SocketAddress.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

authenticationOptions.ClientCertificates ?? new X509CertificateCollection(),
null,
Array.Empty<string>());
if (selectedCertificate.HasPrivateKey())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to throw or at least log. I know this is what we we in SslStream for historical reasons but the failures when we silently ignore given certificate are annoying IMHO.
(this could be separate work)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ManickaPManickaP mentioned this pull request Jul 12, 2022
2 tasks
@ManickaP
ManickaP marked this pull request as ready for review July 12, 2022 19:57
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is update based on the last version of API from #68902 (comment)
Main changes:

  • behavior of DisposeAsync (automatically will close with DefaultCloseErrorCode)
  • removed required from the options and replaced by explicit validation

@CarnaViire@wfurt@rzikm please give it look

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, modulo comments.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicDefaults.cs Outdated
@ManickaP

Copy link
Copy Markdown
MemberAuthor

Failure is #71890, it's been closed in the meantime.

@ManickaP
ManickaP merged commit d434c4a into dotnet:mainJul 13, 2022
@ManickaP
ManickaP deleted the mapichov/quic-connection branch July 13, 2022 07:22
@ManickaP

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-preview7

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2664595774

@github-actions

Copy link
Copy Markdown
Contributor

@ManickaP backporting to release/7.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Listener comment; PreviewFeature attribute
Applying: Feedback
Applying: QuicConnection new API including compilable implementation
error: sha1 information is lacking or useless (src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 QuicConnection new API including compilable implementation
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

ManickaP added a commit to ManickaP/runtime that referenced this pull request Jul 13, 2022
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
@ManickaP

Copy link
Copy Markdown
MemberAuthor

@JamesNK this will need your reaction as well. Note that the options classes were changed in the last API review and we removed the required keyword. If you hit any issues with this PR, let me know.

carlossanlop pushed a commit that referenced this pull request Jul 13, 2022
…72031) (#72106)
* [QUIC] API QuicConnection (#71783)
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
* [QUIC] API QuicStream (#71969)
* Quic stream API surface
* Fixed test compilation
* Fixed http test compilation
* HttpLoopbackConnection Dispose -> DisposeAsync
* QuicStream implementation
* Fixed some tests
* Fixed all QUIC and HTTP tests
* Fixed exception type for stream closed by connection close
* Feedback
* Fixed WebSocket.Client test build
* Feedback, test fixes
* Fixed build on framework and windows
* Fixed winhandler test
* Swap variable based on order in defining class
* Post merge fixes
* Feedback and build
* Reverted connection state to pass around abort error code
* Fixed exception type.
* [QUIC] System.Net.Quic API made public (#72031)
* System.Net.Quic removed from ASP transport package and made part of SDK ref
* Removed manual references to System.Net.Quic.csproj
@karelzkarelz added this to the 7.0.0 milestone Jul 19, 2022
@ghostghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: [QUIC] QuicConnection

5 participants

@ManickaP@CarnaViire@wfurt@rzikm@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[QUIC] API QuicConnection - #71783

Merged
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection
Jul 13, 2022
Merged

[QUIC] API QuicConnection#71783
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection

Conversation

@ManickaP

@ManickaPManickaP commented Jul 7, 2022

Copy link
Copy Markdown
Member

Fixes#68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghostghost assigned ManickaPJul 7, 2022
@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

Author:ManickaP
Assignees:-
Labels:

new-api-needs-documentation, area-System.Net.Quic

Milestone:-

@ManickaPManickaP mentioned this pull request Jul 8, 2022
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch 4 times, most recently from 88ad8f3 to e02c32cCompareJuly 8, 2022 18:36
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch from 189b9f5 to b480845CompareJuly 10, 2022 20:25
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is fairly stable now, ready for review. Still letting it in draft due to unapproved API.
@CarnaViire@wfurt@rzikm if you could give it a look.

Also note that stream will be soon in PR as well.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated

internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint iPEndPoint)
{
// TODO: is the layout same for SocketAddress.Buffer and QuicAddr on all platforms?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should AFAIK. It would be nice to have some assert about it. It differs among platforms but MsQUIC now uses native layout e.g. same as SocketAddress.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

authenticationOptions.ClientCertificates ?? new X509CertificateCollection(),
null,
Array.Empty<string>());
if (selectedCertificate.HasPrivateKey())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to throw or at least log. I know this is what we we in SslStream for historical reasons but the failures when we silently ignore given certificate are annoying IMHO.
(this could be separate work)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ManickaPManickaP mentioned this pull request Jul 12, 2022
2 tasks
@ManickaP
ManickaP marked this pull request as ready for review July 12, 2022 19:57
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is update based on the last version of API from #68902 (comment)
Main changes:

  • behavior of DisposeAsync (automatically will close with DefaultCloseErrorCode)
  • removed required from the options and replaced by explicit validation

@CarnaViire@wfurt@rzikm please give it look

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, modulo comments.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicDefaults.cs Outdated
@ManickaP

Copy link
Copy Markdown
MemberAuthor

Failure is #71890, it's been closed in the meantime.

@ManickaP
ManickaP merged commit d434c4a into dotnet:mainJul 13, 2022
@ManickaP
ManickaP deleted the mapichov/quic-connection branch July 13, 2022 07:22
@ManickaP

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-preview7

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2664595774

@github-actions

Copy link
Copy Markdown
Contributor

@ManickaP backporting to release/7.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Listener comment; PreviewFeature attribute
Applying: Feedback
Applying: QuicConnection new API including compilable implementation
error: sha1 information is lacking or useless (src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 QuicConnection new API including compilable implementation
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

ManickaP added a commit to ManickaP/runtime that referenced this pull request Jul 13, 2022
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
@ManickaP

Copy link
Copy Markdown
MemberAuthor

@JamesNK this will need your reaction as well. Note that the options classes were changed in the last API review and we removed the required keyword. If you hit any issues with this PR, let me know.

carlossanlop pushed a commit that referenced this pull request Jul 13, 2022
…72031) (#72106)
* [QUIC] API QuicConnection (#71783)
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
* [QUIC] API QuicStream (#71969)
* Quic stream API surface
* Fixed test compilation
* Fixed http test compilation
* HttpLoopbackConnection Dispose -> DisposeAsync
* QuicStream implementation
* Fixed some tests
* Fixed all QUIC and HTTP tests
* Fixed exception type for stream closed by connection close
* Feedback
* Fixed WebSocket.Client test build
* Feedback, test fixes
* Fixed build on framework and windows
* Fixed winhandler test
* Swap variable based on order in defining class
* Post merge fixes
* Feedback and build
* Reverted connection state to pass around abort error code
* Fixed exception type.
* [QUIC] System.Net.Quic API made public (#72031)
* System.Net.Quic removed from ASP transport package and made part of SDK ref
* Removed manual references to System.Net.Quic.csproj
@karelzkarelz added this to the 7.0.0 milestone Jul 19, 2022
@ghostghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: [QUIC] QuicConnection

5 participants

@ManickaP@CarnaViire@wfurt@rzikm@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[QUIC] API QuicConnection - #71783

Merged
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection
Jul 13, 2022
Merged

[QUIC] API QuicConnection#71783
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection

Conversation

@ManickaP

@ManickaPManickaP commented Jul 7, 2022

Copy link
Copy Markdown
Member

Fixes#68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghostghost assigned ManickaPJul 7, 2022
@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

Author:ManickaP
Assignees:-
Labels:

new-api-needs-documentation, area-System.Net.Quic

Milestone:-

@ManickaPManickaP mentioned this pull request Jul 8, 2022
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch 4 times, most recently from 88ad8f3 to e02c32cCompareJuly 8, 2022 18:36
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch from 189b9f5 to b480845CompareJuly 10, 2022 20:25
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is fairly stable now, ready for review. Still letting it in draft due to unapproved API.
@CarnaViire@wfurt@rzikm if you could give it a look.

Also note that stream will be soon in PR as well.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated

internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint iPEndPoint)
{
// TODO: is the layout same for SocketAddress.Buffer and QuicAddr on all platforms?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should AFAIK. It would be nice to have some assert about it. It differs among platforms but MsQUIC now uses native layout e.g. same as SocketAddress.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

authenticationOptions.ClientCertificates ?? new X509CertificateCollection(),
null,
Array.Empty<string>());
if (selectedCertificate.HasPrivateKey())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to throw or at least log. I know this is what we we in SslStream for historical reasons but the failures when we silently ignore given certificate are annoying IMHO.
(this could be separate work)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ManickaPManickaP mentioned this pull request Jul 12, 2022
2 tasks
@ManickaP
ManickaP marked this pull request as ready for review July 12, 2022 19:57
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is update based on the last version of API from #68902 (comment)
Main changes:

  • behavior of DisposeAsync (automatically will close with DefaultCloseErrorCode)
  • removed required from the options and replaced by explicit validation

@CarnaViire@wfurt@rzikm please give it look

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, modulo comments.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicDefaults.cs Outdated
@ManickaP

Copy link
Copy Markdown
MemberAuthor

Failure is #71890, it's been closed in the meantime.

@ManickaP
ManickaP merged commit d434c4a into dotnet:mainJul 13, 2022
@ManickaP
ManickaP deleted the mapichov/quic-connection branch July 13, 2022 07:22
@ManickaP

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-preview7

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2664595774

@github-actions

Copy link
Copy Markdown
Contributor

@ManickaP backporting to release/7.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Listener comment; PreviewFeature attribute
Applying: Feedback
Applying: QuicConnection new API including compilable implementation
error: sha1 information is lacking or useless (src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 QuicConnection new API including compilable implementation
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

ManickaP added a commit to ManickaP/runtime that referenced this pull request Jul 13, 2022
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
@ManickaP

Copy link
Copy Markdown
MemberAuthor

@JamesNK this will need your reaction as well. Note that the options classes were changed in the last API review and we removed the required keyword. If you hit any issues with this PR, let me know.

carlossanlop pushed a commit that referenced this pull request Jul 13, 2022
…72031) (#72106)
* [QUIC] API QuicConnection (#71783)
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
* [QUIC] API QuicStream (#71969)
* Quic stream API surface
* Fixed test compilation
* Fixed http test compilation
* HttpLoopbackConnection Dispose -> DisposeAsync
* QuicStream implementation
* Fixed some tests
* Fixed all QUIC and HTTP tests
* Fixed exception type for stream closed by connection close
* Feedback
* Fixed WebSocket.Client test build
* Feedback, test fixes
* Fixed build on framework and windows
* Fixed winhandler test
* Swap variable based on order in defining class
* Post merge fixes
* Feedback and build
* Reverted connection state to pass around abort error code
* Fixed exception type.
* [QUIC] System.Net.Quic API made public (#72031)
* System.Net.Quic removed from ASP transport package and made part of SDK ref
* Removed manual references to System.Net.Quic.csproj
@karelzkarelz added this to the 7.0.0 milestone Jul 19, 2022
@ghostghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: [QUIC] QuicConnection

5 participants

@ManickaP@CarnaViire@wfurt@rzikm@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[QUIC] API QuicConnection - #71783

Merged
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection
Jul 13, 2022
Merged

[QUIC] API QuicConnection#71783
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection

Conversation

@ManickaP

@ManickaPManickaP commented Jul 7, 2022

Copy link
Copy Markdown
Member

Fixes#68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghostghost assigned ManickaPJul 7, 2022
@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

Author:ManickaP
Assignees:-
Labels:

new-api-needs-documentation, area-System.Net.Quic

Milestone:-

@ManickaPManickaP mentioned this pull request Jul 8, 2022
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch 4 times, most recently from 88ad8f3 to e02c32cCompareJuly 8, 2022 18:36
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch from 189b9f5 to b480845CompareJuly 10, 2022 20:25
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is fairly stable now, ready for review. Still letting it in draft due to unapproved API.
@CarnaViire@wfurt@rzikm if you could give it a look.

Also note that stream will be soon in PR as well.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated

internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint iPEndPoint)
{
// TODO: is the layout same for SocketAddress.Buffer and QuicAddr on all platforms?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should AFAIK. It would be nice to have some assert about it. It differs among platforms but MsQUIC now uses native layout e.g. same as SocketAddress.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

authenticationOptions.ClientCertificates ?? new X509CertificateCollection(),
null,
Array.Empty<string>());
if (selectedCertificate.HasPrivateKey())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to throw or at least log. I know this is what we we in SslStream for historical reasons but the failures when we silently ignore given certificate are annoying IMHO.
(this could be separate work)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ManickaPManickaP mentioned this pull request Jul 12, 2022
2 tasks
@ManickaP
ManickaP marked this pull request as ready for review July 12, 2022 19:57
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is update based on the last version of API from #68902 (comment)
Main changes:

  • behavior of DisposeAsync (automatically will close with DefaultCloseErrorCode)
  • removed required from the options and replaced by explicit validation

@CarnaViire@wfurt@rzikm please give it look

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, modulo comments.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicDefaults.cs Outdated
@ManickaP

Copy link
Copy Markdown
MemberAuthor

Failure is #71890, it's been closed in the meantime.

@ManickaP
ManickaP merged commit d434c4a into dotnet:mainJul 13, 2022
@ManickaP
ManickaP deleted the mapichov/quic-connection branch July 13, 2022 07:22
@ManickaP

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-preview7

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2664595774

@github-actions

Copy link
Copy Markdown
Contributor

@ManickaP backporting to release/7.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Listener comment; PreviewFeature attribute
Applying: Feedback
Applying: QuicConnection new API including compilable implementation
error: sha1 information is lacking or useless (src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 QuicConnection new API including compilable implementation
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

ManickaP added a commit to ManickaP/runtime that referenced this pull request Jul 13, 2022
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
@ManickaP

Copy link
Copy Markdown
MemberAuthor

@JamesNK this will need your reaction as well. Note that the options classes were changed in the last API review and we removed the required keyword. If you hit any issues with this PR, let me know.

carlossanlop pushed a commit that referenced this pull request Jul 13, 2022
…72031) (#72106)
* [QUIC] API QuicConnection (#71783)
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
* [QUIC] API QuicStream (#71969)
* Quic stream API surface
* Fixed test compilation
* Fixed http test compilation
* HttpLoopbackConnection Dispose -> DisposeAsync
* QuicStream implementation
* Fixed some tests
* Fixed all QUIC and HTTP tests
* Fixed exception type for stream closed by connection close
* Feedback
* Fixed WebSocket.Client test build
* Feedback, test fixes
* Fixed build on framework and windows
* Fixed winhandler test
* Swap variable based on order in defining class
* Post merge fixes
* Feedback and build
* Reverted connection state to pass around abort error code
* Fixed exception type.
* [QUIC] System.Net.Quic API made public (#72031)
* System.Net.Quic removed from ASP transport package and made part of SDK ref
* Removed manual references to System.Net.Quic.csproj
@karelzkarelz added this to the 7.0.0 milestone Jul 19, 2022
@ghostghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: [QUIC] QuicConnection

5 participants

@ManickaP@CarnaViire@wfurt@rzikm@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[QUIC] API QuicConnection - #71783

Merged
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection
Jul 13, 2022
Merged

[QUIC] API QuicConnection#71783
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection

Conversation

@ManickaP

@ManickaPManickaP commented Jul 7, 2022

Copy link
Copy Markdown
Member

Fixes#68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghostghost assigned ManickaPJul 7, 2022
@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

Author:ManickaP
Assignees:-
Labels:

new-api-needs-documentation, area-System.Net.Quic

Milestone:-

@ManickaPManickaP mentioned this pull request Jul 8, 2022
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch 4 times, most recently from 88ad8f3 to e02c32cCompareJuly 8, 2022 18:36
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch from 189b9f5 to b480845CompareJuly 10, 2022 20:25
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is fairly stable now, ready for review. Still letting it in draft due to unapproved API.
@CarnaViire@wfurt@rzikm if you could give it a look.

Also note that stream will be soon in PR as well.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated

internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint iPEndPoint)
{
// TODO: is the layout same for SocketAddress.Buffer and QuicAddr on all platforms?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should AFAIK. It would be nice to have some assert about it. It differs among platforms but MsQUIC now uses native layout e.g. same as SocketAddress.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

authenticationOptions.ClientCertificates ?? new X509CertificateCollection(),
null,
Array.Empty<string>());
if (selectedCertificate.HasPrivateKey())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to throw or at least log. I know this is what we we in SslStream for historical reasons but the failures when we silently ignore given certificate are annoying IMHO.
(this could be separate work)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ManickaPManickaP mentioned this pull request Jul 12, 2022
2 tasks
@ManickaP
ManickaP marked this pull request as ready for review July 12, 2022 19:57
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is update based on the last version of API from #68902 (comment)
Main changes:

  • behavior of DisposeAsync (automatically will close with DefaultCloseErrorCode)
  • removed required from the options and replaced by explicit validation

@CarnaViire@wfurt@rzikm please give it look

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, modulo comments.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicDefaults.cs Outdated
@ManickaP

Copy link
Copy Markdown
MemberAuthor

Failure is #71890, it's been closed in the meantime.

@ManickaP
ManickaP merged commit d434c4a into dotnet:mainJul 13, 2022
@ManickaP
ManickaP deleted the mapichov/quic-connection branch July 13, 2022 07:22
@ManickaP

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-preview7

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2664595774

@github-actions

Copy link
Copy Markdown
Contributor

@ManickaP backporting to release/7.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Listener comment; PreviewFeature attribute
Applying: Feedback
Applying: QuicConnection new API including compilable implementation
error: sha1 information is lacking or useless (src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 QuicConnection new API including compilable implementation
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

ManickaP added a commit to ManickaP/runtime that referenced this pull request Jul 13, 2022
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
@ManickaP

Copy link
Copy Markdown
MemberAuthor

@JamesNK this will need your reaction as well. Note that the options classes were changed in the last API review and we removed the required keyword. If you hit any issues with this PR, let me know.

carlossanlop pushed a commit that referenced this pull request Jul 13, 2022
…72031) (#72106)
* [QUIC] API QuicConnection (#71783)
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
* [QUIC] API QuicStream (#71969)
* Quic stream API surface
* Fixed test compilation
* Fixed http test compilation
* HttpLoopbackConnection Dispose -> DisposeAsync
* QuicStream implementation
* Fixed some tests
* Fixed all QUIC and HTTP tests
* Fixed exception type for stream closed by connection close
* Feedback
* Fixed WebSocket.Client test build
* Feedback, test fixes
* Fixed build on framework and windows
* Fixed winhandler test
* Swap variable based on order in defining class
* Post merge fixes
* Feedback and build
* Reverted connection state to pass around abort error code
* Fixed exception type.
* [QUIC] System.Net.Quic API made public (#72031)
* System.Net.Quic removed from ASP transport package and made part of SDK ref
* Removed manual references to System.Net.Quic.csproj
@karelzkarelz added this to the 7.0.0 milestone Jul 19, 2022
@ghostghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: [QUIC] QuicConnection

5 participants

@ManickaP@CarnaViire@wfurt@rzikm@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[QUIC] API QuicConnection - #71783

Merged
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection
Jul 13, 2022
Merged

[QUIC] API QuicConnection#71783
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection

Conversation

@ManickaP

@ManickaPManickaP commented Jul 7, 2022

Copy link
Copy Markdown
Member

Fixes#68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghostghost assigned ManickaPJul 7, 2022
@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

Author:ManickaP
Assignees:-
Labels:

new-api-needs-documentation, area-System.Net.Quic

Milestone:-

@ManickaPManickaP mentioned this pull request Jul 8, 2022
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch 4 times, most recently from 88ad8f3 to e02c32cCompareJuly 8, 2022 18:36
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch from 189b9f5 to b480845CompareJuly 10, 2022 20:25
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is fairly stable now, ready for review. Still letting it in draft due to unapproved API.
@CarnaViire@wfurt@rzikm if you could give it a look.

Also note that stream will be soon in PR as well.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated

internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint iPEndPoint)
{
// TODO: is the layout same for SocketAddress.Buffer and QuicAddr on all platforms?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should AFAIK. It would be nice to have some assert about it. It differs among platforms but MsQUIC now uses native layout e.g. same as SocketAddress.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

authenticationOptions.ClientCertificates ?? new X509CertificateCollection(),
null,
Array.Empty<string>());
if (selectedCertificate.HasPrivateKey())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to throw or at least log. I know this is what we we in SslStream for historical reasons but the failures when we silently ignore given certificate are annoying IMHO.
(this could be separate work)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ManickaPManickaP mentioned this pull request Jul 12, 2022
2 tasks
@ManickaP
ManickaP marked this pull request as ready for review July 12, 2022 19:57
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is update based on the last version of API from #68902 (comment)
Main changes:

  • behavior of DisposeAsync (automatically will close with DefaultCloseErrorCode)
  • removed required from the options and replaced by explicit validation

@CarnaViire@wfurt@rzikm please give it look

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, modulo comments.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicDefaults.cs Outdated
@ManickaP

Copy link
Copy Markdown
MemberAuthor

Failure is #71890, it's been closed in the meantime.

@ManickaP
ManickaP merged commit d434c4a into dotnet:mainJul 13, 2022
@ManickaP
ManickaP deleted the mapichov/quic-connection branch July 13, 2022 07:22
@ManickaP

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-preview7

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2664595774

@github-actions

Copy link
Copy Markdown
Contributor

@ManickaP backporting to release/7.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Listener comment; PreviewFeature attribute
Applying: Feedback
Applying: QuicConnection new API including compilable implementation
error: sha1 information is lacking or useless (src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 QuicConnection new API including compilable implementation
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

ManickaP added a commit to ManickaP/runtime that referenced this pull request Jul 13, 2022
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
@ManickaP

Copy link
Copy Markdown
MemberAuthor

@JamesNK this will need your reaction as well. Note that the options classes were changed in the last API review and we removed the required keyword. If you hit any issues with this PR, let me know.

carlossanlop pushed a commit that referenced this pull request Jul 13, 2022
…72031) (#72106)
* [QUIC] API QuicConnection (#71783)
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
* [QUIC] API QuicStream (#71969)
* Quic stream API surface
* Fixed test compilation
* Fixed http test compilation
* HttpLoopbackConnection Dispose -> DisposeAsync
* QuicStream implementation
* Fixed some tests
* Fixed all QUIC and HTTP tests
* Fixed exception type for stream closed by connection close
* Feedback
* Fixed WebSocket.Client test build
* Feedback, test fixes
* Fixed build on framework and windows
* Fixed winhandler test
* Swap variable based on order in defining class
* Post merge fixes
* Feedback and build
* Reverted connection state to pass around abort error code
* Fixed exception type.
* [QUIC] System.Net.Quic API made public (#72031)
* System.Net.Quic removed from ASP transport package and made part of SDK ref
* Removed manual references to System.Net.Quic.csproj
@karelzkarelz added this to the 7.0.0 milestone Jul 19, 2022
@ghostghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: [QUIC] QuicConnection

5 participants

@ManickaP@CarnaViire@wfurt@rzikm@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[QUIC] API QuicConnection - #71783

Merged
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection
Jul 13, 2022
Merged

[QUIC] API QuicConnection#71783
ManickaP merged 23 commits into
dotnet:mainfrom
ManickaP:mapichov/quic-connection

Conversation

@ManickaP

@ManickaPManickaP commented Jul 7, 2022

Copy link
Copy Markdown
Member

Fixes#68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghostghost assigned ManickaPJul 7, 2022
@ghost

ghost commented Jul 7, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #68902
Contributes to #69675

This PR is based on #71579 so it shows more changes that it actually contains.

Author:ManickaP
Assignees:-
Labels:

new-api-needs-documentation, area-System.Net.Quic

Milestone:-

@ManickaPManickaP mentioned this pull request Jul 8, 2022
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch 4 times, most recently from 88ad8f3 to e02c32cCompareJuly 8, 2022 18:36
@ManickaP
ManickaPforce-pushed the mapichov/quic-connection branch from 189b9f5 to b480845CompareJuly 10, 2022 20:25
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is fairly stable now, ready for review. Still letting it in draft due to unapproved API.
@CarnaViire@wfurt@rzikm if you could give it a look.

Also note that stream will be soon in PR as well.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated

internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint iPEndPoint)
{
// TODO: is the layout same for SocketAddress.Buffer and QuicAddr on all platforms?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should AFAIK. It would be nice to have some assert about it. It differs among platforms but MsQUIC now uses native layout e.g. same as SocketAddress.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

authenticationOptions.ClientCertificates ?? new X509CertificateCollection(),
null,
Array.Empty<string>());
if (selectedCertificate.HasPrivateKey())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to throw or at least log. I know this is what we we in SslStream for historical reasons but the failures when we silently ignore given certificate are annoying IMHO.
(this could be separate work)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ManickaPManickaP mentioned this pull request Jul 12, 2022
2 tasks
@ManickaP
ManickaP marked this pull request as ready for review July 12, 2022 19:57
@ManickaP

Copy link
Copy Markdown
MemberAuthor

This is update based on the last version of API from #68902 (comment)
Main changes:

  • behavior of DisposeAsync (automatically will close with DefaultCloseErrorCode)
  • removed required from the options and replaced by explicit validation

@CarnaViire@wfurt@rzikm please give it look

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, modulo comments.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs Outdated
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicDefaults.cs Outdated
@ManickaP

Copy link
Copy Markdown
MemberAuthor

Failure is #71890, it's been closed in the meantime.

@ManickaP
ManickaP merged commit d434c4a into dotnet:mainJul 13, 2022
@ManickaP
ManickaP deleted the mapichov/quic-connection branch July 13, 2022 07:22
@ManickaP

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-preview7

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2664595774

@github-actions

Copy link
Copy Markdown
Contributor

@ManickaP backporting to release/7.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Listener comment; PreviewFeature attribute
Applying: Feedback
Applying: QuicConnection new API including compilable implementation
error: sha1 information is lacking or useless (src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 QuicConnection new API including compilable implementation
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

ManickaP added a commit to ManickaP/runtime that referenced this pull request Jul 13, 2022
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
@ManickaP

Copy link
Copy Markdown
MemberAuthor

@JamesNK this will need your reaction as well. Note that the options classes were changed in the last API review and we removed the required keyword. If you hit any issues with this PR, let me know.

carlossanlop pushed a commit that referenced this pull request Jul 13, 2022
…72031) (#72106)
* [QUIC] API QuicConnection (#71783)
* Listener comment; PreviewFeature attribute
* Feedback
* QuicConnection new API including compilable implementation
* Fixed logging
* Fixed S.N.Quic and S.N.Http tests
* Options now correspond to the issue
* Feedback
* Comments, PreviewFeature attribute and RemoteCertificate disposal.
* Preview feature attribute is assembly wide
* Some typos
* Fixed test with certificate
* Default values as constants
* Event handlers split into methods called via switch expression.
* Some more comments
* Unified unsafe usage
* Fixed some more tests
* Cleaned up some exceptions and resource strings.
* Feedback
* Latest greatest API proposal.
* Fixed Http solution
* Feedback
* [QUIC] API QuicStream (#71969)
* Quic stream API surface
* Fixed test compilation
* Fixed http test compilation
* HttpLoopbackConnection Dispose -> DisposeAsync
* QuicStream implementation
* Fixed some tests
* Fixed all QUIC and HTTP tests
* Fixed exception type for stream closed by connection close
* Feedback
* Fixed WebSocket.Client test build
* Feedback, test fixes
* Fixed build on framework and windows
* Fixed winhandler test
* Swap variable based on order in defining class
* Post merge fixes
* Feedback and build
* Reverted connection state to pass around abort error code
* Fixed exception type.
* [QUIC] System.Net.Quic API made public (#72031)
* System.Net.Quic removed from ASP transport package and made part of SDK ref
* Removed manual references to System.Net.Quic.csproj
@karelzkarelz added this to the 7.0.0 milestone Jul 19, 2022
@ghostghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: [QUIC] QuicConnection

5 participants

@ManickaP@CarnaViire@wfurt@rzikm@karelz