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.

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

TcpClient.ConnectAsync(EndPoint) #44110

Merged
merged 3 commits into from
Nov 3, 2020

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Oct 31, 2020

Tentative attempt to fix #43111.

I'm not familiar with this code so it may be totally off. I'm not sure. Feedback is welcome. Or if somebody wants to finish the job, I'm not against.

Friendly ping @karelz @geoffkizer

@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.

@ghost
Copy link

ghost commented Oct 31, 2020

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

@dnfadmin
Copy link

dnfadmin commented Oct 31, 2020

CLA assistant check
All CLA requirements met.

@ghost
Copy link

ghost commented Oct 31, 2020

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

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

A few comments, but otherwise LGTM. Thanks.

@antonfirsov

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM

@MartyIX MartyIX marked this pull request as ready for review November 1, 2020 16:08
@geoffkizer
Copy link
Contributor

We really should have cancellation tests for the cancellation overloads, but since we don't today it's fine to skip them. Since it's literally just a pass-through to Socket it's not a big deal.

@geoffkizer geoffkizer merged commit 3ae7366 into dotnet:master Nov 3, 2020
@geoffkizer
Copy link
Contributor

Thanks @MartyIX!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 6, 2020
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
Sign up for free to 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.

Missing System.Net.Sockets.TcpClient.Connect(IPEndPoint remoteEP) overload.
7 participants