Replace SocketsHttpConnectionFactory with SocketsConnectionFactory by antonfirsov · Pull Request #40506 · dotnet/runtime

antonfirsov · 2020-08-07T02:52:18Z

Contributes to #40044, resolves#40075, fixes#40361.

To avoid conflicts, this PR builds on top of #40344, which is expected to be merged first. Please ignore the changes coming from that PR.

I was a bit unsure about the requirements for delegating the Stream and Pipe creation logic to SocketsHttpConnectionFactory from the connection. Hope my solution meets the expectations.

/cc @geoffkizer @scalablecory @stephentoub @halter73 @davidfowl @JamesNK

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

…andler/Connections/SocketConnection.cs add ExceptionDispatchInfo handling Co-authored-by: Cory Nelson <phrosty@gmail.com>

address -> EndPoint in error messages Co-authored-by: Cory Nelson <phrosty@gmail.com>

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

geoffkizer · 2020-08-12T20:16:09Z

+
+ // On Windows, connection timeout takes 21 seconds. Abusing this behavior to test the cancellation logic
+ [Fact]
+ [PlatformSpecific(TestPlatforms.Windows)] 


Does this test not work on other platforms? Seems like it should.

On Linux connection "timeout" comes immediately for the non-existing IP address 1.2.3.4. It won't even work with the following code:
CancellationTokenSourcects=newCancellationTokenSource();OperationCanceledExceptionex=awaitAssert.ThrowsAnyAsync<OperationCanceledException>(async()=>{awaitfactory.ConnectAsync(doesNotExist,cancellationToken:cts.Token);cts.Cancel();});

Ok, we should add a comment explaining this

geoffkizer · 2020-08-12T20:22:52Z

 public DnsEndPointWithProperties(string host, int port, HttpRequestMessage initialRequest) : base(host, port)
 {
-InitialRequest = initialRequest;
+_initialRequest = initialRequest;


I don't quite understand why this class exists.
I get that we want to pass the HttpRequest through as a property, but why aren't we just doing that directly via SocketsConnectionFactory? Why do we subclass DnsEndPoint?
@scalablecory Thoughts?

It's the property provider (IConnectionProperties implementation) that is aware of HttpRequestMessage. SocketsConnectionFactory does not know about HTTP stuff.
We subclass DnsEndPoint to reduce allocations. (A DnsEndpoint instance has to be created anyways.)

Ah, okay, I understand now. There's a comment above but it wasn't entirely clear to me.

geoffkizer · 2020-08-12T20:27:32Z

Overall this is looking pretty good to me.

You should get an approval from @scalablecory before merging, because he understands this stuff better than I do.

geoffkizer

LGTM

scalablecory · 2020-08-14T13:27:35Z

 {
- private readonly NetworkStream _stream;
+ private readonly Socket _socket;
+ private readonly SocketsConnectionFactory _factory;


_factory is no longer needed, right?

scalablecory · 2020-08-14T13:29:30Z

- {
- return ValueTask.FromCanceled(cancellationToken);
- }
+ cancellationToken.ThrowIfCancellationRequested();


This should be reverted; it makes the exception come from the method rather than be bundled inside the ValueTask, which won't be expected by users.

scalablecory · 2020-08-14T13:31:30Z

+
+ protected override Stream CreateStream() => _stream ??= new NetworkStream(_socket, true);
+
+ protected override IDuplexPipe CreatePipe() => _pipe ??= new DuplexStreamPipe(CreateStream());


No need to override this, this is the default implementation.

scalablecory

looks good.

antonfirsov · 2020-08-14T13:53:19Z

Test failure is #40436.

antonfirsov · 2020-08-14T15:25:18Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-08-14T15:25:36Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2020-08-14T17:07:03Z

OuterLoop test failures are #32955 and #40788

Geoffrey Kizerand others added 30 commits August 4, 2020 15:42

add NetworkException
9b84cf7

Update src/libraries/System.Net.Primitives/src/Resources/Strings.resx
5b5779d
Co-authored-by: Cory Nelson <phrosty@gmail.com>

add proper serialization logic
78b6b4d

add OperationAborted and better XML comments and error strings
6b83c4e

use NetworkException in SocketsHttpConnectionFactory
298f96d

add the API manually for now
b4e10b7

add skeletons
8ab151c

Update src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpH…
bffcabb
…andler/Connections/SocketConnection.cs add ExceptionDispatchInfo handling Co-authored-by: Cory Nelson <phrosty@gmail.com>

fix casing of constant in test
b3cfa89

Update src/libraries/System.Net.Primitives/src/Resources/Strings.resx
563f86a
address -> EndPoint in error messages Co-authored-by: Cory Nelson <phrosty@gmail.com>

Update src/libraries/System.Net.Primitives/src/Resources/Strings.resx
11fc432
address -> EndPoint in error messages Co-authored-by: Cory Nelson <phrosty@gmail.com>

Address -> EndPoint in enum values
1f8d94b

move MapSocketError to a common file and fix some review issues
6d7872a

add test skeletons
b2a7b45

update NetworkStream to use NetworkException
2185ee5

remove SocketConnectionNetworkStream and use NetworkStream directly
a120d07

basic tests
0cf0a1a

Merge branch 'master' into af/SocketsConnectionFactory
ff9597b

remove InvalidEndPoint
7785efe

remove factory method and update constructor arguments
9a17607

Update src/libraries/System.Net.Primitives/src/System/Net/NetworkExce…
db769f5
…ption.cs Co-authored-by: Cory Nelson <phrosty@gmail.com>

add OperationAborted to GetExceptionMessage
c266425

WIP
728d575

compiles
a90566b

Merge branch 'master' into af/SocketsConnectionFactory-2
5f35b31

revert NetworkException stuff for now
515f224

fixes
84cde84

socket tests succeeding
3f37507

Merge branch 'networkexception' into af/SocketsConnectionFactory-2
fa5d17f

adapt NetworkException
9e4ed04

geoffkizer reviewed Aug 12, 2020
View reviewed changes

Comment threadsrc/libraries/System.Net.Sockets/ref/System.Net.Sockets.csproj Outdated

geoffkizer reviewed Aug 12, 2020
View reviewed changes

Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/System.Net.Sockets.Tests.csproj Outdated

antonfirsov mentioned this pull request Aug 13, 2020
Flaky test: Http2_MultipleConnectionsEnabled_ConnectionLimitNotReached_ConcurrentRequestsSuccessfullyHandled #40436
Closed

address review findings
c15dec3

This was referenced Aug 13, 2020
Fix: NetworkStream throwing inconsistent exceptions #40772
Merged
implement ConnectAsync overloads with CancellationToken on Socket and TcpClient #40750
Merged

antonfirsov added 3 commits August 13, 2020 16:12

tests
9c25a93

Connection_Pipe_ReadWrite_Integration
3fc17d0

better disposal
9091689

jaredpar mentioned this pull request Aug 13, 2020
OSX machines are de-provisioned during CI / PR runs leading to failures #34472
Closed

remove Stream and Pipe factory methods from SocketsConnectionFactory
0b0dde2

geoffkizer approved these changes Aug 13, 2020
View reviewed changes

try to fix test flakyness
72d3a21

scalablecory reviewed Aug 14, 2020
View reviewed changes

scalablecory approved these changes Aug 14, 2020
View reviewed changes

antonfirsov added 2 commits August 14, 2020 16:04

simplify SocketConection
1c9980c

move Connection_Pipe_ReadWrite_Integration to OuterLoop
5e5917b

antonfirsov merged commit 8e5da14 into masterAug 14, 2020

jkotas deleted the af/SocketsConnectionFactory-2 branch August 22, 2020 14:29

runfoappBot mentioned this pull request Sep 29, 2020
Test initial triage jaredpar/runfo#50
Closed

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


		protected override Stream CreateStream() => _stream ??= new NetworkStream(_socket, true);

		protected override IDuplexPipe CreatePipe() => _pipe ??= new DuplexStreamPipe(CreateStream());

Uh oh!

Conversation

antonfirsov commented Aug 7, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

geoffkizer commented Aug 12, 2020

Uh oh!

geoffkizer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scalablecory left a comment

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Aug 14, 2020

Uh oh!

antonfirsov commented Aug 14, 2020

Uh oh!

azure-pipelinesBot commented Aug 14, 2020

Uh oh!

antonfirsov commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

antonfirsov commented Aug 7, 2020•
edited
Loading