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

Initial (partially-reviewed API) System.Net.Connections. #39524

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Initial (partially-reviewed API) System.Net.Connections. #39524

merged 1 commit into from
Jul 28, 2020

Conversation

scalablecory
Copy link
Contributor

@scalablecory scalablecory commented Jul 17, 2020 •
edited
Loading

This implements the already reviewed APIs of #1793. Followup PR will implement the Socket factories, otherwise this should match reviewed APIs.

Adds:

  • System.Net.Connections assembly.

Makes these changes to existing code:

  • System.IO.Pipelines is now included in Net50 SDK.
  • SocketsHttpHandler properties added to connect via a connection factory rather than directly on a Socket.
  • Test code: Loopback connections allows use without their accept servers.
  • Test code: Moved settings read/write code from http/2 accept server into connection.
  • Test code: VirtualNetwork supports read/write shutdown.

TODO:

  • Implement tests for ConnectionExtensions and factories.
  • Some additional tests for SocketsHttpHandler would be useful.

@scalablecory scalablecory added this to the 5.0.0 milestone Jul 17, 2020
@scalablecory scalablecory requested review from geoffkizer, wfurt, alnikola and a team July 17, 2020 16:10
@scalablecory scalablecory self-assigned this Jul 17, 2020
@ghost
Copy link

ghost commented Jul 17, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot
Copy link
Collaborator

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.

@scalablecory
Copy link
Contributor Author

CC @JamesNK

wfurt
wfurt approved these changes Jul 19, 2020
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM with few comments.
But I'm not the expert on this.

src/libraries/System.Net.Connections/src/System/Net/Connections/Connection.cs Show resolved Hide resolved
src/libraries/System.Net.Connections/src/System/Net/Connections/ConnectionBase.cs Show resolved Hide resolved
src/libraries/System.Net.Connections/src/System/Net/Connections/ConnectionExtensions.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved
@wfurt
Copy link
Member

wfurt commented Jul 19, 2020

cc: @safern for any of the build changes.

safern
safern reviewed Jul 19, 2020
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

I'll take a look at the build changes

src/libraries/System.Net.Connections/src/System/Net/Connections/Connection.cs Outdated Show resolved Hide resolved
@scalablecory scalablecory marked this pull request as ready for review July 20, 2020 07:20
@scalablecory
Copy link
Contributor Author

scalablecory commented Jul 20, 2020 •
edited
Loading

@safern can you please verify that the changes to System.IO.Pipelines are correct? Goal is to have it build as part of non-aspnet SDKs, as System.Net.Http now depends on it.
@safern also that there's nothing else I need to do to add System.Net.Connections to SDKs.

@safern
Copy link
Member

safern commented Jul 20, 2020

Yeah, I'll look at this in the morning as these kind of changes need file navigation and in the phone is not ideal. Count on my review tomorrow morning 😊

@ViktorHofer
Copy link
Member

Note to myself, this will conflict with #35606. If you are in first I need to react, otherwise you by declaring that the library is part of the shared framework in a different file.

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@scalablecory
Copy link
Contributor Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl
Copy link
Member

System.IO.Pipelines is now included in Net50 SDK.

MVP ✌🏾

@scalablecory scalablecory reopened this Jul 28, 2020
@scalablecory
Copy link
Contributor Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scalablecory scalablecory merged commit 4b8c5fe into dotnet:master Jul 28, 2020
@omariom
Copy link
Contributor

omariom commented Jul 28, 2020

@scalablecory Will it be available as a .NET Standard nuget?

@davidfowl davidfowl mentioned this pull request Jul 29, 2020
Closed
@karelz

This comment has been minimized.

Sign in to view
@stephentoub
Copy link
Member

it is tied to SocketsHttpHandler

@karelz, it isn't. SocketsHttpHandler is one consumer of the APIs.

@karelz
Copy link
Member

karelz commented Jul 29, 2020

OK, sorry for the wrong answer then, I'll hide my reply to avoid further confusion and let others answer ...

@scalablecory
Copy link
Contributor Author

@scalablecory Will it be available as a .NET Standard nuget?

If you have use cases for this as a .NET Standard package, please open a new issue so we can gauge interest and discuss further.

No concrete plans to announce here but we have been discussing this. Specifically, @davidfowl indicated interest to use this in the .NET Standard SignalR client library.

Note - even if these core abstractions make it into a package, HttpClient's support for it will still only be in .NET 5.

@scalablecory scalablecory deleted the connections branch July 29, 2020 16:05
ericstj
ericstj reviewed Jul 29, 2020
View reviewed changes
src/libraries/System.IO.Pipelines/pkg/System.IO.Pipelines.pkgproj
@@ -11,6 +11,7 @@
</ProjectReference>
<ProjectReference Include="..\src\System.IO.Pipelines.csproj" />
<HarvestIncludePaths Include="lib/netstandard1.3" />
<InboxOnTargetFramework Include="net5.0" />
Copy link
Member

@ericstj ericstj Jul 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is bad. Don't do this. It freezes the API. Either stop producing the package, or treat it as OOB if you still want to control the API like an OOB (EG: System.Text.Json, ImmutableCollections, etc.) I'd recommend the latter. Go check how we package System.Text.Json for an example.

Copy link
Member

Choose a reason for hiding this comment

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

We should be doing the latter. It's just like System.Text.Json etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CC @geoffkizer

Copy link
Member

Choose a reason for hiding this comment

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

I went ahead and submitted a fix for this: #40212

stephentoub
stephentoub reviewed Aug 9, 2020
View reviewed changes
src/libraries/System.Net.Connections/src/System/Net/Connections/Connection.cs
namespace System.Net.Connections
{
/// <summary>
/// A connection.
Copy link
Member

Choose a reason for hiding this comment

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

Can we include more descriptive docs?

src/libraries/System.Net.Connections/src/System/Net/Connections/DuplexPipeStream.cs

public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
{
if (buffer == null) return Task.FromException<int>(ExceptionDispatchInfo.SetCurrentStackTrace(new ArgumentNullException(nameof(buffer))));
Copy link
Member

Choose a reason for hiding this comment

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

Argument exceptions should just be thrown rather than stored into the returned task.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Stream stream = tunnelResponse.Content.ReadAsStream(cancellationToken);
EndPoint remoteEndPoint = new DnsEndPoint(_originAuthority.IdnHost, _originAuthority.Port);

// TODO: the Socket from the response can be funneled into a connection property here.
Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue tracking this?

...aries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpConnectionFactory.cs
{
if (options == null || !options.TryGet(out DnsEndPointWithProperties? httpOptions))
{
return ValueTask.FromException<Connection>(ExceptionDispatchInfo.SetCurrentStackTrace(new HttpRequestException($"{nameof(SocketsHttpConnectionFactory)} requires a {nameof(DnsEndPointWithProperties)} property.")));
Copy link
Member

Choose a reason for hiding this comment

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

SetCurrentStackTrace is really only useful when we expect the call chain and the await chain to be different, e.g. if an exception occurs, is stored into a field, and is then later thrown from that field. Here, presumably we expect the 99% case to be await ConnectAsync(...), in which case the SetCurrentStackTrace here is not only unnecessary.

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@scalablecory @Jacksondr5
Initial (partially-reviewed API) System.Net.Connections. (dotnet#39524)
4ca29c4
@antonfirsov antonfirsov mentioned this pull request Aug 11, 2020
Merged
@ManickaP ManickaP mentioned this pull request Aug 15, 2020
Closed
@geoffkizer geoffkizer mentioned this pull request Sep 15, 2020
Closed
@antonfirsov antonfirsov mentioned this pull request Dec 4, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@buyaa-n buyaa-n buyaa-n left review comments

@safern safern safern left review comments

@wfurt wfurt wfurt approved these changes

@geoffkizer geoffkizer geoffkizer approved these changes

@ManickaP ManickaP ManickaP left review comments

@ericstj ericstj ericstj left review comments

@ViktorHofer ViktorHofer ViktorHofer left review comments

@stephentoub stephentoub stephentoub left review comments

@JamesNK JamesNK JamesNK left review comments

@davidfowl davidfowl davidfowl left review comments

@alnikola alnikola Awaiting requested review from alnikola

@halter73 halter73 Awaiting requested review from halter73

Assignees

@scalablecory scalablecory

Labels
area-System.Net.Http new-api-needs-documentation
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.