Remove System.Net.Connections and related features by geoffkizer · Pull Request #41648 · dotnet/runtime

geoffkizer · 2020-09-01T01:18:11Z

System.Net.Connections is being removed from 5.0, along with the features in SocketsHttpHandler that relied on this (ConnectionFactory and PlaintextFilter).

To simplify the removal, I added a simplified internal version of SocketConnectionFactory to SocketsHttpHandler.

NetworkException/Error is removed also. I moved these to System.Net.Connections so that code can continue to work with minimal changes. The places in NetworkStream etc that were using NetworkException have been reverted to IOException.

@dotnet/ncl please review

@ericstj @danmosemsft FYI. Also @ericstj could you review and ensure my project-level changes are correct?

ghost · 2020-09-01T01:18:13Z

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

Dotnet-GitSync-Bot · 2020-09-01T01:18:14Z

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.

geoffkizer · 2020-09-01T01:19:32Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-09-01T01:19:46Z

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP · 2020-09-01T07:17:53Z

        protected virtual System.Net.Sockets.Socket CreateSocket(System.Net.Sockets.AddressFamily addressFamily, System.Net.Sockets.SocketType socketType, System.Net.Sockets.ProtocolType protocolType, System.Net.EndPoint? endPoint, System.Net.Connections.IConnectionProperties? options) { throw null; }
    }
 }
+namespace System.Net


How does it work if we remove System.Net.Connections from shared framework but still depend on it? Or is it still present? Do we still expose it to the user?

It's not exposed to the user. It's still part of the tree, but not dropped.

karelz · 2020-09-01T10:29:48Z

Thanks @geoffkizer! This PR is against master, I wonder if we should make it directly against release/5.0 branch ...

geoffkizer · 2020-09-01T15:43:54Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-09-01T15:44:08Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2020-09-01T17:11:55Z

-        public async Task Connection_Stream_FailingOperation_ThowsNetworkException()
-        {
-            using var server = SocketTestServer.SocketTestServerFactory(SocketImplementationType.Async, IPAddress.Loopback);
-            using SocketsConnectionFactory factory = new SocketsConnectionFactory(SocketType.Stream, ProtocolType.Tcp);
-            using Connection connection = await factory.ConnectAsync(server.EndPoint);
-
-            connection.ConnectionProperties.TryGet(out Socket socket);
-            Stream stream = connection.Stream;
-            socket.Dispose();
-
-            Assert.Throws<NetworkException>(() => stream.Read(new byte[1], 0, 1));
-            Assert.Throws<NetworkException>(() => stream.Write(new byte[1], 0, 1));
-        }
-
-        [Fact]
-        public async Task Connection_Pipe_FailingOperation_ThowsNetworkException()
-        {
-            using var server = SocketTestServer.SocketTestServerFactory(SocketImplementationType.Async, IPAddress.Loopback);
-            using SocketsConnectionFactory factory = new SocketsConnectionFactory(SocketType.Stream, ProtocolType.Tcp);
-            using Connection connection = await factory.ConnectAsync(server.EndPoint);
-
-            connection.ConnectionProperties.TryGet(out Socket socket);
-            IDuplexPipe pipe = connection.Pipe;
-            socket.Dispose();
-
-            await Assert.ThrowsAsync<NetworkException>(() =>  pipe.Input.ReadAsync().AsTask());
-            await Assert.ThrowsAsync<NetworkException>(() => pipe.Output.WriteAsync(new byte[1]).AsTask());
-        }


If we are keeping the rest of the code anyways, can we preserve these tests by refactoring the assertions for IOException? I would prefer not to loose them.

As far as I can tell, the only reason these tests exist is to check for NetworkException specifically. So I'm not sure what we get by checking for IOException.

I agree that the assertion is less valuable in it's current form, but is it more likely that we are about to re-introduce NetworkException eventually? In that case keeping them around will ensure that we don't loose them by accident.

Depending on the direction we go, we will make a branch from master that reverts this PR and continue to iterate the API --so, I don't think we will lose anything.

scalablecory

lgtm.

@joperezr @safern @ericstj can you verify this change removes the System.Net.Connections assembly from final distro, and that System.IO.Pipeline is back to being out of net50 apps?

safern · 2020-09-01T21:12:31Z

Why are we doing this against master? Do we want this on 6.0?

JamesNK · 2020-09-01T21:34:09Z

      System.IO.FileSystem.Watcher;
      System.IO.IsolatedStorage;
      System.IO.MemoryMappedFiles;
-      System.IO.Pipelines;


Is removing pipelines reference here going to require a reaction in aspnetcore? Did we make changes to not reference it ourselves? @halter73 @Tratcher @JunTaoLuo

Co-authored-by: Cory Nelson <phrosty@gmail.com>

….Tasks.cs Co-authored-by: Cory Nelson <phrosty@gmail.com>

geoffkizer · 2020-09-01T22:32:50Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-09-01T22:33:02Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2020-09-02T02:31:48Z

/backport to release/5.0-rc2

github-actions · 2020-09-02T02:32:44Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/235362717

geoffkizer · 2020-09-02T02:33:02Z

/backport to release/5.0

github-actions · 2020-09-02T02:34:03Z

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/235364028

github-actions · 2020-09-02T02:34:56Z

@geoffkizer backporting to release/5.0-rc2 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: remove System.Net.Connections and related features
error: sha1 information is lacking or useless (src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 remove System.Net.Connections and related features
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!

github-actions · 2020-09-02T02:36:20Z

@geoffkizer backporting to release/5.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: remove System.Net.Connections and related features
error: sha1 information is lacking or useless (src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 remove System.Net.Connections and related features
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!

* remove System.Net.Connections and related features * cleanup csproj changes * Update src/libraries/pkg/baseline/packageIndex.json Co-authored-by: Cory Nelson <phrosty@gmail.com> * Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs Co-authored-by: Cory Nelson <phrosty@gmail.com> Co-authored-by: Geoffrey Kizer <geoffrek@windows.microsoft.com> Co-authored-by: Cory Nelson <phrosty@gmail.com>

danmoseley · 2020-09-02T04:23:33Z

@akoeplinger Do you happen to know what causes git am to give fake ancestor?

akoeplinger · 2020-09-02T13:57:46Z

Hm weird, this normally happens when the repo checkout doesn't contain the commit that the patch was built from (which shouldn't happen since we clone master and the target branch).

It works when I do it locally, but it wouldn't have helped in this case since I got a merge conflict in src/libraries/pkg/baseline/packageIndex.json so a manual backport would've been required anyway.

* remove System.Net.Connections and related features * cleanup csproj changes * Update src/libraries/pkg/baseline/packageIndex.json Co-authored-by: Cory Nelson <phrosty@gmail.com> * Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs Co-authored-by: Cory Nelson <phrosty@gmail.com> Co-authored-by: Geoffrey Kizer <geoffrek@windows.microsoft.com> Co-authored-by: Cory Nelson <phrosty@gmail.com> Co-authored-by: Geoffrey Kizer <geoffrek@windows.microsoft.com> Co-authored-by: Cory Nelson <phrosty@gmail.com>

jeffdoolittle · 2020-09-04T23:10:39Z

Is there information anywhere describing the reasoning behind the removal? Thanks.

halter73 · 2020-09-04T23:27:28Z

We intend for System.Net.Connections to be a general-purpose abstraction and not something that's only used by HttpClient/SocketsHttpHandler. To help prove System.Net.Connections is a good general-purpose abstraction, the plan is to use it to replace Kestrel's current "Bedrock" transport abstraction (dotnet/aspnetcore#13295). Unfortunately, Kestrel's adoption of System.Net.Connections did not land in time for .NET 5, and we do not want to ship an abstraction with only one consumer. System.Net.Connections is not being removed permanently, but it is being delayed until .NET 6.

jeffdoolittle · 2020-09-04T23:45:06Z

we do not want to ship an abstraction with only one consumer

This makes complete sense. Thank you for the backstory. It is very much appreciated.

mjsabby · 2020-09-04T23:57:11Z

@halter73 this is a major impact to Bing. We have taken a dependency on this in RC1 builds. What recourse do we have?

cc @karelz @jeffschwMSFT @danmosemsft

halter73 · 2020-09-05T20:05:02Z

@mjsabby If a Stream-based ConnectCallback is added to SocketsHttpHandler as described in #28721, would that meet your needs?

remove System.Net.Connections and related features

a6ad4e1

geoffkizer added the area-System.Net.Http label Sep 1, 2020

geoffkizer added this to the 5.0.0 milestone Sep 1, 2020

Dotnet-GitSync-Bot added the new-api-needs-documentation label Sep 1, 2020

ManickaP reviewed Sep 1, 2020

View reviewed changes

karelz modified the milestones: 5.0.0, 6.0.0 Sep 1, 2020

cleanup csproj changes

96134da

danmoseley requested review from ericstj and scalablecory September 1, 2020 16:42

antonfirsov reviewed Sep 1, 2020

View reviewed changes

scalablecory approved these changes Sep 1, 2020

View reviewed changes

Comment thread src/libraries/pkg/baseline/packageIndex.json Outdated

Comment thread src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs Outdated

JamesNK reviewed Sep 1, 2020

View reviewed changes

geoffkizer and others added 2 commits September 1, 2020 15:31

Update src/libraries/pkg/baseline/packageIndex.json

6574ac6

Co-authored-by: Cory Nelson <phrosty@gmail.com>

Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket…

779502a

….Tasks.cs Co-authored-by: Cory Nelson <phrosty@gmail.com>

JamesNK mentioned this pull request Sep 1, 2020

Remove inter-process communication doc dotnet/AspNetCore.Docs#19753

Merged

safern reviewed Sep 1, 2020

View reviewed changes

Comment thread src/libraries/System.Net.Connections/src/System/Net/NetworkException.cs

safern reviewed Sep 1, 2020

View reviewed changes

Comment thread src/libraries/System.Net.Primitives/ref/System.Net.Primitives.cs

safern approved these changes Sep 1, 2020

View reviewed changes

geoffkizer merged commit aaecee0 into dotnet:master Sep 2, 2020

This was referenced Sep 2, 2020

[release/5.0-rc2] Remove System.Net.Connections and related features (#41648) #41726

Merged

[release/5.0] Remove System.Net.Connections and related features (#41648) #41725

Merged

dougbu mentioned this pull request Sep 2, 2020

[master] Update dependencies from dotnet/efcore dotnet/runtime dotnet/aspnetcore#25399

Merged

JamesNK mentioned this pull request Sep 3, 2020

React to connection abstractions removal. Remove transporter grpc/grpc-dotnet#1038

Merged

expcat mentioned this pull request Sep 4, 2020

Missing System.IO.Pipelines davidfowl/BedrockFramework#97

Open

ericstj mentioned this pull request Sep 12, 2020

delete dead System.Net.Connections code #42099

Merged

geoffkizer mentioned this pull request Sep 15, 2020

.NET 5 RC1 breaks with "Address family not supported by protocol" in AWS Lambda but not in preview 8 and earlier #42233

Closed

dougbu mentioned this pull request Oct 10, 2020

Upgrade to a 6.0 SDK dotnet/efcore#22918

Merged

antonfirsov mentioned this pull request Nov 3, 2020

How to use Socket to write high-performance sending data in concurrent scenarios? #44182

Closed

geoffkizer deleted the connectionremoval branch November 7, 2020 23:40

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Conversation

geoffkizer commented Sep 1, 2020

Uh oh!

ghost commented Sep 1, 2020

Uh oh!

Dotnet-GitSync-Bot commented Sep 1, 2020

Uh oh!

geoffkizer commented Sep 1, 2020

Uh oh!

azure-pipelines Bot commented Sep 1, 2020

Uh oh!

Uh oh!

ManickaP Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

geoffkizer Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

karelz commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoffkizer commented Sep 1, 2020

Uh oh!

azure-pipelines Bot commented Sep 1, 2020

Uh oh!

antonfirsov Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

geoffkizer Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

antonfirsov Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

scalablecory Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

scalablecory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

safern commented Sep 1, 2020

Uh oh!

JamesNK Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

geoffkizer commented Sep 1, 2020

Uh oh!

azure-pipelines Bot commented Sep 1, 2020

Uh oh!

Uh oh!

Uh oh!

geoffkizer commented Sep 2, 2020

Uh oh!

github-actions Bot commented Sep 2, 2020

Uh oh!

geoffkizer commented Sep 2, 2020

Uh oh!

github-actions Bot commented Sep 2, 2020

Uh oh!

github-actions Bot commented Sep 2, 2020

Uh oh!

github-actions Bot commented Sep 2, 2020

Uh oh!

danmoseley commented Sep 2, 2020

Uh oh!

akoeplinger commented Sep 2, 2020

Uh oh!

jeffdoolittle commented Sep 4, 2020

Uh oh!

halter73 commented Sep 4, 2020

Uh oh!

jeffdoolittle commented Sep 4, 2020

Uh oh!

mjsabby commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

halter73 commented Sep 5, 2020

Uh oh!

karelz commented Sep 1, 2020 •

edited

Loading

mjsabby commented Sep 4, 2020 •

edited

Loading