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

Missing System.Net.Sockets.TcpClient.Connect(IPEndPoint remoteEP) overload. #43111

Closed
MartyIX opened this issue Oct 6, 2020 · 9 comments · Fixed by #44110
Closed

Missing System.Net.Sockets.TcpClient.Connect(IPEndPoint remoteEP) overload. #43111

MartyIX opened this issue Oct 6, 2020 · 9 comments · Fixed by #44110
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Milestone

Comments

@MartyIX
Copy link
Contributor

MartyIX commented Oct 6, 2020

Background and Motivation

This issue proposes to add System.Net.Sockets.TcpClient.ConnectAsync(IPEndPoint remoteEP) method.

This issue was filed based on #40750 and comment #40750 (comment) by @geoffkizer.

Proposed API

namespace System.Net.Sockets
public class TcpClient : IDisposable
{

    public void Connect(IPAddress address, int port);
    public void Connect(IPAddress[] ipAddresses, int port);
    public void Connect(IPEndPoint remoteEP);
    public void Connect(string hostname, int port);

    public Task ConnectAsync(IPAddress address, int port);
    public Task ConnectAsync(IPAddress[] addresses, int port);
+    public Task ConnectAsync(IPEndPoint remoteEP);
    public Task ConnectAsync(string host, int port);

    public ValueTask ConnectAsync (IPAddress address, int port, CancellationToken cancellationToken);
    public ValueTask ConnectAsync (IPAddress[] addresses, int port, CancellationToken cancellationToken);
+    public ValueTask ConnectAsync(IPEndPoint remoteEP,  CancellationToken cancellationToken);
    public ValueTask ConnectAsync (string host, int port, CancellationToken cancellationToken);
}

Usage Examples

Usage example is the same as for System.Net.Sockets.TcpClient.Connect(IPEndPoint remoteEP).

Alternative Designs

N/A.

Risks

N/A

@MartyIX MartyIX added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 6, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Oct 6, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Oct 7, 2020

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

@MihaZupan
Copy link
Member

We would certainly also want the newer ValueTask-CancellationToken overload.

I assume the reason that overload was missed is because the non-cancellable overload was missing.

public ValueTask ConnectAsync(IPEndPoint remoteEP,  CancellationToken cancellationToken);

@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Oct 8, 2020
@karelz karelz added this to the 6.0.0 milestone Oct 8, 2020
@karelz karelz added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 8, 2020
@karelz
Copy link
Member

karelz commented Oct 8, 2020

Proposal updated in top post and marked as ready for review -- cc @scalablecory

@scalablecory
Copy link
Contributor

I vaguely recall that we decided to leave this one off intentionally, but I could be misremembering.

@geoffkizer
Copy link
Contributor

This just seems like an oversight. We have a sync overload for this, but not async.

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 20, 2020
@terrajobst
Copy link
Member

terrajobst commented Oct 20, 2020

Video

  • Looks good as proposed
namespace System.Net.Sockets
{
    public class TcpClient : IDisposable
    {
        //public void Connect(IPAddress address, int port);
        //public void Connect(IPAddress[] ipAddresses, int port);
        //public Task Connect(IPEndPoint remoteEP);
        //public void Connect(string hostname, int port);

        //public Task ConnectAsync(IPAddress address, int port);
        //public Task ConnectAsync(IPAddress[] addresses, int port);
        //public Task ConnectAsync(string host, int port);
        public Task ConnectAsync(IPEndPoint remoteEP);

        //public ValueTask ConnectAsync(IPAddress address, int port, CancellationToken cancellationToken);
        //public ValueTask ConnectAsync(IPAddress[] addresses, int port, CancellationToken cancellationToken);
        //public ValueTask ConnectAsync(string host, int port, CancellationToken cancellationToken);
        public ValueTask ConnectAsync(IPEndPoint remoteEP, CancellationToken cancellationToken);
    }
}

@karelz
Copy link
Member

karelz commented Oct 20, 2020

@MartyIX are you interested in submitting a PR?

@MartyIX
Copy link
Contributor Author

MartyIX commented Oct 21, 2020

@karelz I would kind of love to try but I don't have too much time lately on hand. So I would let it up-for-grabs.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants